1267278 - RDM's browser_menu_item_02 test leaks browser.xul on e10s

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[Matteo Ferretti [:zer0] [:matteo]]

On Windows Debug this test is leaking a window:

TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_menu_item_02.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/browser.xul]

see: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9682626bf2ab&selectedJob=19903988

It's currently disabled, but we should investigate why it's happening and fix it.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

This is a general e10s leak from the looks of it. I'm seeing it on Ash for Linux as well. I'll update the skip-if annotation accordingly.

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8d8a23760c8b

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8d8a23760c8b

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Any updates here? I'm looking to turn on OSX 10.10 debug e10s in production Really Soon Now and this blocks that.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

As I understand it, :zer0 is actively working on it, but it only reproduces on try for him, so it's a slow process to try fixes.  If it's blocking work, we could disable it for now and re-enable once we're more confident in it.

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: MozReview Request: Bug 1267278 - remove the usage of some add-on SDK modules, and refactoring the unit tests; r=jryans

Review commit: https://reviewboard.mozilla.org/r/54266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54266/

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please use a commit message that summarizes what the patch is doing instead of restating the problem. Also, I noticed that you left the global skip-if for OSX debug e10s at the top of browser.ini. Was that intentional?

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8754833 [details]
MozReview Request: Bug 1267278 - remove the usage of some add-on SDK modules, and refactoring the unit tests; r=jryans

https://reviewboard.mozilla.org/r/54266/#review50986

Overall, I think the idea looks good, assuming this does fix the failures.  A few issues left to resolve first though.

::: devtools/client/responsive.html/manager.js:5
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> + /* globals Services */

Let's explicitly bring in Services:

const Services = require("Services");

::: devtools/client/responsive.html/manager.js:175
(Diff revision 1)
> +        ResponsiveUIManager.setMenuCheckFor(target);
> +        break;
> +      case "browser-delayed-startup-finished":
> +        ResponsiveUIManager.setMenuCheckFor(getActiveTab(target));
> +        break;
> +      case "TabClose":

As part of the page state changes I landed recently, `ResponsiveUI` now watches for `beforeunload` on the tab.  This seems to cover both tab close and window close caess, so I don't think we need listeners for `TabClose` or `close` from window anymore.

If there's a case I am forgetting, please let me know!

::: devtools/client/responsive.html/manager.js:186
(Diff revision 1)
> +        }
> +        break;
> -  }
> +    }
> +  },
> +
> +  initMenuCheckListenerFor(window) {

This function seems to need to run for each new browser window.  Right now, it's called once when `activeTabs` is empty.  But there is only one `ResponsiveUIManager` and it's shared between browser windows.  If RDM is open in window 1, and then you open window 2 and try to open RDM, we wouldn't call this function, since `activeTabs` is not empty due to window 1.

::: devtools/client/responsive.html/manager.js:193
(Diff revision 1)
> +    tabContainer.addEventListener("TabSelect", this.handleMenuCheck);
> +    tabContainer.addEventListener("TabClose", this.handleMenuCheck);
> +
> +    window.addEventListener("close", this.handleMenuCheck);
> +
> +    Services.obs.addObserver(this.handleMenuCheck,

What's the case that needs us to watch for this observer notification?  `setMenuCheckFor` is already yielding on `startup`, so it should be waiting long enough for the menu to exist...  What am I missing?

::: devtools/client/responsive.html/test/browser/browser.ini:6
(Diff revision 1)
>  [DEFAULT]
>  tags = devtools
>  subsuite = devtools
>  # !e10s: RDM only works for remote tabs
>  # OSX debug e10s: Bug 1268319 - Leaking the world
>  skip-if = !e10s || (os == 'mac' && e10s && debug)

I have the same question as RyanVM: can we also remove the `(os == 'mac' && e10s && debug)` skip-if after these changes?

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

https://reviewboard.mozilla.org/r/54266/#review50986

> Let's explicitly bring in Services:
> 
> const Services = require("Services");

I prefer that too, but `Services`, so I modified the code in that way. But `Services` is already in the scope, so we're redeclaring that. Is that OK? Should we remove `Services` to be automatically as module globals and make it explicit evey time? (Not in this patch)

> As part of the page state changes I landed recently, `ResponsiveUI` now watches for `beforeunload` on the tab.  This seems to cover both tab close and window close caess, so I don't think we need listeners for `TabClose` or `close` from window anymore.
> 
> If there's a case I am forgetting, please let me know!

You're right, both events are not needed now.

> This function seems to need to run for each new browser window.  Right now, it's called once when `activeTabs` is empty.  But there is only one `ResponsiveUIManager` and it's shared between browser windows.  If RDM is open in window 1, and then you open window 2 and try to open RDM, we wouldn't call this function, since `activeTabs` is not empty due to window 1.

Yes, it's partially true. At the beginning that wasn't necessary but now this code needs to be *partially* called for each new browser window. The observer, for instance, needs to be called only once. I modified the code in such way.

> What's the case that needs us to watch for this observer notification?  `setMenuCheckFor` is already yielding on `startup`, so it should be waiting long enough for the menu to exist...  What am I missing?

We're observing this notification to listen when a new window is opened; I was doing so due to an edge case I noticed that left the RDM's menu check checked when it shouldn't, but I'm not able to reproduce it with the current code anymore; so I'm inclined to remove it. Notice that this code will be probably needed anyway when we detach a tab with RDM opened, but since this would involve additional code anyway I think we should just re-add it once we're dealing with that.

> I have the same question as RyanVM: can we also remove the `(os == 'mac' && e10s && debug)` skip-if after these changes?

I have removed that, probably it messed up during my rebasing.

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8754833 [details]
MozReview Request: Bug 1267278 - remove the usage of some add-on SDK modules, and refactoring the unit tests; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54266/diff/1-2/

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

I refactor everything to use `BrowserTestUtils`, and ensure we're properly close RDM *and* the tab before closing the `window` in the unit test. I've also stripped out from the patch anything unnecessary now – thanks to the changes happened in the code base in the meantime – and it seems promising: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01e4ff40e192 (the ES warning was a leftover that I've fixed already).

I pushed another more extensive try build with the other platforms in the meantime we're reviewing the code.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> Please use a commit message that summarizes what the patch is doing instead
> of restating the problem.

In jetpack we used to just add the bug number / description, and I did the same until now in devtools. If that is wrong, I'm happy to change to a different format! (I was also using multiline commit message in the past, to describe shortly the changes made, after the first line, but apparently it wasn't a good thing).

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #11)
> In jetpack we used to just add the bug number / description, and I did the
> same until now in devtools. If that is wrong, I'm happy to change to a
> different format! (I was also using multiline commit message in the past, to
> describe shortly the changes made, after the first line, but apparently it
> wasn't a good thing).
I don't think there's any problem with the format. This is what's used for all commits at Mozilla:
Bug <number> - <description>; r=<reviewer>
Optionally followed by other lines going into more details.

What RyanVM is saying is that the <description> part needs to say what you changed, not say what the problem is. It's way more useful to what has been changed in the first line of the commit when looking at mercurial logs.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #11)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> > Please use a commit message that summarizes what the patch is doing instead
> > of restating the problem.
> 
> In jetpack we used to just add the bug number / description, and I did the
> same until now in devtools. If that is wrong, I'm happy to change to a
> different format! (I was also using multiline commit message in the past, to
> describe shortly the changes made, after the first line, but apparently it
> wasn't a good thing).

Multiline commit messages are great to have, we should encourage them more!  I use them sometimes for complex things.

Like :pbro says, I think the main point is we want the summary line to describe the actual change made, instead of just restating the bug title.  Feel free to add more lines to the message after the summary line if you'd like to!

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8754833 [details]
MozReview Request: Bug 1267278 - remove the usage of some add-on SDK modules, and refactoring the unit tests; r=jryans

https://reviewboard.mozilla.org/r/54266/#review52620

We're getting closer, but there are a few event management issues left to work out.

::: devtools/client/responsive.html/manager.js:167
(Diff revision 2)
> +    ResponsiveUIManager.setMenuCheckFor(target);
> +  },
> +
> +  initMenuCheckListenerFor(window) {
> +    let { tabContainer } = window.gBrowser;
> +    tabContainer.addEventListener("TabSelect", this.handleMenuCheck);

So, it seems like we only need _one_ `TabSelect` event listener per window with this design, right?  The target of the event will be the new tab, and `ResponsiveUIManager` is a singleton so only one event is needed.

With the current code, if you open one browser window and start RDM in 2 tabs, we will add 2 `TabSelect` listeners here, but we only needed 1.

What about this:  `ResponsiveUIManager` could keep a `WeakSet` of browser windows with at least 1 RDM tab, so that we only add a listener once.  Then there's only one listener per browser window.

(By the way, it seems like `activeTabs` should probably be a `WeakMap`, not `Map`, but perhaps in a different bug...)

::: devtools/client/responsive.html/manager.js:170
(Diff revision 2)
> +  initMenuCheckListenerFor(window) {
> +    let { tabContainer } = window.gBrowser;
> +    tabContainer.addEventListener("TabSelect", this.handleMenuCheck);
> +  },
> +
> +  removeMenuCheckListenerFor(window) {

This is now mismatched with `initMenuCheckListenerFor`, since `initMenuCheckListenerFor` is called for every tab in a window, but this is only called for the last one to close, so there will be extra listeners remaining.

::: devtools/client/responsive.html/test/browser/browser_menu_item_01.js:17
(Diff revision 2)
>  
>  const activateTab = (tab) => new Promise(resolve => {
> -  events.once(activate, "data", resolve);
> +  let { tabContainer } = tabUtils.getOwnerWindow(tab).gBrowser;
> +
> +  tabContainer.addEventListener("TabSelect", function listener({type}) {
> +    this.removeEventListener(type, listener);

I think `this` would be the _tab_ not the `tabContainer`?  Maybe just use `tabContainer` to avoid issues.

::: devtools/client/responsive.html/test/browser/browser_menu_item_02.js:8
(Diff revision 2)
>  
>  "use strict";
>  
>  // Test RDM menu item is checked when expected, on multiple windows.
>  
> -const TEST_URL = "data:text/html;charset=utf-8,";
> +const TEST_URL = "data:text/html;charset=utf-8,hello";

Haha, does the test change without the "hello"?  Just curious. :)

::: devtools/client/responsive.html/test/browser/browser_menu_item_02.js:19
(Diff revision 2)
>    return menu.getAttribute("checked") === "true";
>  };
>  
>  add_task(function* () {
> -  const window1 = yield openBrowserWindow();
> -
> +  const window1 = yield BrowserTestUtils.openNewBrowserWindow();
> +  let { gBrowser} = window1;

Nit: Space before }

Comment 15

[Matteo Ferretti [:zer0] [:matteo]]

https://reviewboard.mozilla.org/r/54266/#review52620

> So, it seems like we only need _one_ `TabSelect` event listener per window with this design, right?  The target of the event will be the new tab, and `ResponsiveUIManager` is a singleton so only one event is needed.
> 
> With the current code, if you open one browser window and start RDM in 2 tabs, we will add 2 `TabSelect` listeners here, but we only needed 1.
> 
> What about this:  `ResponsiveUIManager` could keep a `WeakSet` of browser windows with at least 1 RDM tab, so that we only add a listener once.  Then there's only one listener per browser window.
> 
> (By the way, it seems like `activeTabs` should probably be a `WeakMap`, not `Map`, but perhaps in a different bug...)

Yes, we need only 1, but that's the good thing: if you open one browser window and start RDM in 2 tabs, since `ResponsiveUIManager` is a singleton, the listener reference will be the same, so only one will be added. We don't need to trace anything.

(Agreed about `activeTabs`. I thought was added in that way for a reason)

> This is now mismatched with `initMenuCheckListenerFor`, since `initMenuCheckListenerFor` is called for every tab in a window, but this is only called for the last one to close, so there will be extra listeners remaining.

As said, the listener it would be added only once per `window`, so it shouldn't mismatching.

> I think `this` would be the _tab_ not the `tabContainer`?  Maybe just use `tabContainer` to avoid issues.

`this` points to `tabContainer` for sure (see: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#The_value_of_this_within_the_handler).

> Haha, does the test change without the "hello"?  Just curious. :)

It's actually a leftover to verify manually that the URL was loaded and the window I was operating was the correct one. :) I can remove it, or adding a meaningful value instead. :)

> Nit: Space before }

Argh, thanks!
We should have a eslint rule, for that. :)

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/54266/#review52620

> Yes, we need only 1, but that's the good thing: if you open one browser window and start RDM in 2 tabs, since `ResponsiveUIManager` is a singleton, the listener reference will be the same, so only one will be added. We don't need to trace anything.
> 
> (Agreed about `activeTabs`. I thought was added in that way for a reason)

Huh, you are right, adding the same listener more than once just discards the additional listeners.  I guess I have just never tried that case before in the past.

> As said, the listener it would be added only once per `window`, so it shouldn't mismatching.

I was mistaken before about a listener being added for every tab, instead one listener is added per window.  However, there is still a different issue:

1. Open window 1 with one tab, and start RDM there
2. Open window 2 with one tab, and start RDM there
3. Exit RDM in each tab

We only remove a listener when there are no more active tabs in RDM _in any window_ (since it's a singleton).  This means the listener is only removed from the browser window of the last tab to close RDM.

So, we'll still need to tweak something here so that the listener is removed correctly.

> `this` points to `tabContainer` for sure (see: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#The_value_of_this_within_the_handler).

You are right, it is in fact the `tabContainer`.  Since this is not a generic listener applied to many different elements though, I think it would be easier to read without using `this` and instead matching `tabContainer.addEventListener` by using `tabContainerr.removeEventListener` so it's very obvious the add / remove is correctly balanced.

> It's actually a leftover to verify manually that the URL was loaded and the window I was operating was the correct one. :) I can remove it, or adding a meaningful value instead. :)

Whatever you like. :)

Comment 17

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8754833 [details]
MozReview Request: Bug 1267278 - remove the usage of some add-on SDK modules, and refactoring the unit tests; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54266/diff/2-3/

Comment 18

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)

> 1. Open window 1 with one tab, and start RDM there
> 2. Open window 2 with one tab, and start RDM there
> 3. Exit RDM in each tab
> 
> We only remove a listener when there are no more active tabs in RDM _in any
> window_ (since it's a singleton).  This means the listener is only removed
> from the browser window of the last tab to close RDM.
> So, we'll still need to tweak something here so that the listener is removed
> correctly.

I fixed that adding a new method, `isActiveForWindow`, see the patch. I think we probably won't have a lot of tabs with RDM active there, so that approach should be OK – I rather prefer avoid to store additional object references when possible.


> You are right, it is in fact the `tabContainer`.  Since this is not a
> generic listener applied to many different elements though, I think it would
> be easier to read without using `this` and instead matching
> `tabContainer.addEventListener` by using `tabContainerr.removeEventListener`

I use that generic form because it's easier to me to read actually – and the function is independent by the variables above – but if you have strong opinion about that, I'll change before land (it's still like this in the new patch).

I also updated the commit message.

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8754833 [details]
MozReview Request: Bug 1267278 - remove the usage of some add-on SDK modules, and refactoring the unit tests; r=jryans

https://reviewboard.mozilla.org/r/54266/#review53582

Great, I think this is just about ready to go.  Thanks for working on it!

Have you done a try run lately?  Hopefully it looks good.

::: devtools/client/responsive.html/test/browser/browser_menu_item_01.js:17
(Diff revision 3)
>  
>  const activateTab = (tab) => new Promise(resolve => {
> -  events.once(activate, "data", resolve);
> +  let { tabContainer } = tabUtils.getOwnerWindow(tab).gBrowser;
> +
> +  tabContainer.addEventListener("TabSelect", function listener({type}) {
> +    this.removeEventListener(type, listener);

Please do change `this` to `tabContainer`, since it's not a generic usage here.  For me it's most important to be able to easily scan that they are balanced, which is very simple when you see `tabContainer` on both calls.

Comment 20

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8754833 [details]
MozReview Request: Bug 1267278 - remove the usage of some add-on SDK modules, and refactoring the unit tests; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54266/diff/3-4/

Comment 21

[Matteo Ferretti [:zer0] [:matteo]]

Here the latest try build results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59c414bb99f4

I don't think there is anything related to this bug, so I'm going to land the patch.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c2f28446c94a
remove the usage of some add-on SDK modules, and refactoring the unit tests; r=jryans

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2f28446c94a

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2f28446c94a