Closed Bug 1101624 Opened 9 years ago Closed 5 years ago

Intermittent browser_alltabslistener.js | Test timed out - expected PASS

Categories

(Firefox :: General, defect, P3)

Product:
Firefox
Component:
General
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: cbook, Unassigned)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

use dummy page instead of about:blank to avoid a race,
1.70 KB, patch
mconley
: review+
Gijs
: checkin+
Details | Diff | Splinter Review
patch
1.58 KB, patch
Gijs
: review+
Details | Diff | Splinter Review 
Ubuntu VM 12.04 b2g-inbound pgo test mochitest-browser-chrome-1

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=865215&repo=b2g-inbound

07:39:55 INFO - 59 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_alltabslistener.js | Test timed out - expected PASS
Dave, looks like something broke. :-(
Blocks: 1093594
Flags: needinfo?(dtownsend+bugmail)
My best guess here is that if we initiated a real network load (dummy_page.html would do) after starting the waitForDocLoadComplete promises it might avoid a race condition. I may not have time to test or try that for at least a week if not two though.
comment 25 seems worth trying. Mike, look OK to you?
Attachment #8529057 - Flags: review?(mconley)
Comment on attachment 8529057 [details] [diff] [review]
use dummy page instead of about:blank to avoid a race,

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

Let's try it.

::: browser/base/content/test/general/browser_alltabslistener.js
@@ +85,5 @@
>  
>  function test() {
>    waitForExplicitFinish();
>  
> +  gBackgroundTab = gBrowser.addTab("http://example.org/browser/browser/base/content/test/general/dummy_page.html");

If this sticks, it'd probably make more sense to have the URL be a const that these refer to.

So maybe do that before landing.
Attachment #8529057 - Flags: review?(mconley) → review+
w/ the URL pulled into a global constant:

remote:   https://hg.mozilla.org/integration/fx-team/rev/6e82407a7c65
... I should just go to sleep.

I meant:

https://hg.mozilla.org/integration/fx-team/rev/09034b8710cb
(In reply to TBPL Robot from comment #77)
> submit_timestamp: 2014-11-26T21:10:22
> log:
> https://treeherder.mozilla.org/ui/logviewer.html#?repo=fx-team&job_id=1307952
> repository: fx-team
> who: philringnalda[at]gmail[dot]com
> machine: tst-linux64-spot-797
> buildname: Ubuntu VM 12.04 x64 fx-team debug test mochitest-browser-chrome-1
> revision: 679466398b30
> 
> 54 INFO TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_alltabslistener.js | Test timed out - expected PASS
> 56 INFO TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_alltabslistener.js | Found a tab after previous test timed out:
> http://example.org/browser/browser/base/content/test/general/dummy_page.html
> - expected PASS
> 57 INFO TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_alltabslistener.js | Found a tab after previous test timed out:
> http://example.org/browser/browser/base/content/test/general/dummy_page.html
> - expected PASS

Computer says no. I almost wonder if there's an actual bug with the state listener not being fired? :-\
Keywords: leave-open
Attachment #8529057 - Flags: checkin+
Attached patch patch — — Splinter Review 
So the last change wasn't quite what I described, this is what I meant. I ran this test a few hundred times by itself on try and saw no failures. But then I also ran it by itself without the change a few hundred times and also saw no failure so seems likely something happening in an earlier test is making this one unstable somehow.

Either way probably doesn't hurt to try this change and see what happens.
Flags: needinfo?(dtownsend)
Attachment #8536798 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8536798 [details] [diff] [review]
patch

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

::: browser/base/content/test/general/browser_alltabslistener.js
@@ +99,5 @@
>      waitForDocLoadComplete(gBackgroundBrowser),
>      waitForDocLoadComplete(gForegroundBrowser)
>    ];
> +  gBackgroundBrowser.loadURI(kBasePage);
> +  gForegroundBrowser.loadURI(kBasePage);

While we're here, any reason not to switch to promiseTabLoadEvent(tab, kBasePage), which seems to be more frequently used in browser/base/content/test/general, and automagically ignores about:blank, and will save us a few lines of code here? :-)
Attachment #8536798 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #154)
> Comment on attachment 8536798 [details] [diff] [review]
> patch
> 
> Review of attachment 8536798 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/general/browser_alltabslistener.js
> @@ +99,5 @@
> >      waitForDocLoadComplete(gBackgroundBrowser),
> >      waitForDocLoadComplete(gForegroundBrowser)
> >    ];
> > +  gBackgroundBrowser.loadURI(kBasePage);
> > +  gForegroundBrowser.loadURI(kBasePage);
> 
> While we're here, any reason not to switch to promiseTabLoadEvent(tab,
> kBasePage), which seems to be more frequently used in
> browser/base/content/test/general, and automagically ignores about:blank,
> and will save us a few lines of code here? :-)

Remember how we switched away from the tab load event in bug 1093594 because progress events were still happening afterwards?
(In reply to Dave Townsend [:mossop] (Out till December 1st) from comment #155)
> (In reply to :Gijs Kruitbosch from comment #154)
> > Comment on attachment 8536798 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8536798 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/base/content/test/general/browser_alltabslistener.js
> > @@ +99,5 @@
> > >      waitForDocLoadComplete(gBackgroundBrowser),
> > >      waitForDocLoadComplete(gForegroundBrowser)
> > >    ];
> > > +  gBackgroundBrowser.loadURI(kBasePage);
> > > +  gForegroundBrowser.loadURI(kBasePage);
> > 
> > While we're here, any reason not to switch to promiseTabLoadEvent(tab,
> > kBasePage), which seems to be more frequently used in
> > browser/base/content/test/general, and automagically ignores about:blank,
> > and will save us a few lines of code here? :-)
> 
> Remember how we switched away from the tab load event in bug 1093594 because
> progress events were still happening afterwards?

Well, the previous incarnation only checked 1 of the two tabs, and didn't ignore about:blank loads. Is fixing those bits not enough here?
(In reply to :Gijs Kruitbosch from comment #157)
> (In reply to Dave Townsend [:mossop] (Out till December 1st) from comment
> #155)
> > (In reply to :Gijs Kruitbosch from comment #154)
> > > Comment on attachment 8536798 [details] [diff] [review]
> > > patch
> > > 
> > > Review of attachment 8536798 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: browser/base/content/test/general/browser_alltabslistener.js
> > > @@ +99,5 @@
> > > >      waitForDocLoadComplete(gBackgroundBrowser),
> > > >      waitForDocLoadComplete(gForegroundBrowser)
> > > >    ];
> > > > +  gBackgroundBrowser.loadURI(kBasePage);
> > > > +  gForegroundBrowser.loadURI(kBasePage);
> > > 
> > > While we're here, any reason not to switch to promiseTabLoadEvent(tab,
> > > kBasePage), which seems to be more frequently used in
> > > browser/base/content/test/general, and automagically ignores about:blank,
> > > and will save us a few lines of code here? :-)
> > 
> > Remember how we switched away from the tab load event in bug 1093594 because
> > progress events were still happening afterwards?
> 
> Well, the previous incarnation only checked 1 of the two tabs, and didn't
> ignore about:blank loads. Is fixing those bits not enough here?

Maybe, not sure. But I have to divert my attention to something else right now so I'm going to land this as-is and come back to it if it is still failing:

https://hg.mozilla.org/integration/fx-team/rev/802cc1027e76
Clearly that didn't work
(In reply to Dave Townsend [:mossop] from comment #161)
> Clearly that didn't work

No, but the odd thing is that it seems that from:

https://treeherder.mozilla.org/ui/logviewer.html#?repo=mozilla-inbound&job_id=4657968

the pages are loading fine, and are loading well before the timeout happens. So it seems like the promises aren't being fulfilled despite the pages loading:

18:01:37 INFO - TEST-INFO | screentopng: exit 0
18:01:37 INFO - 49 INFO checking window state
18:01:37 INFO - 50 INFO Waiting for browser load
18:01:37 INFO - 51 INFO Waiting for browser load
18:01:37 INFO - 52 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.org/browser/browser/base/content/test/general/dummy_page.html" line: 0}]
18:01:37 INFO - 53 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.org/browser/browser/base/content/test/general/dummy_page.html" line: 0}]
18:01:37 INFO - 54 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_alltabslistener.js | Test timed out - expected PASS 


on the other hand, the timeout screenshot doesn't show either of the tabs, so it's not clear to me when that screenshot was taken relative to when the test timed out and/or the harness cleaned up the extra tabs.
(In reply to :Gijs Kruitbosch from comment #176)
> (In reply to Dave Townsend [:mossop] from comment #161)
> > Clearly that didn't work
> 
> No, but the odd thing is that it seems that from:
> 
> https://treeherder.mozilla.org/ui/logviewer.html#?repo=mozilla-
> inbound&job_id=4657968
> 
> the pages are loading fine, and are loading well before the timeout happens.
> So it seems like the promises aren't being fulfilled despite the pages
> loading:

What's more, the progress listener logs every onStateChange it sees. And it apparently sees none so it appears that the progress listener is completely broken, and this is in non-e10s!
At least some of the (orange) logs have them, I also suspect I picked a run from before your last patch, but I did think there should have been logs even then...