1225756 - e10s XMLHttpRequest/send-redirect-bogus.htm | XMLHttpRequest: send() - Redirects (bogus Location header) (302: mailto:[e-mail]) - assert_equals: expected 0 but got 302

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/logviewer.html#?job_id=13858433&repo=try

Comment 1

[James Graham [:jgraham]]

I'm pretty sure this isn't intermittent but consistently fails in e10s.

Comment 2

[(no longer active)]

Stone, since you're already investigating bug 1255597, do you mind looking at this one as well?  Thanks!

Comment 3

[Ming-Chou Shih [:stone]]

Sure. I will investigate it after 1255597.

Comment 4

[Ming-Chou Shih [:stone]]

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

Comment 5

[Ming-Chou Shih [:stone]]

This issue isn't intermittent but consistently fails in e10s

Comment 6

[Ming-Chou Shih [:stone]]

Attachment: Refine error code when redirecting to invalid location to let nsHttpChannel process it as error.

The test case 'send-redirect-bogus.htm' will test HTTP redirect with invalid location 'mailto:[e-mail]'. 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:[e-mail]' 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.

Comment 7

[Ming-Chou Shih [:stone]]

Attachment: Remove 'expected fail' settings and expect testcases in send-redirect-bogus.htm should be passed.

Comment 8

[(no longer active)]

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?

Comment 9

[Ming-Chou Shih [:stone]]

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

Comment 10

[Jason Duell]

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.

Comment 11

[Honza Bambas (:mayhemer)]

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.

Comment 12

[Ming-Chou Shih [:stone]]

Attachment: Part2: Remove 'expected fail' settings and expect testcases in send-redirect-bogus.htm should be passed. r=jgraham

Changed reviewer in patch summary

Comment 13

[Ming-Chou Shih [:stone]]

Attachment: Part1: Let the regular veto handlers check on redirects then check mRedirectChannelChild in the result callback. r?mayhemer

Comment 14

[Honza Bambas (:mayhemer)]

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

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #14)
> explain reasonable 

*reasonably*

Comment 16

[Ming-Chou Shih [:stone]]

Attachment: Part1: Let the regular veto handlers check on redirects then check mRedirectChannelChild in the result callback. r=mayhemer

Follow reviewer's feedbacks to refine codes and add comments.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0afad0814a3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/da6890d34fd3

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0afad0814a3c
https://hg.mozilla.org/mozilla-central/rev/da6890d34fd3

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 20

[Ming-Chou Shih [:stone]]

Ongoing. Waiting for try server results.

Comment 21

[Ming-Chou Shih [:stone]]

Attachment: Let the regular veto handlers check on redirects then check mRedirectChannelChild in the result callback. r=mayhemer,jgraham

Merged the origin patches to single patch and changed its summary.
try results are in https://treeherder.mozilla.org/#/jobs?repo=try&revision=69754a2625533e5bcd5c9d063ba5926aab89b152

Comment 22

[Ming-Chou Shih [:stone]]

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

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Uplifted to beta in https://hg.mozilla.org/releases/mozilla-beta/rev/73b0aecf21171693fb31efa4a3b0a40aa55bb1d0