805068 - Make browser chrome tests more self-contained by giving each test a new blank tab

Status: VERIFIED FIXED

(Testing :: Mochitest, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Bug 567950 is an example of an orange caused by a test replacing the original tab present at test start with a new tab in a cleanup function. Because it didn't stop the load in the new tab, the load event was interfering with the next test.

I think we should:
- avoid, wherever possible, having tests mess with the original tab in this way
- when not possible (e.g. for some sessionstore or tabbrowser tests), make sure the tests are waiting for the new tab to be fully loaded before continuing to the next test

To get there, we need to evaluate the list of tests that are doing this tab replacement. I worked up a patch that introduces an error in those cases, to generate such a list:
https://hg.mozilla.org/try/rev/e897764a7cdb
https://tbpl.mozilla.org/?tree=Try&rev=e897764a7cdb

Comment 1

[Nathan Froyd [:froydnj]]

What would be super-nifty would be checking all the tests that run after the failing tests picked out by your patch and then seeing how many of those have intermittent oranges...

Comment 2

[Ed Morley [:emorley]]

Failing tests from the try run in comment 0:

browser/base/content/test/browser_allTabsPanel.js
browser/base/content/test/browser_bug406216.js
browser/base/content/test/browser_bug455852.js
browser/base/content/test/browser_bug519216.js
browser/base/content/test/browser_bug563588.js
browser/base/content/test/browser_bug575561.js
browser/base/content/test/browser_bug585830.js
browser/base/content/test/browser_bug623893.js
browser/base/content/test/browser_bug763468.js
browser/base/content/test/browser_bug767836.js
browser/base/content/test/browser_overflowScroll.js
browser/base/content/test/browser_popupUI.js
browser/base/content/test/browser_save_private_link.js
browser/base/content/test/browser_save_video.js
browser/base/content/test/browser_tabMatchesInAwesomebar.js
browser/base/content/test/newtab/browser_newtab_private_browsing.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_beforeunload.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_certexceptionsui.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_commandline_toggle.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_cookieacceptdialog.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_crh.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_fastswitch.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_forgetthissite.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_geoprompt.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_localStorage.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_newwindow_stopcmd.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openLocationLastURL.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_opendir.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_pageinfo.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_popupblocker.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_popupmode.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_protocolhandler.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_transition.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_urlbarfocus.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_viewsource.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_zoomrestore.js
browser/components/search/test/browser_426329.js
browser/components/search/test/browser_contextmenu.js
browser/components/sessionstore/test/browser_394759_privatebrowsing.js
browser/components/sessionstore/test/browser_454908.js
browser/components/sessionstore/test/browser_522545.js
browser/components/sessionstore/test/browser_586068-apptabs.js
browser/components/sessionstore/test/browser_586068-apptabs_ondemand.js
browser/components/sessionstore/test/browser_586068-reload.js
browser/components/sessionstore/test/browser_586068-select.js
browser/components/sessionstore/test/browser_600545.js
browser/components/sessionstore/test/browser_601955.js
browser/components/tabview/test/browser_tabview_bug595601.js
browser/components/tabview/test/browser_tabview_bug600812.js
browser/components/tabview/test/browser_tabview_bug608153.js
browser/components/tabview/test/browser_tabview_bug608158.js
browser/components/tabview/test/browser_tabview_bug608405.js
browser/components/tabview/test/browser_tabview_bug613541.js
browser/components/tabview/test/browser_tabview_bug624265.js
browser/components/tabview/test/browser_tabview_bug624727.js
browser/components/tabview/test/browser_tabview_bug624847.js
browser/components/tabview/test/browser_tabview_bug626455.js
browser/components/tabview/test/browser_tabview_bug633788.js
browser/components/tabview/test/browser_tabview_bug650280.js
browser/components/tabview/test/browser_tabview_bug679853.js
browser/components/tabview/test/browser_tabview_bug685692.js
browser/components/tabview/test/browser_tabview_privatebrowsing.js
browser/devtools/webconsole/test/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js
browser/devtools/webconsole/test/browser_webconsole_bug_618311_private_browsing.js
dom/indexedDB/test/browser_privateBrowsing.js
dom/tests/browser/browser_focus_steal_from_chrome.js
dom/tests/browser/browser_focus_steal_from_chrome_during_mousedown.js
toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js
toolkit/content/tests/browser/browser_default_image_filename.js
toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js
toolkit/content/tests/browser/browser_save_resend_postdata.js

I've only looked through some of these, but there are definitely instances of orange in the tests following some of them:

browser/base/content/test/browser_allTabsPanel.js
-> Next test: browser_bug380960.js - orange bug 585361

browser/components/privatebrowsing/test/browser/browser_privatebrowsing_transition.js
-> Next test: browser_privatebrowsing_ui.js - orange bug 759707

browser/components/privatebrowsing/test/browser/browser_privatebrowsing_urlbarfocus.js
-> Next test: browser_privatebrowsing_urlbarundo.js - orange bug 802011

browser/components/search/test/browser_426329.js
-> Next test: browser_483086.js - orange bug 762958

browser/components/search/test/browser_contextmenu.js
-> Next test: browser_248970_a.js - orange bug 625960 & orange bug 554611

browser/components/sessionstore/test/browser_586068-apptabs_ondemand.js
-> Next test: browser_586068-browser_state_interrupted.js - orange bug 802953

browser/components/tabview/test/browser_tabview_bug600812.js
-> Next test: browser_tabview_bug602432.js - orange bug 704417

browser/components/tabview/test/browser_tabview_bug608153.js
-> Next test: browser_tabview_bug608158.js - orange bug 667216

Comment 3

[Nathan Froyd [:froydnj]]

Since this patch only ever existed on try, try resets have consigned it to the Bitbucket of Deleted Things.  Gavin, do you have the patch lying around still?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Here's the patch.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Pushed that to try: https://tbpl.mozilla.org/?tree=Try&rev=767e780df824

Comment 6

[Nathan Froyd [:froydnj]]

Tests from bc1 in Gavin's push that are orange (I know bc2 and bc3 are clean based on fixes that I've pushed over the past couple of days.  Some bc1 tests below have also been fixed, but I'm not going to sort those out atm.):



TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug406216.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug455852.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug519216.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug563588.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug577121.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug585830.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug623893.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_overflowScroll.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_popupUI.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_save_video.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_967000_button_charEncoding.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_967000_button_feeds.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_967000_button_sync.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_968447_bookmarks_toolbar_items_in_panel.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/search/test/browser_426329.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_522545.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_586068-apptabs.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_586068-apptabs_ondemand.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_586068-reload.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_586068-select.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_600545.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_601955.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug595601.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug600812.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug608153.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug608158.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug608405.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug613541.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug624847.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug626455.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug633788.js | Test destroyed original tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/tabview/test/browser_tabview_bug685692.js | Test destroyed original tab

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #0)
> Bug 567950 is an example of an orange caused by a test replacing the
> original tab present at test start with a new tab in a cleanup function.
> Because it didn't stop the load in the new tab, the load event was
> interfering with the next test.

Can the test framework call stop() between tests or something like that? To me this seems preferable over adding esoteric constraints to the way people write tests.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to :Gavin Sharp (email [e-mail]) from comment #0)
> > Bug 567950 is an example of an orange caused by a test replacing the
> > original tab present at test start with a new tab in a cleanup function.
> > Because it didn't stop the load in the new tab, the load event was
> > interfering with the next test.
> 
> Can the test framework call stop() between tests or something like that? To
> me this seems preferable over adding esoteric constraints to the way people
> write tests.

What's so esoteric about trying to ensure that tests are more-or-less self-contained?  The bustage from attempting to land bug 999618 last week suggests that having tests that pretend they haven't changed the browser state really does cause problems for the rest of the test suite.

I could understand wanting to do stop() or similar if we had mass quantities of problematic tests.  But we have about thirty tests left with problems, fewer after Tim lands his sessionstore/tabview cleanup patches.  This small number of problematic tests suggests that having the constraint of preserving the original tab is not all that burdensome.

Just for fun, I've pushed to try this patch + the patch from bug 999618 + a fix to browser-test.js to call stop() after every test.  If tests come up busted, we'll know that stop() isn't enough:

https://tbpl.mozilla.org/?tree=Try&rev=cd3f581ebff3

Comment 9

[Dão Gottwald [:dao]]

(In reply to Nathan Froyd (:froydnj) from comment #8)
> (In reply to Dão Gottwald [:dao] from comment #7)
> > Can the test framework call stop() between tests or something like that? To
> > me this seems preferable over adding esoteric constraints to the way people
> > write tests.
> 
> What's so esoteric about trying to ensure that tests are more-or-less
> self-contained?

Treating the first tab special in some way and making tests self-contained are two different things. Unlike the original browser window, the first tab is really just another tab as far as the test framework is concerned. We also don't prevent tests from loading things in the first tab, and indeed if they do so and don't stop the load before finishing the test you get the same problem.

> I could understand wanting to do stop() or similar if we had mass quantities
> of problematic tests.  But we have about thirty tests left with problems,

That's not a small number and there would an ongoing cost of enforcing this policy in the future.

> Just for fun, I've pushed to try this patch + the patch from bug 999618 + a
> fix to browser-test.js to call stop() after every test.  If tests come up
> busted, we'll know that stop() isn't enough:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=cd3f581ebff3

Great. If stop() isn't enough, we should figure out why.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #9)
> Treating the first tab special in some way and making tests self-contained
> are two different things. Unlike the original browser window, the first tab
> is really just another tab as far as the test framework is concerned. We
> also don't prevent tests from loading things in the first tab, and indeed if
> they do so and don't stop the load before finishing the test you get the
> same problem.

After pondering over this a bit more, I think it would actually make sense for the framework to add a blank tab, call stop() and remove the other tab between tests. Should be pretty cheap with tab animations disabled, although we should measure it to make sure we don't make test suite runs take significantly longer.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to Dão Gottwald [:dao] from comment #9)
> > Treating the first tab special in some way and making tests self-contained
> > are two different things. Unlike the original browser window, the first tab
> > is really just another tab as far as the test framework is concerned. We
> > also don't prevent tests from loading things in the first tab, and indeed if
> > they do so and don't stop the load before finishing the test you get the
> > same problem.
> 
> After pondering over this a bit more, I think it would actually make sense
> for the framework to add a blank tab, call stop() and remove the other tab
> between tests. Should be pretty cheap with tab animations disabled, although
> we should measure it to make sure we don't make test suite runs take
> significantly longer.

Well, certainly the previous try run proved that calling stop was not enough.  Here's is a try push with what I understood your new proposal to be:

https://tbpl.mozilla.org/?tree=Try&rev=aa759a529369

The across-the-board oranges suggest that I probably screwed something up.  Regardless, I think time is better spent fixing tests and putting in more "write tests correctly" checks.  The current checks of:

- don't leave tabs lying around;
- don't leave windows lying around;
- don't leak DOM windows;
- relinquish control of the refresh driver; and
- don't add properties to the global.

don't impose any significant burden on people and I don't see that the one under discussion here would cause any more problems than those.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Nathan Froyd (:froydnj) from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > After pondering over this a bit more, I think it would actually make sense
> > for the framework to add a blank tab, call stop() and remove the other tab
> > between tests. Should be pretty cheap with tab animations disabled, although
> > we should measure it to make sure we don't make test suite runs take
> > significantly longer.
> 
> Well, certainly the previous try run proved that calling stop was not
> enough.  Here's is a try push with what I understood your new proposal to be:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=aa759a529369
> 
> The across-the-board oranges suggest that I probably screwed something up.

Probably, yes.

> Regardless, I think time is better spent fixing tests and putting in more
> "write tests correctly" checks.

I disagree; I don't think the tests are broken and therefore we shouldn't "fix" them. We should fix the framework.

> The current checks of:
> 
> - don't leave tabs lying around;
> - don't leave windows lying around;
> - don't leak DOM windows;
> - relinquish control of the refresh driver; and
> - don't add properties to the global.
> 
> don't impose any significant burden on people and I don't see that the one
> under discussion here would cause any more problems than those.

Those things are all obviously incorrect (the opposite of what I called "esoteric" above). I'm challenging the idea that tests removing superfluous tabs in no particular order are in the same bucket.

Comment 13

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to Nathan Froyd (:froydnj) from comment #11)
> > Here's is a try push with what I understood your new proposal to be:
> > 
> > https://tbpl.mozilla.org/?tree=Try&rev=aa759a529369
> > 
> > The across-the-board oranges suggest that I probably screwed something up.
> 
> Probably, yes.

First of all, you're not calling stop() for the added tab but for the one you're about to remove.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > (In reply to Nathan Froyd (:froydnj) from comment #11)
> > > Here's is a try push with what I understood your new proposal to be:
> > > 
> > > https://tbpl.mozilla.org/?tree=Try&rev=aa759a529369
> > > 
> > > The across-the-board oranges suggest that I probably screwed something up.
> > 
> > Probably, yes.
> 
> First of all, you're not calling stop() for the added tab but for the one
> you're about to remove.

Since I clearly don't know what I'm doing here, would you write up the appropriate patch?

Comment 15

[Dão Gottwald [:dao]]

I'll give it a try.

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch: automatically give each test a fresh tab

Seems to work locally, although I didn't run the entire test suite. I'll push this to the try server.

Comment 17

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #16)
> Created attachment 8414528 [details] [diff] [review]
> patch: automatically give each test a fresh tab
> 
> Seems to work locally, although I didn't run the entire test suite. I'll
> push this to the try server.

https://tbpl.mozilla.org/?tree=Try&rev=1d0d7231f223

Comment 18

[Dão Gottwald [:dao]]

Attachment: patch: automatically give each test a fresh tab, v2

https://tbpl.mozilla.org/?tree=Try&rev=568f6e2b59eb

* There was a bug in tabview/test/head.js that I fixed (afterAllTabsLoaded would infinitely wait for a load event when a blank tab was added with an immediate stop()). I blindly copied this fix over to browser/devtools/webconsole/test/head.js. The above try run is my first one with devtools tests enabled.

* There's something wrong with browser_social_chatwindow_resize.js, but it's not obvious to me what that is. If this was the last issue, I'd just get this patch landed with that test disabled and continue investigating its issue in a separate bug.

* There appears to be a problem in browser_visibleTabs_bookmarkAllPages.js which I think could only be caused by misbehaving code (e.g. a stale event handler) from some other test silently running in the background. Failures from an earlier try run:

TEST-UNEXPECTED-FAIL | Only one tab is visible - Got 2, expected 1
TEST-UNEXPECTED-FAIL | Only one uri is returned - Got 2, expected 1
TEST-UNEXPECTED-FAIL | It's the correct URI - Got about:blank, expected http://mochi.test:8888/

I couldn't reproduce this locally when running all tests in browser/base/content/test/general/.

Comment 19

[Dão Gottwald [:dao]]

Attachment: patch: automatically give each test a fresh tab, v3

https://tbpl.mozilla.org/?tree=Try&rev=15ec41848ad4

browser/devtools/webconsole/test/browser_bug_871156_ctrlw_close_tab.js revealed a problem with SimpleTest.waitForFocus. It timed out, waiting for a stopped about:blank load to finish. I've changed SimpleTest.waitForFocus to ignore document.readyState for about:blank. Not sure if this will cause other problems.

There's a leak in mochitest-devtools-chrome-1 that I can't make sense of (TEST-UNEXPECTED-FAIL | leakcheck | 3233179 bytes leaked (AsyncLatencyLogger, AsyncStatement, AtomImpl, BackstagePass, BodyRule, ...)).

> * There appears to be a problem in browser_visibleTabs_bookmarkAllPages.js
> which I think could only be caused by misbehaving code (e.g. a stale event
> handler) from some other test silently running in the background. Failures
> from an earlier try run:
> 
> TEST-UNEXPECTED-FAIL | Only one tab is visible - Got 2, expected 1
> TEST-UNEXPECTED-FAIL | Only one uri is returned - Got 2, expected 1
> TEST-UNEXPECTED-FAIL | It's the correct URI - Got about:blank, expected
> http://mochi.test:8888/
> 
> I couldn't reproduce this locally when running all tests in
> browser/base/content/test/general/.

Apparently this is intermittent.

Comment 20

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #19)
> Created attachment 8415103 [details] [diff] [review]
> patch: automatically give each test a fresh tab, v3
> 
> https://tbpl.mozilla.org/?tree=Try&rev=15ec41848ad4
> 
> There's a leak in mochitest-devtools-chrome-1 that I can't make sense of
> (TEST-UNEXPECTED-FAIL | leakcheck | 3233179 bytes leaked
> (AsyncLatencyLogger, AsyncStatement, AtomImpl, BackstagePass, BodyRule,
> ...)).

Rob/Dave, any idea what's going on there?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

(In reply to Dão Gottwald [:dao] from comment #19)
> There's a leak in mochitest-devtools-chrome-1 that I can't make sense of
> (TEST-UNEXPECTED-FAIL | leakcheck | 3233179 bytes leaked
> (AsyncLatencyLogger, AsyncStatement, AtomImpl, BackstagePass, BodyRule,
> ...)).

This may be of interest to mccr8 as well because that signature looks similar to the leaks we saw related to enabling async CC/GC the first time around.

Comment 22

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #19)
> Created attachment 8415103 [details] [diff] [review]
> patch: automatically give each test a fresh tab, v3
> 
> https://tbpl.mozilla.org/?tree=Try&rev=15ec41848ad4
> 
> browser/devtools/webconsole/test/browser_bug_871156_ctrlw_close_tab.js
> revealed a problem with SimpleTest.waitForFocus. It timed out, waiting for a
> stopped about:blank load to finish. I've changed SimpleTest.waitForFocus to
> ignore document.readyState for about:blank. Not sure if this will cause
> other problems.

Here's an alternative version replacing the document.readyState check with webProgress.isLoadingDocument:
https://tbpl.mozilla.org/?tree=Try&rev=d719d2f5a4f9

Comment 23

[Andrew McCreight [:mccr8]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21)
> This may be of interest to mccr8 as well because that signature looks
> similar to the leaks we saw related to enabling async CC/GC the first time
> around.

No, this is different.  That other leak is in M4, and just involves 1 document.

This one involves 4 nsGlobalWindows and 24 nsDocuments.  If you have nothing else to go on, the first step to investigating this is to reproduce it locally, then you can get a shutdown cycle collector log by running with MOZ_CC_LOG_SHUTDOWN=1 MOZ_CC_LOG_THREAD=main, then use the find_roots script from here to figure out what is holding the nsGlobalWindows alive: https://github.com/amccreight/heapgraph

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 8415103 [details] [diff] [review]
patch: automatically give each test a fresh tab, v3

Obviously I need to figure out what to do with browser_social_chatwindow_resize.js, browser_visibleTabs_bookmarkAllPages.js and the mochitest-devtools-chrome-1 leak, but these are all unlikely to be due to a mistake in my changes here, so I think this is good for a review pass.

Comment 25

[Dão Gottwald [:dao]]

Attachment: patch: automatically give each test a fresh tab, v4

browser_social_chatwindow_resize.js turned out to be a previously-intermittent failure, bug 966579.

I pinned down the devtools leak to browser_dbg_clean-exit-window.js, also known in bug 933950 as a test that would -- without this patch -- only fail when run on its own.

Filed bug 1005420 for browser_visibleTabs_bookmarkAllPages.js.

Comment 26

[Nathan Froyd [:froydnj]]

I think closing the dependent bugs was premature.  Have you, e.g., tested your patch to see if it fixes the intermittent in bug 993712, or tested with the patch from bug 999618 to see if it fixes the errors that patch introduced in completely different tests?  It doesn't seem worthwhile to land your patch if there's no evidence that it's actually fixing anything.

Comment 27

[Dão Gottwald [:dao]]

(In reply to Nathan Froyd (:froydnj) from comment #26)
> I think closing the dependent bugs was premature.  Have you, e.g., tested
> your patch to see if it fixes the intermittent in bug 993712,

No, I haven't, because the idea that bug 993712 would be related was only a wild guess in the first place (bug 993712 comment 175). We'll see whether it does or doens't go away when this lands.

> or tested with
> the patch from bug 999618 to see if it fixes the errors that patch
> introduced in completely different tests?

I haven't played with this patch further since I don't see why we'd want to land it. See below.

> It doesn't seem worthwhile to
> land your patch if there's no evidence that it's actually fixing anything.

It fixes the issue this bug was originally filed for. Tests removing the original tab won't affect subsequent tests anymore.

Comment 28

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #10)
> After pondering over this a bit more, I think it would actually make sense
> for the framework to add a blank tab, call stop() and remove the other tab
> between tests. Should be pretty cheap with tab animations disabled, although
> we should measure it to make sure we don't make test suite runs take
> significantly longer.

I looked at some numbers with and without this patch and there's generally quite a bit of fluctuation. It would often be slower or faster by some minutes, but there doesn't seem to be a trend.

Comment 29

[Nathan Froyd [:froydnj]]

(In reply to Dão Gottwald [:dao] from comment #27)
> (In reply to Nathan Froyd (:froydnj) from comment #26)
> > It doesn't seem worthwhile to
> > land your patch if there's no evidence that it's actually fixing anything.
> 
> It fixes the issue this bug was originally filed for. Tests removing the
> original tab won't affect subsequent tests anymore.

I don't know that this is true.  Consider bug 999618:

- browser_save_video.js replaces the original tab (along with several other tests in that bug);
- Making those tests not replace the original tab caused other tests to fail;
- Testing on try confirmed that browser_save_video.js was the problematic test.

That is, changing browser_save_video.js to *not* replace the original tab caused other tests to fail.  Ergo, the replacement of the tab *is* affecting other tests.  Making the test harness cover that up doesn't improve anything.

Comment 30

[Dão Gottwald [:dao]]

(In reply to Nathan Froyd (:froydnj) from comment #29)
> - browser_save_video.js replaces the original tab (along with several other
> tests in that bug);
> - Making those tests not replace the original tab caused other tests to fail;
> - Testing on try confirmed that browser_save_video.js was the problematic
> test.
> 
> That is, changing browser_save_video.js to *not* replace the original tab
> caused other tests to fail.  Ergo, the replacement of the tab *is* affecting
> other tests.

If you look at my patch, you will see that I actually make browser_save_video.js not replace the original tab (not because this was a problem with my patch but because it's pointless now).

> Making the test harness cover that up doesn't improve anything.

I'm don't understand what you mean by "cover up" here.

Comment 31

[Nathan Froyd [:froydnj]]

(In reply to Dão Gottwald [:dao] from comment #30)
> (In reply to Nathan Froyd (:froydnj) from comment #29)
> > - browser_save_video.js replaces the original tab (along with several other
> > tests in that bug);
> > - Making those tests not replace the original tab caused other tests to fail;
> > - Testing on try confirmed that browser_save_video.js was the problematic
> > test.
> > 
> > That is, changing browser_save_video.js to *not* replace the original tab
> > caused other tests to fail.  Ergo, the replacement of the tab *is* affecting
> > other tests.
> 
> If you look at my patch, you will see that I actually make
> browser_save_video.js not replace the original tab (not because this was a
> problem with my patch but because it's pointless now).

Indeed.  And moving that bit of code into the test harness ensures that we won't discover any similar interdependencies between tests in the future.  Which is a bad thing.

Comment 32

[Dão Gottwald [:dao]]

No, it doesn't cover up the interdepency but prevents it. It makes tests more self-contained. This is the whole point.

Comment 33

[Dão Gottwald [:dao]]

Attachment: patch v4

unbitrotted

Comment 34

[Mark Hammond [:markh] [:mhammond]]

I just landed a fix for bug 966579 and tested it with the patch here - can you please remove the line that disables browser_social_chatwindow_resize.js?

Comment 35

[Dão Gottwald [:dao]]

Attachment: patch v5

(In reply to Mark Hammond [:markh] from comment #34)
> I just landed a fix for bug 966579 and tested it with the patch here - can
> you please remove the line that disables browser_social_chatwindow_resize.js?

done!

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8417966 [details] [diff] [review]
patch v5

Review of attachment 8417966 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/tabview/test/head.js
@@ +141,5 @@
>      let isRestorable = !(tab.hidden && !restoreHiddenTabs &&
>                           browser.__SS_restoreState &&
>                           browser.__SS_restoreState == TAB_STATE_NEEDS_RESTORE);
>  
> +    if (isRestorable && browser.webProgress.isLoadingDocument) {

I think I fixed what you've been hitting here yesterday with bug 1002843.

Comment 37

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #36)
> ::: browser/components/tabview/test/head.js
> @@ +141,5 @@
> >      let isRestorable = !(tab.hidden && !restoreHiddenTabs &&
> >                           browser.__SS_restoreState &&
> >                           browser.__SS_restoreState == TAB_STATE_NEEDS_RESTORE);
> >  
> > +    if (isRestorable && browser.webProgress.isLoadingDocument) {
> 
> I think I fixed what you've been hitting here yesterday with bug 1002843.

I think there were two problems. One was the messed up boolean expression that you fixed, the other is the contentDocument.readyState == "complete" check. If you add a tab and call stop() immediately, its readyState is "uninitialized" and there will be no "load" event, hence afterAllTabsLoaded will time out. As far as I can tell, webProgress.isLoadingDocument is exactly what we need here and there's no point in additionally checking readyState.

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #37)
> I think there were two problems. One was the messed up boolean expression
> that you fixed, the other is the contentDocument.readyState == "complete"
> check. If you add a tab and call stop() immediately, its readyState is
> "uninitialized" and there will be no "load" event, hence afterAllTabsLoaded
> will time out. As far as I can tell, webProgress.isLoadingDocument is
> exactly what we need here and there's no point in additionally checking
> readyState.

Ah, right. Looks good to me then.

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 8417966 [details] [diff] [review]
patch v5

https://hg.mozilla.org/integration/fx-team/rev/0c890d80e9a6

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Assuming this sticks, I'm going to want this on Aurora31 as well for the ESR. It has a small conflict in browser/components/tabview/test/head.js from bug 1002843 that'll need rebasing around, though (Tim says we probably don't want to uplift that patch to 31 just for this).

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c890d80e9a6

Comment 42

[Marco Mucci [:MarcoM]]

Hi Juan, can you review this bug if it will require further QA verification.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Dao, do you mind taking care of the small rebase in comment 40? :)

Comment 44

[Dão Gottwald [:dao]]

Attachment: aurora patch

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/37037e7a19e8