Closed Bug 1225756 Opened 5 years ago Closed 5 years ago

e10s XMLHttpRequest/send-redirect-bogus.htm | XMLHttpRequest: send() - Redirects (bogus Location header) (302: mailto:someone@example.org) - assert_equals: expected 0 but got 302

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: hiro, Assigned: stone)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active] btpp-active)

Attachments

(3 files, 3 obsolete files)

Summary: INtermittent XMLHttpRequest/send-redirect-bogus.htm | XMLHttpRequest: send() - Redirects (bogus Location header) (302: mailto:someone@example.org) - assert_equals: expected 0 but got 302 → Intermittent XMLHttpRequest/send-redirect-bogus.htm | XMLHttpRequest: send() - Redirects (bogus Location header) (302: mailto:someone@example.org) - assert_equals: expected 0 but got 302
I'm pretty sure this isn't intermittent but consistently fails in e10s.
tracking-e10s: --- → ?
Summary: Intermittent XMLHttpRequest/send-redirect-bogus.htm | XMLHttpRequest: send() - Redirects (bogus Location header) (302: mailto:someone@example.org) - assert_equals: expected 0 but got 302 → e10s XMLHttpRequest/send-redirect-bogus.htm | XMLHttpRequest: send() - Redirects (bogus Location header) (302: mailto:someone@example.org) - assert_equals: expected 0 but got 302
Flags: needinfo?(mrbkap)
Flags: needinfo?(mrbkap) → needinfo?(ehsan)
Stone, since you're already investigating bug 1255597, do you mind looking at this one as well?  Thanks!
Flags: needinfo?(ehsan) → needinfo?(sshih)
Sure. I will investigate it after 1255597.
Assignee: nobody → sshih
Flags: needinfo?(sshih)
(In reply to Ming-Chou Shih [:stone] from comment #3)
> Sure. I will investigate it after 1255597.
Sorry for the typo, I don't mind to investigate it.
This issue isn't intermittent but consistently fails in e10s
Component: web-platform-tests → Networking: HTTP
Product: Testing → Core
The test case 'send-redirect-bogus.htm' will test HTTP redirect with invalid location 'mailto:someone@example.org'. The expected result is that XHR gets failed result and return 0 when client retrieving its status.

On non-e10s, HTTP response with redirecting to 'mailto:someone@example.org' is cancelled by nsCORSListenerProxy because the created channel doesn't support interface 'nsIHttpChannel'. It behaved as expected.

On e10s, before nsAsyncRedirectVerifyHelper notifying global observers (including nsCORSListenerProxy), HttpChannelChild query channel with interface 'nsIChildChannel' failed and return NS_ERROR_FAILURE, which is not handled correctly when nsHttpChannel processing response. In this case, nsHttpChannel will treat it as succeed. XHR will retrieve status from HTTP channel and gets 302.

Change the return error to NS_ERROR_DOM_BAD_URI can let nsHttpChannel::ContinueProcessResponse2 handle it as a failure and cancel itself.

Would you mind to review this patch for me? Thanks.
Attachment #8733795 - Flags: review?(jduell.mcbugs)
Hmm, are we currently hitting this check? <https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1806>  That seems super fragile...  Perhaps we should relax that check and turn it into something like NS_FAILED(rv) too?
Whiteboard: [necko-active]
(In reply to :Ehsan Akhgari from comment #8)
> Hmm, are we currently hitting this check?
> <https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpChannel.cpp#1806>  That seems super fragile...  Perhaps we should
> relax that check and turn it into something like NS_FAILED(rv) too?

It is the check block other failures to cancel the channel. I tried NS_FAILED(rv) but unfortunately it leads failures of other test cases (test cases in /fetch/api/redirect/redirect-location-worker.html). Here are the tried result https://treeherder.mozilla.org/#/jobs?repo=try&revision=4130f07a0888.

It looks like there are other cases to be considered if we want to relax the check. I am happy to dig more but I need some suggestions to do it or not.
Whiteboard: [necko-active] → [necko-active] btpp-active
Comment on attachment 8733795 [details] [diff] [review]
Refine error code when redirecting to invalid location to let nsHttpChannel process it as error.

Review of attachment 8733795 [details] [diff] [review]:
-----------------------------------------------------------------

Honza, I think you're a better reviewer for this than me.  I'm not clear on whether this approach is OK for all cases where we get a channel that isn't a nsIChildChannel.
Attachment #8733795 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Attachment #8733797 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Comment on attachment 8733795 [details] [diff] [review]
Refine error code when redirecting to invalid location to let nsHttpChannel process it as error.

Review of attachment 8733795 [details] [diff] [review]:
-----------------------------------------------------------------

I think the better way is to call gHttpHandler->AsyncOnChannelRedirect in HttpChannelChild::Redirect1Begin even if don't have mRedirectChannelChild.  In HttpChannelChild::OnRedirectVerifyCallback then fail the result if !mRedirectChannelChild, maybe to DOM_BAD_URI.

The goal is tho to first let the regular veto handlers check on this redirect and try to get the correct error result from them, and not to synthesize it here, what should only be done as the last resort.
Attachment #8733795 - Flags: review?(honzab.moz) → review-
Attachment #8733797 - Flags: review?(honzab.moz) → review?(james)
Attachment #8733797 - Flags: review?(james) → review+
Comment on attachment 8742169 [details] [diff] [review]
Part1: Let the regular veto handlers check on redirects then check mRedirectChannelChild in the result callback. r?mayhemer

Review of attachment 8742169 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but only with the comments addressed.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1536,5 @@
>    nsCOMPtr<nsIHttpChannel> newHttpChannel =
>        do_QueryInterface(mRedirectChannelChild);
>  
> +  if (NS_SUCCEEDED(result)) {
> +    if (!mRedirectChannelChild) {

nit: 

if (NS_SUCCEEDED(result) && !mRedirectChannelChild) {
}

explain reasonable in a comment why you set NS_ERROR_DOM_BAD_URI
Attachment #8742169 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #14)
> explain reasonable 

*reasonably*
Follow reviewer's feedbacks to refine codes and add comments.
Attachment #8742169 - Attachment is obsolete: true
Attachment #8742680 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0afad0814a3c
https://hg.mozilla.org/mozilla-central/rev/da6890d34fd3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(sshih)
Ongoing. Waiting for try server results.
Flags: needinfo?(sshih)
Comment on attachment 8744818 [details] [diff] [review]
Let the regular veto handlers check on redirects then check mRedirectChannelChild in the result callback. r=mayhemer,jgraham

Approval Request Comment
[Feature/regressing bug #]:
NA
[User impact if declined]:
When XHR received response which redirects to a URI with invalid protocol on e10s will get status=302 (expect 0)
[Describe test coverage new/current, TreeHerder]:
http://w3c-test.org/XMLHttpRequest/send-redirect-bogus.htm
[Risks and why]: 
NA
[String/UUID change made/needed]:
NA
Attachment #8744818 - Flags: approval-mozilla-aurora?
Attachment #8744818 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8744818 [details] [diff] [review]
Let the regular veto handlers check on redirects then check mRedirectChannelChild in the result callback. r=mayhemer,jgraham

e10s related, Beta47+
Attachment #8744818 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.