1093594 - e10s - Fix browser/base/content/test/general/browser_alltabslistener.js to work with e10s

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[:Gijs (out for now; he/him)]

It seems to basically get completely confused over the order of the events it's expecting. I have no idea why this test expects what it expects and/or why it doesn't work in e10s. Dave, you added this test, can you elaborate?

Comment 1

[Dão Gottwald [:dao]]

What order of events does the test get with e10s enabled? Is it deterministic? If it is, the next step would be to see if the order makes sense. Getting onLocationChange without a prior onStateChange or onSecurityChange without a prior onLocationChange wouldn't make much sense, for instance.

Comment 2

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #1)
> What order of events does the test get with e10s enabled? Is it
> deterministic?

Hard to tell offhand, because the failure-spam is pretty bad.

> If it is, the next step would be to see if the order makes
> sense. Getting onLocationChange without a prior onStateChange or
> onSecurityChange without a prior onLocationChange wouldn't make much sense,
> for instance.

Are you volunteering to take this? It's easy to check the answer to these questions by just #-'ing out the skip-if in browser.ini and running the test with --e10s. :-)

(I have another 100-odd tests to check for e10s-compat, so I'm definitely not (right now), sorry)

Comment 3

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #2)
> Are you volunteering to take this?

Not at this time, maybe in a week or two if it's still unowned then...

Comment 4

[Dave Townsend [:mossop]]

I think the test was just verifying that the same events and the same order came through allTabsListener as the regular progress listener. Looking at the test the ordering listed there looks pretty sane:

onStateChange to signal the start of the load
onLocationChange to signal we're getting a particular document
onSecurityChange(s) to signal the results of any tls handshake
onStateChange to signal the end of the load

Getting events in a different order to that would be concerning I think so this might be a signal of a real problem. I'll try to look at this later in the week.

Comment 5

[Dave Townsend [:mossop]]

Clearly I failed and it will be next week at the earliest before I can look at this properly.

Comment 6

[Dave Townsend [:mossop]]

Ok this is just a poorly written test. The initial set up doesn't wait for the new tabs to finish loading completely before proceeding so a bunch of progress messages from those loads mess things up. Spinning a patch through try now.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0ee1863802b3

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch

waitForDocLoadComplete uses progress listener notifications to wait for a load to complete, this can be after the load event fires. This makes us wait for both browsers to finish before starting the tests. I've tentatively re-enabled the test everywhere as bug 951680 looks like it might have been caused by a similar problem (racing between executeSoon and the progress notifications). I did 20 try runs on linux first with no failures.

Comment 8

[:Gijs (out for now; he/him)]

Comment on attachment 8524715 [details] [diff] [review]
patch

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

Nice!

Can you file a followup bug to make http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/head.js#458 do cleanup (registerCleanupFunction, I guess...) if it doesn't get fired? I foresee issues (which we might have already) if that listener gets to hang around on a tab we reuse...

Comment 9

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/240df159e238

And filed bug 1101060

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/240df159e238