feat(browser): add browser.disconnected event #960

sukrosono · 2017-10-05T15:49:58Z

This patch add browser.disconnected event :

when browser.close() called.
when browser.disconnect() called.
fix Add browser.closed and page.closed events #952

sukrosono · 2017-10-05T15:53:22Z

this just take one point on existing issues.
When the other one still open i will take care of it when i got free time.

Out of context

My attempt was add beforeEach block under Page.close suite, but the linting yell Unexpected token browser with plain function or arrow syntax. So i giving up on that.

aslushnikov · 2017-10-06T00:22:50Z

docs/api.md

 ```

+#### event: 'closed'
+Emitted when page closed or crashed.


nit: Emitted when page gets closed or page renderer crashes.

aslushnikov · 2017-10-06T00:23:02Z

lib/Page.js

    return this.mainFrame().title();
  }

+  _closed() {


let's inline this method

aslushnikov · 2017-10-06T00:27:36Z

test/test.js

+    it('should emmited when page got crashed', SX(async function() {
+      let emitted = false;
+      page.on('closed', () => emitted = true);
+      page.goto('chrome://crash');


you should be able to await page.goto here, instead of doing the waitForEvents on the following line

it fail on my machine if i do await page.goto(url); , with jasmine timeout.

here the travis test failure. That's why i wait on error, maybe you can assist me?

sukrosono · 2017-10-07T20:12:23Z

@aslushnikov dude, i need help on browser.closed, when it crashes. I have no idea how to make the browser crashes and how to make spec about it. I already trying a couple of hour but no result.

ebidel · 2017-10-07T20:22:18Z

@brutalcrozt the page chrome://crash/ will crash the browser. you can use that for testing!

sukrosono · 2017-10-09T04:35:55Z

@ebidel Thanks Eric, i notice that now. But surely i still don't know how to make browser._onCrash. Maybe that because my lack of understanding the relation browser and page in the codebase.

sukrosono · 2017-10-19T19:40:10Z

Since now we get Browser.pages and Browser.targets ,

Browser.closed now should only fired when get closed via Browser.close.
ping @aslushnikov for review, fix #952

aslushnikov · 2017-10-20T01:59:17Z

lib/Browser.js

 }

+Browser.Events = {
+  Closed: 'closed'


This should be called 'disconnected' and should be emitted whenever the connection is closed

Done, thanks for put a trust in me.

aslushnikov · 2017-10-20T17:02:25Z

lib/Page.js

  }

  _onTargetCrashed() {
+    this._closed();


there should be no need to emit the error event now; it should be just 'crashed'

cc @JoelEinbinder

aslushnikov · 2017-10-20T17:02:59Z

lib/Page.js

    return this.mainFrame().title();
  }

+  _closed() { this.emit(Page.Events.Closed, 'page closed'); }


the 'closed' event is not needed any more since we have target events; they should cover everything

sukrosono · 2017-10-21T03:45:27Z

@aslushnikov IDK why this PR fail on node 7 when running yarn run lint.

aslushnikov · 2017-10-23T18:39:43Z

lib/Browser.js


  disconnect() {
    this._connection.dispose();
+    this._disconnected();


the event should also be fired whenever the connection itself gets disconnected (e.g. whenever the browser gets closed)

@aslushnikov Sorry, i don't get it. Could you re-explain it to me? 😕

aslushnikov · 2017-10-23T18:40:59Z

lib/Browser.js

    return version.product;
  }

+  _disconnected() { this.emit(Browser.Events.Disconnected, 'connection disconnected');}


nit: formatting here is improper. The "inlining" is to remove the function _disconnected and just emit the event in disconnected method right away.

sukrosono · 2017-10-28T12:43:54Z

ping @aslushnikov for review

aslushnikov · 2017-10-31T05:01:31Z

lib/Browser.js


  disconnect() {
    this._connection.dispose();
+    this.emit(Browser.Events.Disconnected, 'connection disconnected');


this event should be emitted when the browser gets closed; e.g. when it crashes.

Done, let me know if it still wrong. So it's introduce breaking changes?

Wait, I think there's some miscommunication here.

I actually meant the browser crash, not the page crash.

The only signal we get when the browser crashes is the disconnect of web socket (see lib/Connection.js). We should capture this event and dispatch the disconnected for it

Okay, we take a look on it. I also need your advice to reflect the documentation changes for this one. FYI build fail because travis are down.

I hope that was the right one, i skip the test for this case.

aslushnikov · 2017-10-31T18:56:36Z

test/test.js

+      newBrowser.disconnect();
+      expect(isDisconnected).toBe(true);
+    }));
+    xit('should be emitted when underlying websocket gets closed', SX(async function() {


you should be able to:

launch a browser

connect to it with pptr.connect

close the browser, make sure the connected one issues the disconnected event

Let me know if it still not fit, is that a parrot in your profile picture?

aslushnikov · 2017-10-31T18:56:53Z

lib/Browser.js

    return version.product;
  }

+


nit: stray line

aslushnikov · 2017-10-31T18:57:49Z

lib/Browser.js

    /** @type {Map<string, Target>} */
    this._targets = new Map();
+    this._connection.on('ws.closed', closed => {
+      this.disconnect();


no need to run this.disconnect() here - you only need to emit the event.

sukrosono · 2017-11-01T08:55:02Z

docs/api.md

 });
 ```
+#### event: 'disconnected'
+emitted whenever the connection is closed `Browser.close() Browser.disconnect()`.


Not sure this one, let me know if any better suggestion.

aslushnikov

We're getting close!

aslushnikov · 2017-11-03T04:13:02Z

docs/api.md

 });
 ```
+#### event: 'disconnected'
+emitted whenever the connection is closed `Browser.close() Browser.disconnect()`.


Let's do the following wording:

Emitted when puppeteer gets disconnected from the browser instance. This might happen because one of the following:

browser closed or crashed

browser.disconnect method was called

aslushnikov · 2017-11-03T04:18:54Z

lib/Browser.js

    this._closeCallback = closeCallback || new Function();
    /** @type {Map<string, Target>} */
    this._targets = new Map();
+    this._connection.on('ws.closed', closed => {


we shouldn't whitelist certain events and give them special treatment.

Instead, let's introduce a "closedCallback" for the connection and initialize it here:

this._connection.setClosedCallback(() => { this.emit(Browser.Events.Disconnected); });

aslushnikov · 2017-11-03T04:19:42Z

lib/Browser.js

    /** @type {Map<string, Target>} */
    this._targets = new Map();
+    this._connection.on('ws.closed', closed => {
+      this.emit(Browser.Events.Disconnected, 'connection disconnected');


let's drop the argument for the event, it's unclear if it's useful.

aslushnikov · 2017-11-03T04:20:54Z

lib/Browser.js


  disconnect() {
    this._connection.dispose();
+    this.emit(Browser.Events.Disconnected, 'connection disconnected');


no need to emit here: this will result in disconnected event dispatched twice (one from here, another from the connection been closed)

aslushnikov · 2017-11-03T04:22:29Z

lib/Connection.js


-  _onClose() {
+  _onClose(code, reason) {
+    if (code) this.emit('ws.closed', [code, reason]);


let's emit as the very last line in the method. This way, if there's an exception in user-land code, we won't be left in a non-consistent state.

We should emit unconditionally

Let's not emit any details for now

done, hopefully i am not misunderstand again.

aslushnikov · 2017-11-03T04:24:30Z

test/test.js

+        isDisconnected = true;
+      });
+      newBrowser.disconnect();
+      expect(isDisconnected).toBe(true);


please, close originalBrowser here so that it's not left dangling

aslushnikov · 2017-11-03T04:28:31Z

test/test.js

+        isDisconnected = true;
+        resolver();
+      });
+      browser._connection._ws.close(3006, 'intentionally kill it');


we should not reach to private variables in tests.
Instead, this test could be written like the previous one, but instead of disconnecting
the new browser, closing original one:

it('should be emitted when underlying websocket gets closed', SX(async function() { const browser = await puppeteer.launch(defaultBrowserOptions); const newBrowser = await puppeteer.connect({ browserWSEndpoint: originalBrowser.wsEndpoint() }); let isDisconnected = false; newBrowser.once('disconnected', () => isDisconnected = true); await originalBrowser.close(); expect(isDisconnected).toBe(true); }));

Sorry i didn't know that, i just try to get closer to

should be emitted when underlying websocket gets closed

Most of my time yesterday was spend here, i wish know it earlier. Next time i should be more clever on testing.

@brutalcrozt feel free to ask questions next time if you're getting stuck.

aslushnikov · 2017-11-03T08:53:40Z

lib/Connection.js

-}
-
-module.exports = {Connection, Session};
+/**


Are these different line-endings? Can you please revert the change so that the actual changes are clear?

Perhaps, certainly i will do.

sukrosono · 2017-11-03T09:09:02Z

test/test.js

  });

+  describe('Browser.Events.disconnected', function() {
+    it('should emitted when browser gets closed', SX(async function() {


this one fired disconnected twice, i notice that. Same effect when i use the browser, Assuming it added to two instance the browser and originalBrowser. For me it's important you to know. If i am wrong please leave some suggestion.

Good catch!

That's because:

Connection should reset it's this._ws to null once the COnnection._onClose method is called.

Connection._onClose should remove added event listeners (use helper.addEventListener / helper.removeEventListeners for this)

The Connection.dispose() method should fast-return if this._ws === null.

I am not fully understand it, but i do the trick. Hopefully good to go 😄

sukrosono · 2017-11-03T09:16:36Z

@aslushnikov This one is ready captain. 😄

aslushnikov · 2017-11-03T09:18:08Z

lib/Connection.js


+  /**
+   * @param {function()} callback
+   */


can you please format this?

setClosedCallback(callback) { this._closeCallback = callback; }

Done, so no inline on class method? Inline only on normal code?

"Inlining" is not about code formatting.
To "inline a function" means to get the function body and paste it instead of function invocations.

More on wiki: inline expanstion

aslushnikov · 2017-11-03T09:20:23Z

lib/Connection.js

  }

  _onClose() {
+    this._closeCallback();


this should be the last line in the method

for the sake of cleanliness, let's check that delegate is set:

if (this._closeCallback) this._closeCallback();

aslushnikov · 2017-11-03T09:45:31Z

test/test.js

  });

+  describe('Browser.Events.disconnected', function() {
+    it('should emitted when browser gets closed', SX(async function() {


Good catch!

That's because:

Connection should reset it's this._ws to null once the COnnection._onClose method is called.

Connection._onClose should remove added event listeners (use helper.addEventListener / helper.removeEventListeners for this)

The Connection.dispose() method should fast-return if this._ws === null.

aslushnikov · 2017-11-03T09:51:55Z

test/test.js

+      await originalBrowser.close();
+      expect(isDisconnected).toBe(true);
+    }));
+    it('should emitted when browser.disconnect() called', SX(async function() {


Let's unify these three tests into one. This will save us on some time while running tests.

const originalBrowser = await puppeteer.launch(defaultBrowserOptions); const browserWSEndpoint = originalBrowser.wsEndpoint(); const remoteBrowser1 = await puppeteer.connect({browserWSEndpoint}); const remoteBrowser2 = await puppeteer.connect({browserWSEndpoint}); let disconnectedOriginal = 0; let disconnectedRemote1 = 0; let disconnectedRemote2 = 0; originalBrowser.on('disconnected', () => ++disconnectedOriginal); remoteBrowser1.on('disconnected', () => ++disconnectedRemote1); remoteBrowser2.on('disconnected', () => ++disconnectedRemote2); await remoteBrowser2.disconnect(); expect(disconnectedOriginal).toBe(0); expect(disconnectedRemote1).toBe(0); expect(disconnectedRemote2).toBe(1); await originalBrowser.close(); expect(disconnectedOriginal).toBe(1); expect(disconnectedRemote1).toBe(1); expect(disconnectedRemote2).toBe(1);

aslushnikov · 2017-11-04T01:43:18Z

Thanks!

sukrosono added 8 commits September 27, 2017 00:28

Allow options.timeout to be 0 to disable timeout

fa81395

update related documentation

29ac71d

drop unnecessary comment

c8b3b55

change testing spec to grid.html instead of empty.html

848aebb

Merge branch 'master' of https://github.com/GoogleChrome/puppeteer

15333f3

Merge branch 'master' of https://github.com/GoogleChrome/puppeteer

87c32df

Add page.closed event

35f3922

Fire page.closed event when page got crashed.

245a942

aslushnikov reviewed Oct 6, 2017

View reviewed changes

sukrosono added 5 commits October 6, 2017 17:59

revise from review 1

7a07ff9

Merge branch 'master' of https://github.com/GoogleChrome/puppeteer

57b1273

add browser.closed events

66e3e7d

switch back to working Page.Events.closed test spec

069b24f

mend

52f66df

sukrosono added 5 commits October 11, 2017 11:59

Merge branch 'master' into master

20cd113

close browser when page got crashes

ea33739

Merge branch 'master' of https://github.com/GoogleChrome/puppeteer

6081c95

Merge branch 'master' of https://github.com/GoogleChrome/puppeteer

32cb39e

add test for Browser.closed

57a7633

aslushnikov reviewed Oct 20, 2017

View reviewed changes

Merge branch 'master' of https://github.com/GoogleChrome/puppeteer

40d510f

sukrosono changed the title ~~feat(page): add page.closed event~~ feat(browser): add browser.disconnected event Oct 21, 2017

revision from review 2

4c5f3ec

aslushnikov reviewed Oct 23, 2017

View reviewed changes

sukrosono added 3 commits October 24, 2017 03:06

revision from review 3

587f587

fix linebreaks

40ee644

Merge branch 'master' of https://github.com/GoogleChrome/puppeteer

312f38e

aslushnikov reviewed Oct 31, 2017

View reviewed changes

revision from review 4

9efff54

aslushnikov mentioned this pull request Oct 31, 2017

Unable to deploy Puppeteer - Unzipped size must be smaller than 262144000 bytes. - AWS Lambda #603

Closed

revise browser crash

76a796e

aslushnikov reviewed Oct 31, 2017

View reviewed changes

revis add test spec

13befdc

sukrosono commented Nov 1, 2017

View reviewed changes

remove unused export to hit green and hopefully be merged

30a4c13

aslushnikov reviewed Nov 3, 2017

View reviewed changes

revise connection.setClosedCallback

b724ddf

aslushnikov reviewed Nov 3, 2017

View reviewed changes

fix wrong line ending

20e328d

sukrosono commented Nov 3, 2017

View reviewed changes

aslushnikov reviewed Nov 3, 2017

View reviewed changes

Revise, prevent redundant call to _closeCallback

10d22b3

aslushnikov approved these changes Nov 4, 2017

View reviewed changes

aslushnikov merged commit 03fefb5 into puppeteer:master Nov 4, 2017

feat(browser): add browser.disconnected event #960

feat(browser): add browser.disconnected event #960

Uh oh!

Conversation

sukrosono commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sukrosono commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Out of context

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sukrosono Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sukrosono commented Oct 7, 2017

Uh oh!

ebidel commented Oct 7, 2017

Uh oh!

sukrosono commented Oct 9, 2017

Uh oh!

sukrosono commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sukrosono commented Oct 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sukrosono commented Oct 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sukrosono Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aslushnikov left a comment

Choose a reason for hiding this comment

Uh oh!

sukrosono commented Oct 5, 2017 •

edited

Loading

sukrosono commented Oct 5, 2017 •

edited

Loading

sukrosono Oct 6, 2017 •

edited

Loading

sukrosono Nov 1, 2017 •

edited

Loading

sukrosono Nov 3, 2017 •

edited

Loading