Closed Bug 1011748 Opened 6 years ago Closed 5 years ago

statusText attribute for XMLHttpRequest should be empty while doing redirect

Categories

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

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: xKhorasan, Assigned: Michael.milazzo, Mentored)

Details

(Whiteboard: [lang=c++])

Attachments

(2 files, 7 obsolete files)

User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko /20100101 Firefox/32.0
Build ID: 20140516030204

See bug 998076 comment 14 and bug998076 comment 15.

In WHATWG XMLHttpRequest (XHR) spec, the HEADERS_RECEIVED state is described as "All redirects (if any) have been followed and all HTTP headers of the final response have been received. Several response members of the object are now available." [1].  Since the statusText attribute of XHR is response's member, and XHR's state is OPENED while doing redirect, the statusText of XHR should be empty string while doing redirect.
However, currently statusText of XHR is not empty string while doing redirect in Firefox.

[1] http://xhr.spec.whatwg.org/#dom-xmlhttprequest-headers_received


Steps to reproduce:
1. launch firefox and open about:config
2. set "devtools.chrome.enabled" to true
3. open http://request-xkhorasan.rhcloud.com/test
4. press Shift+F4 to launch Scratchpad
5. paste attachment script to Scratchpad, and select "Browser" option in Environment menu
6. Press Ctrl+R (or command+R on MacOSX) to execute script


Actual result:
'["Moved Temporarily","OK"]' is alerted.

Expected result:
'["","OK"]' is alerted.
GetStatusText presumably needs to check the current state before doing anything.
Whiteboard: [mentor=bzbarsky@mit.ed][lang=c++]
Please use this patch instead.  I looked at what I originally posted and saw that it had other changes that didn't belong in the file.  Thanks!
Attachment #8436350 - Attachment is obsolete: true
Attachment #8436376 - Flags: review?(bzbarsky)
Comment on attachment 8436376 [details] [diff] [review]
This is a clean version of the previously submitted patch that only contains changes for nsXMLHttpRequest and a commit message in the expected format.

So this is checking readyState & 0x110, right?

That seems like a pretty confusing fragile way to test that readyState is not UNSENT or OPENED.  I'd prefer an explicit test for inequality against those two constants instead.

r=me with that fixed.
Attachment #8436376 - Flags: review?(bzbarsky) → review+
Here is the updated patch.  I was trying to write a unit test for this particular case.  I started down the mochitest path.  Do you feel that is the best way to test this particular issue?  Thanks!
Attachment #8436376 - Attachment is obsolete: true
Attachment #8437446 - Flags: review?(bzbarsky)
Comment on attachment 8437446 [details] [diff] [review]
Updated patch based on review feedback.

Now the indentation has gone a bit wonky; please fix that.  And no need to put the bug number in the comment: it's already in the commit message.  And add spaces after the "//".

r=me with that.

A mochitest is the right way to test this, yes.
Attachment #8437446 - Flags: review?(bzbarsky) → review+
Fixed the comment and spacing.  Thanks!
Attachment #8437446 - Attachment is obsolete: true
Attachment #8437457 - Flags: review?(bzbarsky)
Comment on attachment 8437457 [details] [diff] [review]
Made changes based on last review.

r=me
Attachment #8437457 - Flags: review?(bzbarsky) → review+
Michael, I assume you're planning to write the test before requesting that this be checked in?
Assignee: nobody → Michael.milazzo
Flags: needinfo?(Michael.milazzo)
(In reply to Boris Zbarsky [:bz] from comment #9)
> Michael, I assume you're planning to write the test before requesting that
> this be checked in?

Yes, I'm currently writing the mochitest.  I am trying to modify the test script attached to this bug to fit into the Mochitest framework.  I think I've resolved my issues with getting the Components.xxx objects resolved, but the "load" event handler isn't getting invoked.  I'm going to resume debugging it tonight after work.
Flags: needinfo?(Michael.milazzo)
Here is the mochitest unit test I made for this bug.  Thanks!
Attachment #8438866 - Flags: review?(bzbarsky)
Comment on attachment 8438866 [details] [diff] [review]
This patch contains the unit test and associated SJS files.

>+  		alert(xhr.statusText);

This line should go away.

Also, please replace the tabs in the test file with spaces.

With those fixed, looks great!
Attachment #8438866 - Flags: review?(bzbarsky) → review+
Removed the alert and tabs should not be in the HTML file.
Attachment #8438866 - Attachment is obsolete: true
Attachment #8440235 - Flags: review?(bzbarsky)
Comment on attachment 8440235 [details] [diff] [review]
Modified patch for mochitest unit tests.

Looks great.
Attachment #8440235 - Flags: review?(bzbarsky) → review+
Awesome, thanks!  Is there anything else I need to do in order to get my patch committed?
Add the checkin-needed keyword (I just did that).  I also did a try run with this patch, just in case: https://tbpl.mozilla.org/?tree=Try&rev=d04ab0004e73
Keywords: checkin-needed
Hmm.  So the only failures are on M-e10s (which only runs on Linux opt builds on tbpl, which is why my try push did not pick it up) and on B2G ICS Emulator, which is also running e10s.

I can definitely reproduce on Mac (including in a debug build) if I run the new test with "mach mochitest-plain --e10s".

Michael, are you willing to take a crack at figuring out what's going there, or do you want me to?
Flags: needinfo?(Michael.milazzo)
(In reply to Boris Zbarsky [:bz] from comment #19)
> Hmm.  So the only failures are on M-e10s (which only runs on Linux opt
> builds on tbpl, which is why my try push did not pick it up) and on B2G ICS
> Emulator, which is also running e10s.
> 
> I can definitely reproduce on Mac (including in a debug build) if I run the
> new test with "mach mochitest-plain --e10s".
> 
> Michael, are you willing to take a crack at figuring out what's going there,
> or do you want me to?


I'll take a crack at it and let you know if I need any assistance.  Thanks!
Flags: needinfo?(Michael.milazzo)
Hmm, IMHO bug 998076 comment 19 might help you.
So I've done a little digging into this issue and I'd like to bounce an idea off of everyone just to make sure I'm going down the right path.  Using the specialpowers-http-notify-request invalidates the test because it only provides notification once all the redirects have been followed.  I did not see a specialpowers- notification for the http notification I need to know about.

I think a way around the issue is to modify SpecialPowersObserver.js so that is can proxy these notifications to the observers running in the content processes similar to how http-on-modify-request is proxied.  I was thinking about specifying the proxy notification as specialpowers-http-on-examine-response.  Am I going in the right direction?
Flags: needinfo?(bzbarsky)
That makes sense, yes.  Alternately, we could try disabling the test in e10s (e.g. via try/catch around the observer bits)...  Probably better to make it work if we can, though.
Flags: needinfo?(bzbarsky)
Mentor: bzbarsky
Whiteboard: [mentor=bzbarsky@mit.ed][lang=c++] → [lang=c++]
Is there anything special that needs to be done if you change testing/specialpowers/components/SpecialPowersObserver.js or testing/specialpowers/content/specialpowersAPI.js?  I modified both files to support proxying http-on-examine-response just as http-on-modify-request was and it doesn't appear that my changes are being loaded when I run mochitest.  Is there a really good way to debug this?  I tried debugging using the Debugging console within the nightly build, but I can't see the SpecialPowersObserver code in the debugger.
Flags: needinfo?(xKhorasan+mozilla)
Please disregard my previous post.  I figured it out and now the test passes in both single process and e10 modes.
Attachment #8444018 - Flags: review?(bzbarsky)
Flags: needinfo?(xKhorasan+mozilla)
Comment on attachment 8444018 [details] [diff] [review]
Modifications made so the test works in e10 mode also.

>+  var statusTexts = [];

Should be ["", ""] if you're later going to assert that is(statusTexts[0], "").

I assume you've checked that the test fails without the patch, right?

r=me
Attachment #8444018 - Flags: review?(bzbarsky) → review+
It actually passes in E10 mode without the patch applied.  Do you know if E10 mode uses the same XMLHttpRequest class?  I'm inclined to think that it doesn't based on the debugging I've done on my test.  Oddly, I am also getting three different notifications for specialpowers-http-on-examine-response in E10 mode and only two in non-E10 mode. The value of xhr.statusText for the first two notifications is "", and the last one is "OK".  In non-E10 mode, it returned "Moved Temporarily", then "OK" using exactly two notifications when the patch was not applied.  Any advice on where to look in the source for E10 XHR to dig further?
It does use the same XHR class, but a different HTTP channel implementation.  The actual HTTP load happens in the parent process and it's possible we don't send the redirect notifications quite correctly down to the child process...

I wouldn't worry about this passing in e10s for now without the patch as long as it fails without the patch in non-e10s.  <aybe file a followup bug on necko for the e10s weirdness?
This patch includes the code change and all the mochitest mods in a single file.  I omitted the mods to specialpowers and also specified to skip the test if using b2g or e10.  Could you please push this to the try server to see if it passes all clear?
Attachment #8437457 - Attachment is obsolete: true
Attachment #8440235 - Attachment is obsolete: true
Attachment #8444018 - Attachment is obsolete: true
Attachment #8445692 - Flags: review?(bzbarsky)
Comment on attachment 8445692 [details] [diff] [review]
Patch that includes code changes and non-E10 mochitest unit test mods in a single file.

r=me

I pushed https://tbpl.mozilla.org/?tree=Try&rev=7838bb8c3703
Attachment #8445692 - Flags: review?(bzbarsky) → review+
Try run looks good.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/09e37635fc07
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Michael, thank you for sticking with this when it sort of scope-crept!
Thank you!  It was a fun bug to fix and I learned a quite a bit in the process.  Do you have any recommendations on where to start looking for the e10 signaling so I can track down the behavior I found while hashing out the mochitest?
I don't know offhand, but Jason Duell might.
Flags: needinfo?(jduell.mcbugs)
> I think a way around the issue is to modify SpecialPowersObserver.js so that
> is can proxy these notifications to the observers running in the content
> processes similar to how http-on-modify-request is proxied. 

I didn't know that we proxied on-{modify|examine}-request, etc to the child.  Is that something that we need to do just for specific tests that ask for it, or do we need those notifications to be fired in the child in general?  If the latter, we should probably have necko fire them (they're currently disabled: see bug
806753) instead of whatever (specialPowers-only?) method we're using now.

>  Oddly, I am also getting three different notifications for
>  specialpowers-http-on-examine-response in E10 mode and only two in non-E10
>  mode.

Necko (the networking layer) doesn't fire http-on-examine-response in the child, so all these notifications must be for the "real" parent channel (not the "stub" HttpChannelChild that lives in the child, which is really just a client of the real parent channel).  I can't think of any reason why the necko e10s architecture would fire off 3 examine-response notifications instead of 2.  My first suggestion would be to fire up a debugger and set a breakpoint where the http-examine-response is fired, and see if it actually happens 3 times: 

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1237

If not I'd suspect something is duplicating the notification in the SpecialPowers logic.

Honza may have some idea here too.
Flags: needinfo?(jduell.mcbugs) → needinfo?(honzab.moz)
Do the break points.  I don't know specialpowers to just say if it's coming from there.
Flags: needinfo?(honzab.moz)
I've filed bug 1222128 for fixing this test in e10s.
You need to log in before you can comment on or make changes to this bug.