:xKhorasan Reporter
	Description • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 10 years ago

GetStatusText presumably needs to check the current state before doing anything.

	Michael A. Milazzo Assignee
	Comment 3 • 10 years ago

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!

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 10 years ago

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.

	Michael A. Milazzo Assignee
	Comment 5 • 10 years ago

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!

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 10 years ago

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.

	Michael A. Milazzo Assignee
	Comment 7 • 10 years ago

Fixed the comment and spacing.  Thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 10 years ago

Comment on attachment 8437457 [details] [diff] [review]
Made changes based on last review.

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 10 years ago

Michael, I assume you're planning to write the test before requesting that this be checked in?

	Michael A. Milazzo Assignee
	Comment 10 • 10 years ago

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

	Michael A. Milazzo Assignee
	Comment 11 • 10 years ago

Here is the mochitest unit test I made for this bug.  Thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 10 years ago

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!

	Michael A. Milazzo Assignee
	Comment 13 • 10 years ago

Removed the alert and tabs should not be in the HTML file.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 10 years ago

Comment on attachment 8440235 [details] [diff] [review]
Modified patch for mochitest unit tests.

Looks great.

	Michael A. Milazzo Assignee
	Comment 15 • 10 years ago

Awesome, thanks!  Is there anything else I need to do in order to get my patch committed?

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 17 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fc4cf38057e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc765663d8f8

	Carsten Book [:Tomcat]
	Comment 18 • 10 years ago

sorry had to backout for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=41776923&tree=Mozilla-Inbound

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 10 years ago

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?

	Michael A. Milazzo Assignee
	Comment 20 • 10 years ago

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

	:xKhorasan Reporter
	Comment 21 • 10 years ago

Hmm, IMHO bug 998076 comment 19 might help you.

	Michael A. Milazzo Assignee
	Comment 22 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 10 years ago

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.

	Michael A. Milazzo Assignee
	Comment 24 • 10 years ago

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.

	Michael A. Milazzo Assignee
	Comment 25 • 10 years ago

Please disregard my previous post.  I figured it out and now the test passes in both single process and e10 modes.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 10 years ago

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

	Michael A. Milazzo Assignee
	Comment 27 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 10 years ago

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?

	Michael A. Milazzo Assignee
	Comment 29 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 10 years ago

Try run looks good.

	Carsten Book [:Tomcat]
	Comment 32 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/09e37635fc07

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/09e37635fc07

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 10 years ago

Michael, thank you for sticking with this when it sort of scope-crept!

	Michael A. Milazzo Assignee
	Comment 35 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 10 years ago

I don't know offhand, but Jason Duell might.

	Jason Duell
	Comment 37 • 10 years ago

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

	Honza Bambas (:mayhemer)
	Comment 38 • 10 years ago

Do the break points.  I don't know specialpowers to just say if it's coming from there.

	Andrew McCreight [:mccr8]
	Comment 39 • 9 years ago

I've filed bug 1222128 for fixing this test in e10s.