Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 6 years ago

For a while we already see bug 1383686 which is an intermittent failure during closing tabs, and where we return too early.

As the example on MDN shows the event we make use of to listen for a tab to be closed is wrong. `TabClose` is actually used to indicate that the tab is about to be closed: https://developer.mozilla.org/en-US/docs/Web/Events/TabClose#Example

Due to that we have a race condition for closing tabs in general by using the `WebDriver:close` method.

Dao, is there a specific event which would indicate that the tab has been gone, or would we have to poll and check if the tab to be closed is no longer listed in the tabs list?

	Dão Gottwald [:dao]
	Comment 1 • 6 years ago

It depends on what exactly you want to do when a tab closes. If you want to know exactly when a page is unloaded, you can use a frame script for that. If you want to know when gBrowser.tabs is updated, then there's no specific event for that. If you're interested in other information updates, then "tab has closed" may not be the best proxy for that, see bug 1444007 for instance.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 6 years ago

We clearly have to wait until the tab is visually gone from the UI, and all of its references are gone. Which means that we have to wait that `gBrowser.tabs` got updated, right? If yes, polling that list for the tab we closed seem to be the only solution then.

	Dão Gottwald [:dao]
	Comment 3 • 6 years ago

(In reply to Henrik Skupin (:whimboo) from comment #2)
> We clearly have to wait until the tab is visually gone from the UI,

To me that's not clear, but if you say so, sure. Polling might be the most practical solution then.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 6 years ago

I actually found a better way as what is used for Mochitests. Given that we have framescripts attached to each tab, we can wait until the appropriate message manager has been destroyed:

https://dxr.mozilla.org/mozilla-central/rev/7ff499dfcd51cf4a95ebf0db506b415bf7bb27c3/testing/mochitest/browser-test.js#29-63

Using such a method I can prove that we return too early at the moment for both closing a tab and a window. With the behavior in place I'm able to delay the return of the command until the underlying tab has been closed.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 5 • 6 years ago

Here the logs for both closing a tab and window with and without the upcoming patch:

Without the patch:
------------------

Closing tab:
> 1523954642707	Marionette	TRACE	3 -> [0,19,"WebDriver:ElementClick",{"id":"8be25976-1361-3647-b678-9fcac5121773"}]
> 1523954642806	Marionette	DEBUG	Received DOM event beforeunload for http://127.0.0.1:61920/clicks.html
> 1523954642846	Marionette	DEBUG	Received DOM event TabClose for [object XULElement]
> 1523954642867	Marionette	TRACE	3 <- [1,19,null,{}]
> 1523954642869	Marionette	DEBUG	Received DOM event pagehide for http://127.0.0.1:61920/clicks.html
> 1523954642870	Marionette	DEBUG	Received DOM event unload for http://127.0.0.1:61920/clicks.html
> 1523954642872	Marionette	DEBUG	Received observer notification outer-window-destroyed for 2147483653

Closing window:
> 1523954644458	Marionette	TRACE	4 -> [0,22,"WebDriver:ElementClick",{"id":"25d372c3-9288-e84d-a38e-560c87062bb3"}]
> 1523954645037	Marionette	DEBUG	Received DOM event beforeunload for http://127.0.0.1:61920/clicks.html
> 1523954645076	Marionette	DEBUG	Received DOM event unload for [object XULDocument]
> 1523954645077	Marionette	TRACE	4 <- [1,22,null,{}]
> 1523954645093	Marionette	DEBUG	Received DOM event pagehide for http://127.0.0.1:61920/clicks.html
> 1523954645095	Marionette	DEBUG	Received DOM event unload for http://127.0.0.1:61920/clicks.html
> 1523954645098	Marionette	DEBUG	Received observer notification outer-window-destroyed for 2147483661

With the patch:
---------------

Closing tab:
> 1523954316668	Marionette	TRACE	3 -> [0,19,"WebDriver:ElementClick",{"id":"33573867-c0f5-6446-b10d-7222e45b8a01"}]
> 1523954316728	Marionette	DEBUG	Received DOM event beforeunload for http://127.0.0.1:61713/clicks.html
> 1523954316768	Marionette	DEBUG	Received DOM event TabClose for [object XULElement]
> 1523954316790	Marionette	DEBUG	Received DOM event pagehide for http://127.0.0.1:61713/clicks.html
> 1523954316791	Marionette	DEBUG	Received DOM event unload for http://127.0.0.1:61713/clicks.html
> 1523954316849	Marionette	TRACE	3 <- [1,19,null,{}]

Closing window:
> 1523954318653	Marionette	TRACE	4 -> [0,22,"WebDriver:ElementClick",{"id":"bf3a54f9-e0ef-614d-a9a2-8c6f8decf05e"}]
> 1523954318683	Marionette	DEBUG	Received DOM event beforeunload for http://127.0.0.1:61713/clicks.html
> 1523954318721	Marionette	DEBUG	Received DOM event unload for [object XULDocument]
> 1523954318738	Marionette	DEBUG	Received DOM event pagehide for http://127.0.0.1:61713/clicks.html
> 1523954318739	Marionette	DEBUG	Received DOM event unload for http://127.0.0.1:61713/clicks.html
> 1523954318779	Marionette	TRACE	4 <- [1,22,null,{}]

As it is visible in both cases we currently return a couple of milliseconds too early. For slow running builds the time difference can even be larger, and will result in this race condition.

The patch will remove the registered observer for `outer-window-destroyed` from `listener.js` because it is useless. At this time the message manager has already been killed, and we cannot send a response at all to the parent process.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 6 years ago

Note that this doesn't only affect `WebDriver:Close` as stated above but as my last comment showed also `WebDriver:ElementClick`. Here some stats for `WebDriver:Close`:

Without the patch:
------------------

Closing tab:
> 1523957866222	Marionette	TRACE	3 -> [0,19,"WebDriver:CloseWindow",{}]
> 1523957866293	Marionette	TRACE	3 <- [1,19,null,["2147483649"]]

Closing window (single tab open):
> 1523958200279	Marionette	TRACE	3 -> [0,22,"WebDriver:CloseWindow",{}]
> 1523958200334	Marionette	TRACE	3 <- [1,22,null,["2147483649"]]

With the patch:
---------------

Closing tab:
> 1523957633669	Marionette	TRACE	3 -> [0,20,"WebDriver:CloseWindow",{}]
> 1523957634508	Marionette	TRACE	3 <- [1,20,null,["2147483649"]]

Closing window (single tab open):
> 1523958350856	Marionette	TRACE	3 -> [0,22,"WebDriver:CloseWindow",{}]
> 1523958350972	Marionette	TRACE	3 <- [1,22,null,["2147483649"]]

Without the patch we return within 70ms, while it takes up to 900ms to actually remove the tab from the UI, or 120ms to close the window. Interesting that closing a window with a single tab open is faster than closing one of multiple tabs.

	Code Review Bot [:reviewbot]
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8968461 [details]
Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window.

https://reviewboard.mozilla.org/r/237172/#review242954


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/browser.js:457
(Diff revision 1)
>  };
>  
>  /**
> + * Class to detect when a top-level browsing context has been completely gone.
> + *
> + * If a top-level browsing context is about to close appropriate `TabClose` and

Error: Line 457 exceeds the maximum line length of 78. [eslint: max-len]

::: testing/marionette/browser.js:484
(Diff revision 1)
> +  unregister() {
> +    Services.obs.removeObserver(this, "message-manager-close");
> +    Services.obs.removeObserver(this, "message-manager-disconnect");
> +  }
> +
> +  observe(subject, topic, data) {

Error: 'data' is defined but never used. [eslint: no-unused-vars]

::: testing/marionette/proxy.js:13
(Diff revision 1)
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  const {
> +  TabDestroyObserver,
> +} = ChromeUtils.import("chrome://marionette/content/browser.js");

Error: Cu.import imports into variables and into global scope. [eslint: mozilla/no-import-into-var-and-global]

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 6 years ago

(In reply to Henrik Skupin (:whimboo) from comment #5)
> The patch will remove the registered observer for `outer-window-destroyed`
> from `listener.js` because it is useless. At this time the message manager

This is actually wrong, and the last try build shows that. This observer is used to detect removal of frames, and as such has to remain.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 6 years ago

As it looks like Android is not happy:

https://treeherder.mozilla.org/logviewer.html#?job_id=174081207&repo=try&lineNumber=1874

It hangs most likely in the promise to wait for the tab (message manager) to be gone. I can verify that this is not happening due to non-e10s mode, but maybe the Java stack handles that differently.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 13 • 6 years ago

For Fennec the BrowserApp [1] class is used in Marionette, and specifically its `closeTab` method [2]. As it can be seen in the code it automatically selects the next tab, and then dispatches a "Tab:Closed" event. This event gets picked up in `onEvent` [3], and triggers `_handleTabClosed()` [4]. Only here the `TabClose` event is getting fired, while BrowserApp was updated before. So I assume it will be fine to not have to further wait.

I will retrigger the Mn10 job on Android for now to see if that is a permanent failure or just an intermittent.

[1] https://developer.mozilla.org/en-US/docs/Archive/Add-ons/Legacy_Firefox_for_Android/API/BrowserApp/closeTab
[2] https://dxr.mozilla.org/mozilla-central/rev/6276ec7ebbf33e3484997b189f20fc1511534187/mobile/android/base/java/org/mozilla/gecko/Tabs.java#453-473
[3] https://dxr.mozilla.org/mozilla-central/rev/6276ec7ebbf33e3484997b189f20fc1511534187/mobile/android/chrome/content/browser.js#1978-1980
[4] https://dxr.mozilla.org/mozilla-central/rev/6276ec7ebbf33e3484997b189f20fc1511534187/mobile/android/chrome/content/browser.js#1251-1291

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 14 • 6 years ago

So it perma fails on Android. Interesting is that it only happens for `test_click_close_tab`, which triggers closing the tab by clicking a link. It doesn't fail for closing the window, nor for `WebDriver:close`.

	Andreas Tolfsen ❲:ato❳
	Comment 15 • 6 years ago
mozreview-review

Comment on attachment 8968461 [details]
Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window.

https://reviewboard.mozilla.org/r/237172/#review243008

Pre-liminary review.  I might have some more comments about the API
once you explain to me how TabDestroyObserver works.

In a passing note, do we have tests for this?

::: testing/marionette/browser.js:454
(Diff revision 3)
>  /**
> + * Class to detect when a top-level browsing context has been completely gone.
> + *
> + * If a top-level browsing context is about to close appropriate `TabClose`
> + * and `unload` events are fired. But there are no existent events which
> + * indicate that the browsing context is completely gone.
> + *
> + * Therefore the closing and disconnect state of the current message manager
> + * can be observed, and used to determine the final closed state.
> + */
> +this.TabDestroyObserver = class {

It feels like this class more naturally belongs in
testing/marionette/wm.js.  Because I’m changing code in this general
area as part of the window tracking patch, would you mind if we
moved it there – despite it being the only thing in there for the
moment?

::: testing/marionette/browser.js:470
(Diff revision 3)
> +  register() {
> +    this.outstanding = new Set();
> +    this.promiseResolver = null;
> +    this.seenClose = false;
> +
> +    Services.obs.addObserver(this, "message-manager-close");
> +    Services.obs.addObserver(this, "message-manager-disconnect");
> +  }

Make this re-entrant safe.  If register() has already been called
we shouldn’t add the observers again.  This would cause the observers
to fire twice.

::: testing/marionette/browser.js:479
(Diff revision 3)
> +  unregister() {
> +    Services.obs.removeObserver(this, "message-manager-close");
> +    Services.obs.removeObserver(this, "message-manager-disconnect");
> +  }

Same here with regards to re-entry.

::: testing/marionette/browser.js:484
(Diff revision 3)
> +  observe(subject, topic) {
> +    switch (topic) {
> +      case "message-manager-close":
> +        this.outstanding.add(subject);
> +        break;

Please correct me if I’m wrong, but the observers will fire for
_any browser_ that is closed.  This means the tab we are waiting
on to be closed must be passed to TabDestroyObserver so it can
compare the subject with the expected browser.

Pseudo-code:

> observe(subject, topic) {
>   if (subject !== this.browser) {
>     return;
>   }
> 
>   switch (topic) {
>     …
>   }
> }

::: testing/marionette/proxy.js:151
(Diff revision 3)
> -        switch (type) {
> +        new TabDestroyObserver().wait().then(() => {
> -          case "TabClose":
> -          case "unload":
> -            this.removeHandlers();
> +          this.removeHandlers();
> -            resolve();
> +          resolve();
> -            break;
> +        });

Maybe this API would look better?

> await TabDestroyObserver.wait(tab);
> this.removeHanlders();
> resolve();

	Andreas Tolfsen ❲:ato❳
	Comment 16 • 6 years ago

Comment on attachment 8968461 [details]
Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window.

I don’t know how mozreview managed to set r?ato on this.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 17 • 6 years ago

Here the events from the log:

> 1523982019539 Marionette  TRACE   66 -> [0,18,"WebDriver:ElementClick",{"id":"467739fb-6a51-4ad3-8509-753521ec092c"}]
> 1523982020506 Marionette  DEBUG   Received DOM event beforeunload for http://172.17.0.5:45038/clicks.html
> 1523982021157 Marionette  DEBUG   Received DOM event TabClose for [object XULElement]
> 1523982021250 Marionette  DEBUG   Received observer notification message-manager-close
> 1523982021392 Marionette  DEBUG   Received DOM event pagehide for http://172.17.0.5:45038/clicks.html
> 1523982021420 Marionette  DEBUG   Received DOM event unload for http://172.17.0.5:45038/clicks.html
> 1523982021994 Marionette  DEBUG   Received observer notification outer-window-destroyed for 33
> 1523982022099 Marionette  DEBUG   Received observer notification message-manager-disconnect
[..]
> 1523982379856	Marionette	DEBUG	Closed connection 66

The above shows that we have clearly seen the wanted observer notifications, and that nothing needs to be changed in regards of those. I will add more logging to the code in question to see where we hang.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 18 • 6 years ago
mozreview-review-reply

Comment on attachment 8968461 [details]
Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window.

https://reviewboard.mozilla.org/r/237172/#review243008

So that is how it works... Basically it's a copy of https://dxr.mozilla.org/mozilla-central/rev/5ded36cb383d3ccafd9b6c231c5120dcdae196a2/testing/mochitest/browser-test.js#29.

We want to use it each time a registered `TabClose` or `unload` event listener is called. Both cases mean that a tab or a window is going to be closed, and the handler has the time to do some clean-up work. In our case we register for two observer notifications, which will tell us when the message manager gets disconnected and the framescript has been blown away. Only when that happened we can be sure that the tab is really gone. 

As it looks like `message-manager-close` can be used for some clean-up work but is not always guaranteed to be fired, eg. a crash. So not sure if we really need that. But `message-manager-disconnected` is really needed here.

Maybe we could also register for that notification globally and dispatch a custom Marionette event with the window handle. That way consumers could use it to identify if the close activity belongs to them.

The question is what we need now, and what could be delayed to a follow-up bug.

> It feels like this class more naturally belongs in
> testing/marionette/wm.js.  Because I’m changing code in this general
> area as part of the window tracking patch, would you mind if we
> moved it there – despite it being the only thing in there for the
> moment?

Sure. That sounds fine. I can move it. I actually was not sure where to put it in. But lets wait what else you come up with regarding the API and my explanation from above.

> Please correct me if I’m wrong, but the observers will fire for
> _any browser_ that is closed.  This means the tab we are waiting
> on to be closed must be passed to TabDestroyObserver so it can
> compare the subject with the expected browser.
> 
> Pseudo-code:
> 
> > observe(subject, topic) {
> >   if (subject !== this.browser) {
> >     return;
> >   }
> > 
> >   switch (topic) {
> >     …
> >   }
> > }

The subject here is not the tab/window but the message manager which is going to close. So we would have to map it to the correct browser, or window.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 19 • 6 years ago

Ok, the problem on Android is a simple exception:

https://dxr.mozilla.org/mozilla-central/rev/789e30ff2e3d6e1fcfce1a373c1e5635488d24da/testing/marionette/proxy.js#209-211

The tab we are trying to remove the event listener from doesn't exist anymore. And as such `node` is null.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 21 • 6 years ago
mozreview-review-reply

Comment on attachment 8968461 [details]
Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window.

https://reviewboard.mozilla.org/r/237172/#review243008

> Sure. That sounds fine. I can move it. I actually was not sure where to put it in. But lets wait what else you come up with regarding the API and my explanation from above.

I completely re-worked the API and created a Promise that can be used via sync.js.

> Maybe this API would look better?
> 
> > await TabDestroyObserver.wait(tab);
> > this.removeHanlders();
> > resolve();

I was not able to get the `await` working even not with the new implementation. It complains that a method needs to be async to use await, but that is also happening when I did it. Hm?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 23 • 6 years ago

The latest update works fine for closing tabs and browser windows, but I still have to find a method to identify other chrome window types. Hopefully by tomorrow the final patch should be ready.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 24 • 6 years ago

Turned out it is pretty easy because for `closeWindow()` we cannot use the browser message manager but have to make use of the window message manager.

	Andreas Tolfsen ❲:ato❳
	Comment 28 • 6 years ago
mozreview-review

Comment on attachment 8968461 [details]
Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window.

https://reviewboard.mozilla.org/r/237172/#review251046

::: testing/marionette/browser.js:174
(Diff revision 8)
> -    return this.contentBrowser.messageManager;
> +    let rv = null;
> +
> +    if (this.contentBrowser) {
> +      rv = this.contentBrowser.messageManager;
> +    }
> +
> +    return rv;

Why not just this?

> if (this.contentBrowser.messageManager) {
>   return this.contentBrowser.messageManager;
> }
> return null;

::: testing/marionette/browser.js:289
(Diff revision 8)
>     * @return {Promise}
>     *     A promise which is resolved when the current window has been closed.
>     */
>    closeWindow() {
>      return new Promise(resolve => {
> -      this.window.addEventListener("unload", resolve, {once: true});
> +      let destroyed = MessageManagerDestroyedPromise(

Mind adding "new" here?  I know it isn’t exactly a class, but a
class is really just a fancy prototype.

::: testing/marionette/browser.js:290
(Diff revision 8)
>     *     A promise which is resolved when the current window has been closed.
>     */
>    closeWindow() {
>      return new Promise(resolve => {
> -      this.window.addEventListener("unload", resolve, {once: true});
> +      let destroyed = MessageManagerDestroyedPromise(
> +          this.window.messageManager);

this.messageManager right?

::: testing/marionette/browser.js:293
(Diff revision 8)
>      return new Promise(resolve => {
> -      this.window.addEventListener("unload", resolve, {once: true});
> +      let destroyed = MessageManagerDestroyedPromise(
> +          this.window.messageManager);
> +
> +      this.window.addEventListener("unload", () => {
> +        destroyed.then(resolve);

With the caveat that you need to make the arrow function async,
this is easier to read:

> await destroyed;
> resolve();

::: testing/marionette/browser.js:319
(Diff revision 8)
>          !this.tab) {
>        return this.closeWindow();
>      }
>  
>      return new Promise((resolve, reject) => {
> +      let destroyed = MessageManagerDestroyedPromise(this.messageManager);

new

::: testing/marionette/browser.js:323
(Diff revision 8)
> -        this.tabBrowser.deck.addEventListener("TabClose", resolve, {once: true});
> +        this.tabBrowser.deck.addEventListener(
> +            "TabClose", () => destroyed.then(resolve), {once: true});
>          this.tabBrowser.closeTab(this.tab);
>  
>        } else if (this.tabBrowser.removeTab) {
>          // Firefox
> -        this.tab.addEventListener("TabClose", resolve, {once: true});
> +        this.tab.addEventListener(
> +            "TabClose", () => destroyed.then(resolve), {once: true});

I sort of feel we can improve readability here.  Maybe this will
work?

> let browserDetached = async () => {
>   await destroyed;
>   resolve();
> };
>
> if (this.tabBrowser.closeTab) {
>   // Fennec
>   this.tabBrowser.deck.addEventListener("TabClose", browserDetached, {once: true});
>   …

::: testing/marionette/proxy.js:160
(Diff revision 8)
> +          case "TabClose":
> +            messageManager = this.browser.messageManager;
>              break;
>          }
> +
> +        MessageManagerDestroyedPromise(messageManager).then(() => {

new

::: testing/marionette/proxy.js:160
(Diff revision 8)
> +          case "TabClose":
> +            messageManager = this.browser.messageManager;
>              break;
>          }
> +
> +        MessageManagerDestroyedPromise(messageManager).then(() => {

Can we avoid the chaining with an await?

::: testing/marionette/sync.js:16
(Diff revision 8)
> -/* exported PollPromise, TimedPromise */
> +  /* exported PollPromise, TimedPromise */
> -this.EXPORTED_SYMBOLS = ["PollPromise", "TimedPromise"];
> +  "PollPromise",
> +  "TimedPromise",
> +
> +  /* exported MessageManagerDestroyedPromise */

I think actually these exported comments are no longer necessary.
You can verify with "./mach doc testing/marionette" and inspecting
the sync internals page.

::: testing/marionette/sync.js:17
(Diff revision 8)
> -this.EXPORTED_SYMBOLS = ["PollPromise", "TimedPromise"];
> +  "PollPromise",
> +  "TimedPromise",
> +
> +  /* exported MessageManagerDestroyedPromise */
> +  "MessageManagerDestroyedPromise",

Mind sorting this?

::: testing/marionette/sync.js:182
(Diff revision 8)
> + * If a top-level browsing context or a window is about to close
> + * appropriate `TabClose` and `unload` events are fired. But there
> + * are no existent events which indicate that the browsing context
> + * or window are really closed yet. Therefore the disconnect state
> + * of the appropriate browser or window message manager can be observed,
> + * and used to determine the final closed state.

This doesn’t just apply to top-level browsing contexts and we ought
to be more concrete.  I don’t think it is useful to talk about
TabClose or unload here since <xul:browser>s can exist independently
of tabs.

I propose this following:

> One can observe the removal and detachment of a content browser
> (`<xul:browser>`) by its message manager disconnecting.  When the
> browser is associated with a tab, this is safer than relying on
> events such as `TabClose` and `unload`, which signalise the _intent
> to_ remove a tab and consequently would lead to the destruction of
> the content browser.

::: testing/marionette/sync.js:189
(Diff revision 8)
> + * are no existent events which indicate that the browsing context
> + * or window are really closed yet. Therefore the disconnect state
> + * of the appropriate browser or window message manager can be observed,
> + * and used to determine the final closed state.
> + *
> + * @param {MessageManager} messageManager

ChromeMessageSender

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 29 • 6 years ago
mozreview-review-reply

Comment on attachment 8968461 [details]
Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window.

https://reviewboard.mozilla.org/r/237172/#review251046

> this.messageManager right?

No, to close the window we cannot observe the browser message manager, but the window message manager is needed.

> With the caveat that you need to make the arrow function async,
> this is easier to read:
> 
> > await destroyed;
> > resolve();

Doh! The arrow function needs to be async! I totally overlooked that part, which explains why your suggestion didn't work before. I will definetly update that.

> I sort of feel we can improve readability here.  Maybe this will
> work?
> 
> > let browserDetached = async () => {
> >   await destroyed;
> >   resolve();
> > };
> >
> > if (this.tabBrowser.closeTab) {
> >   // Fennec
> >   this.tabBrowser.deck.addEventListener("TabClose", browserDetached, {once: true});
> >   …

Sounds plausible.

> I think actually these exported comments are no longer necessary.
> You can verify with "./mach doc testing/marionette" and inspecting
> the sync internals page.

Those are necessary because otherwise the linter will fail. See my other comment on this bug.

> Mind sorting this?

I wanted to separate the generic and special promises. `MessageManagerDestroyedPromise` could actually use the `TimedPromise` in the future to not hang forever in case the observer notification doesn't fire.

> This doesn’t just apply to top-level browsing contexts and we ought
> to be more concrete.  I don’t think it is useful to talk about
> TabClose or unload here since <xul:browser>s can exist independently
> of tabs.
> 
> I propose this following:
> 
> > One can observe the removal and detachment of a content browser
> > (`<xul:browser>`) by its message manager disconnecting.  When the
> > browser is associated with a tab, this is safer than relying on
> > events such as `TabClose` and `unload`, which signalise the _intent
> > to_ remove a tab and consequently would lead to the destruction of
> > the content browser.

I updated the content by your proposal but also made modifications because you ignored the chrome window case. Both are different. Please let me know if that is fine.

> ChromeMessageSender

This is not only a `ChromeMessageSender`, but a `ChromeMessageBroadcaster` for the window message manager. Both are sub-classes of `MessageListenerManager`, which we would need here.

	Andreas Tolfsen ❲:ato❳
	Comment 31 • 6 years ago
mozreview-review-reply

Comment on attachment 8968461 [details]
Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window.

https://reviewboard.mozilla.org/r/237172/#review251046

> I wanted to separate the generic and special promises. `MessageManagerDestroyedPromise` could actually use the `TimedPromise` in the future to not hang forever in case the observer notification doesn't fire.

I don’t really see the point of that, but OK.

	Andreas Tolfsen ❲:ato❳
	Comment 32 • 6 years ago
mozreview-review

Comment on attachment 8968461 [details]
Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window.

https://reviewboard.mozilla.org/r/237172/#review251242

	Pulsebot
	Comment 33 • 6 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3558cd78821d
[marionette] Fix race condition for closing a browser and chrome window. r=ato

	Narcis Beleuzu [:NarcisB]
	Comment 34 • 6 years ago

Backed out for wr failures on processing-model/basic.html

Backout: https://hg.mozilla.org/integration/autoland/rev/b31bf225f2cd99f4220695bf6a59b77f6611cb3c
Push: https://hg.mozilla.org/integration/autoland/rev/3558cd78821d557168827d3281d966ab4a0ca905
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=179353865&repo=autoland&lineNumber=12521

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 35 • 6 years ago

Currently I have no idea what's going wrong here. I had a look at the diff of the two images and as you can see in this attachment there are some rounded corners at the bottom of the window which do not appear in on of those windows.

Just before the test fails I can see the following log line:

> 0:27.68 pid:25580 1527065545249	Marionette	INFO	Found 14 pixels different, maximum difference per channel 92

And I have no idea in how to interpret it. Why does it say maximum difference of 92 while we only have 14?

Note I can reproduce locally but only when also running all the tests in that folder which are run before basic.html. Just running basic.html passes all the time.

Note that for all the tests above we do not close the window, so I have no idea why my patch triggered a problem here.

James, do you have an idea?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 36 • 6 years ago

For reference here the failing wr jobs while my patch was on autoland:

https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=bce37e838f6a&filter-searchStr=wr&tochange=b31bf225f2cd

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 37 • 6 years ago

Further checks have shown that the problem is related to my changes in proxy.js when we are trying to detect the message manager disconnect. Here I made use of `this.browser.window` which in case of the reftest window might be null, because it's not a browser.

It is hard to debug because I don't know how to enable Marionette logging. Even additional `dump()` statements do not appear on the command line. Maybe all of those are getting filtered out?

	James Graham [:jgraham]
	Comment 38 • 6 years ago

I think this is an OSX-only bug in the reftest harness. I need to investigate more closely, but it seems like sometimes we end up in a state where the harness thinks there are differences between the images but actually there aren't. A convenient workaround is to add restart-after: true to the expectation metadata for the previous failing test.

	James Graham [:jgraham]
	Comment 41 • 6 years ago
mozreview-review

Comment on attachment 8980274 [details]
Bug 1452653 - [wpt-reftests] Restart after audio_has_no_subtitles.html for false positive.

https://reviewboard.mozilla.org/r/246430/#review252528

::: testing/web-platform/meta/webvtt/rendering/cues-with-video/processing-model/audio_has_no_subtitles.html.ini:3
(Diff revision 1)
>  [audio_has_no_subtitles.html]
>    expected: TIMEOUT
> +  restart-after: true  # Bug 1463844

This comment won't survive a metadata update, better to use
```
  bug: 1463844
```

or similar

	Pulsebot
	Comment 44 • 6 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b78efc35faa
[wpt-reftests] Restart after audio_has_no_subtitles.html for false positive. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/6a28d74472b1
[marionette] Fix race condition for closing a browser and chrome window. r=ato

	Cristian Brindusan [:cbrindusan]
	Comment 45 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0b78efc35faa
https://hg.mozilla.org/mozilla-central/rev/6a28d74472b1

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 46 • 6 years ago

Test-only patch which fixes a race condition in Marionette. Please uplift to beta.

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/55860ae6ceae
https://hg.mozilla.org/releases/mozilla-beta/rev/e5056e8f4692

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 48 • 6 years ago

I would like to see this race also fixed on esr60. Can you please uplift? Thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 49 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/982e107e8c1e
https://hg.mozilla.org/releases/mozilla-esr60/rev/8744ef3afa00