1222128 - Fix test_bug1011748.html so it works with e10s

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Tentative fix

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.

Comment 2

[Andrew McCreight [:mccr8]]

Isn't this creating the XHR in the parent process? I'm not very familiar with browser-chrome tests.

Comment 3

[Gabriele Svelto [:gsvelto]]

(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 4

[Gabriele Svelto [:gsvelto]]

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.

Comment 5

[Boris Zbarsky [: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.

Comment 6

[Gabriele Svelto [:gsvelto]]

(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.

Comment 7

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Turn test_bug1011748.html into a browser mochitest to make it run properly in e10s mode

Revised patch, the try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffc07f55aeb3

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Gabriele Svelto [:gsvelto]]

Thanks for the review, the tests are green so let's land it.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b38540998a0

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b38540998a0