Closed Bug 1222128 Opened 9 years ago Closed 8 years ago

Fix test_bug1011748.html so it works with e10s

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox45 --- affected
firefox48 --- fixed

People

(Reporter: mccr8, Assigned: gsvelto)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

This test seems a little tricky compared to the other tests that observe http- things, because the observer (which is in the parent) checks the status of a content object (the XHR), and the status will change over time. It feels like that at some level the parent must synchronously query the child.
Blocks: e10s-tests
tracking-e10s: --- → +
Attached patch [PATCH WIP] Tentative fix (obsolete) — Splinter Review
I don't know if this is the right approach but converting it in a browser mochitest seems to do the trick. Not taking the bug yet because I did this mostly by trial and error; I really don't know if this is the right approach here.
Attachment #8738530 - Flags: feedback?(continuation)
Isn't this creating the XHR in the parent process? I'm not very familiar with browser-chrome tests.
Attachment #8738530 - Flags: feedback?(continuation)
(In reply to Andrew McCreight [:mccr8] from comment #2)
> Isn't this creating the XHR in the parent process? I'm not very familiar
> with browser-chrome tests.

I think it does, creating the XMLHttpRequest within a ContextUtils.spawn() call doesn't work and the other browser mochitests using it are doing it the same way which is why I tried this approach.
Comment on attachment 8738530 [details] [diff] [review]
[PATCH WIP] Tentative fix

Boris, you reviewed the original test, do you think this is an appropriate conversion or not? This works fine in both e10s and non-e10s modes and I *think* it's testing the same scenario as the original test but I'm not sure not being very familiar with it.
Attachment #8738530 - Flags: feedback?(bzbarsky)
Comment on attachment 8738530 [details] [diff] [review]
[PATCH WIP] Tentative fix

Why do you need to add/remove the tab, if you're just doing the XHR completely chrome-side?

Anyway, you can test whether this test is testing the right thing by commenting out this block in nsXMLHttpRequest::GetStatusText:

1119   if (readyState == UNSENT || readyState == OPENED) {
1120     return;
1121   }

and seeing whether your modified test fails.
Attachment #8738530 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky [:bz] from comment #5)
> Why do you need to add/remove the tab, if you're just doing the XHR
> completely chrome-side?

It's a copy-paste leftover.

> Anyway, you can test whether this test is testing the right thing by
> commenting out this block in nsXMLHttpRequest::GetStatusText:
> 
> 1119   if (readyState == UNSENT || readyState == OPENED) {
> 1120     return;
> 1121   }
> 
> and seeing whether your modified test fails.

Just tried that and it causes the test to fail. I'll put up a revised patch and ask for review then.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Comment on attachment 8739965 [details] [diff] [review]
[PATCH] Turn test_bug1011748.html into a browser mochitest to make it run properly in e10s mode

r=me
Attachment #8739965 - Flags: review?(bzbarsky) → review+
Thanks for the review, the tests are green so let's land it.
https://hg.mozilla.org/mozilla-central/rev/4b38540998a0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: