Closed Bug 1093594 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
36.3
Tracking Status
e10s + ---

People

(Reporter: Gijs, Assigned: mossop)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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?
Flags: qe-verify-
Flags: needinfo?(dtownsend+bugmail)
Flags: in-testsuite+
Flags: firefox-backlog+
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.
(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)
(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...
Flags: needinfo?(dao)
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.
Clearly I failed and it will be next week at the earliest before I can look at this properly.
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
Assignee: nobody → dtownsend+bugmail
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Flags: needinfo?(dao)
Flags: needinfo?(dtownsend+bugmail)
Attached patch patchSplinter Review
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.
Attachment #8524715 - Flags: review?(gijskruitbosch+bugs)
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...
Attachment #8524715 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/240df159e238
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Depends on: 1101624
You need to log in before you can comment on or make changes to this bug.