Fix test_bug1011748.html so it works with e10s

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: gsvelto)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox45 affected, firefox48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.

Updated

3 years ago
Blocks: 984139
tracking-e10s: --- → +
(Assignee)

Comment 1

3 years ago
Created attachment 8738530 [details] [diff] [review]
[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.
Attachment #8738530 - Flags: feedback?(continuation)
(Reporter)

Comment 2

3 years ago
Isn't this creating the XHR in the parent process? I'm not very familiar with browser-chrome tests.
(Reporter)

Updated

3 years ago
Attachment #8738530 - Flags: feedback?(continuation)
(Assignee)

Comment 3

3 years ago
(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.
(Assignee)

Comment 4

3 years ago
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+
(Assignee)

Comment 6

3 years ago
(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
(Assignee)

Comment 7

3 years ago
Created attachment 8739965 [details] [diff] [review]
[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
Attachment #8738530 - Attachment is obsolete: true
Attachment #8739965 - Flags: review?(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
Attachment #8739965 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 9

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

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b38540998a0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.