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

VERIFIED FIXED in Firefox 31

Status

Testing
BrowserTest
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: Gavin, Assigned: dao)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

unspecified
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox30 wontfix, firefox31 fixed, firefox32 fixed, firefox-esr24 wontfix)

Details

(Whiteboard: p=0 s=it-32c-31a-30b.1 [qa-])

Attachments

(2 attachments, 6 obsolete attachments)

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
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...
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
Blocks: 950073

Updated

4 years ago
Whiteboard: p=0
Blocks: 996504
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?
Flags: needinfo?(gavin.sharp)
Created attachment 8409032 [details] [diff] [review]
patch

Here's the patch.
Flags: needinfo?(gavin.sharp)
Pushed that to try: https://tbpl.mozilla.org/?tree=Try&rev=767e780df824

Updated

4 years ago
No longer blocks: 950073
Flags: firefox-backlog+
Depends on: 999604
Depends on: 999618
Depends on: 1000198
Depends on: 1000894
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
Depends on: 1001521
(Assignee)

Comment 7

4 years ago
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Comment 10

4 years ago
(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.
See Also: → bug 993712
Depends on: 1002517
(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.
(Assignee)

Comment 12

4 years ago
(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.
(Assignee)

Comment 13

4 years ago
(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.
(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?
Flags: needinfo?(dao)
(Assignee)

Comment 15

4 years ago
I'll give it a try.
(Assignee)

Comment 16

4 years ago
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.
(Assignee)

Comment 17

4 years ago
(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
(Assignee)

Comment 18

4 years ago
Created attachment 8414939 [details] [diff] [review]
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/.
Attachment #8414528 - Attachment is obsolete: true
(Assignee)

Comment 19

4 years ago
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.

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.
Attachment #8414939 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
(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?
Flags: needinfo?(rcampbell)
Flags: needinfo?(dcamp)
Flags: needinfo?(dao)
(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.
(Assignee)

Comment 22

4 years ago
(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
(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
(Assignee)

Comment 24

4 years ago
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.
Attachment #8415103 - Flags: review?(ttaubert)
Attachment #8415103 - Flags: review?(gavin.sharp)
(Assignee)

Updated

4 years ago
Depends on: 966579
(Assignee)

Updated

4 years ago
Depends on: 933950
(Assignee)

Updated

4 years ago
Depends on: 1005420
(Assignee)

Comment 25

4 years ago
Created attachment 8416871 [details] [diff] [review]
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.
Assignee: nobody → dao
Attachment #8409032 - Attachment is obsolete: true
Attachment #8415103 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8415103 - Flags: review?(ttaubert)
Attachment #8415103 - Flags: review?(gavin.sharp)
Attachment #8416871 - Flags: review?(ttaubert)
Attachment #8416871 - Flags: review?(gavin.sharp)
Flags: needinfo?(rcampbell)
Flags: needinfo?(dcamp)
(Assignee)

Updated

4 years ago
Summary: add warning when tests replace the original tab, eliminate tests that do it incorrectly → Make browser chrome tests more self-contained by giving each test a new blank tab
(Assignee)

Updated

4 years ago
Blocks: 993712
See Also: bug 993712
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.
Flags: needinfo?(dao)
(Assignee)

Comment 27

4 years ago
(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.
Flags: needinfo?(dao)
(Assignee)

Comment 28

4 years ago
(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.
(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.
(Assignee)

Comment 30

4 years ago
(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.
(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.
(Assignee)

Comment 32

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

Comment 33

4 years ago
Created attachment 8417673 [details] [diff] [review]
patch v4

unbitrotted
Attachment #8416871 - Attachment is obsolete: true
Attachment #8416871 - Flags: review?(ttaubert)
Attachment #8416871 - Flags: review?(gavin.sharp)
Attachment #8417673 - Flags: review?(ttaubert)
Attachment #8417673 - Flags: review?(gavin.sharp)
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?
Flags: needinfo?(dao)
(Assignee)

Comment 35

4 years ago
Created attachment 8417966 [details] [diff] [review]
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!
Attachment #8417673 - Attachment is obsolete: true
Attachment #8417673 - Flags: review?(ttaubert)
Attachment #8417673 - Flags: review?(gavin.sharp)
Attachment #8417966 - Flags: review?(ttaubert)
Attachment #8417966 - Flags: review?(gavin.sharp)
Flags: needinfo?(dao)
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.
Attachment #8417966 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 37

4 years ago
(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.
(Assignee)

Updated

4 years ago
Whiteboard: p=0
(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.
(Assignee)

Comment 39

4 years ago
Comment on attachment 8417966 [details] [diff] [review]
patch v5

https://hg.mozilla.org/integration/fx-team/rev/0c890d80e9a6
Attachment #8417966 - Flags: review?(gavin.sharp)
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).
https://hg.mozilla.org/mozilla-central/rev/0c890d80e9a6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(Assignee)

Updated

4 years ago
Blocks: 933950
No longer depends on: 933950
(Assignee)

Updated

4 years ago
Blocks: 1005420
No longer depends on: 1005420
Hi Juan, can you review this bug if it will require further QA verification.
Flags: needinfo?(jbecerra)
Whiteboard: p=0 s=it-32c-31a-30b.2 [qa?]

Updated

4 years ago
Whiteboard: p=0 s=it-32c-31a-30b.2 [qa?] → p=0 s=it-32c-31a-30b.1 [qa?]
Dao, do you mind taking care of the small rebase in comment 40? :)
status-firefox30: --- → wontfix
status-firefox31: --- → affected
status-firefox32: --- → fixed
status-firefox-esr24: --- → wontfix
Flags: needinfo?(dao)

Updated

4 years ago
Flags: needinfo?(jbecerra)
Whiteboard: p=0 s=it-32c-31a-30b.1 [qa?] → p=0 s=it-32c-31a-30b.1 [qa-]

Updated

4 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Comment 44

4 years ago
Created attachment 8421507 [details] [diff] [review]
aurora patch
Flags: needinfo?(dao)
(Assignee)

Updated

4 years ago
No longer depends on: 999604
https://hg.mozilla.org/releases/mozilla-aurora/rev/37037e7a19e8
status-firefox31: affected → fixed
Depends on: 1007418
(Assignee)

Updated

4 years ago
No longer depends on: 999618
(Assignee)

Updated

4 years ago
No longer depends on: 1000198
(Assignee)

Updated

4 years ago
No longer depends on: 1000894
(Assignee)

Updated

4 years ago
No longer depends on: 1001521
(Assignee)

Updated

4 years ago
No longer depends on: 1002517
(Assignee)

Updated

4 years ago
No longer blocks: 993712
You need to log in before you can comment on or make changes to this bug.