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

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: hiro, Assigned: stone)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 wontfix, firefox47 fixed, firefox48 fixed)

Details

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

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Updated

4 years ago
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

Updated

3 years ago
Flags: needinfo?(mrbkap)
Flags: needinfo?(mrbkap) → needinfo?(ehsan)

Comment 2

3 years ago
Stone, since you're already investigating bug 1255597, do you mind looking at this one as well?  Thanks!
Flags: needinfo?(ehsan) → needinfo?(sshih)
Assignee

Comment 3

3 years ago
Sure. I will investigate it after 1255597.
Assignee: nobody → sshih
Flags: needinfo?(sshih)
Assignee

Comment 4

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

Comment 5

3 years ago
This issue isn't intermittent but consistently fails in e10s
Component: web-platform-tests → Networking: HTTP
Product: Testing → Core
Assignee

Comment 6

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

Comment 8

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

Updated

3 years ago
Whiteboard: [necko-active]
Assignee

Comment 9

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

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

Updated

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

Comment 16

3 years ago
Follow reviewer's feedbacks to refine codes and add comments.
Attachment #8742169 - Attachment is obsolete: true
Attachment #8742680 - Flags: review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0afad0814a3c
https://hg.mozilla.org/mozilla-central/rev/da6890d34fd3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(sshih)
Assignee

Comment 20

3 years ago
Ongoing. Waiting for try server results.
Assignee

Updated

3 years ago
Flags: needinfo?(sshih)
Assignee

Comment 22

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