1231512 - Get NS_ERROR_IN_PROGRESS error when redirecting a request from onHeadersReceived.

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Blake Winton (:bwinton) (:☕️)]

I'm getting a "NS_ERROR_IN_PROGRESS [nsIHttpChannel.redirectTo]" when I try to redirect a request from onHeadersReceived.
https://developer.chrome.com/extensions/webRequest#type-BlockingResponse suggests it should be possible…
Failing code at https://github.com/bwinton/404archive/tree/3cb822a7796f10c5adcce600a7c3a2c70cb3f5f8

Comment 1

[Giorgio Maone [:ma1]]

Reason is https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1781

We are calling nsIHttpChannel.redirectTo() on an open channel (in a http-on-examine-response observer).
This cannot work since this method just replaces the URI to be later opened by asyncOpen(), and in facts the channel being not open yet is the precondition correctly enforced here.

In order to behave per-spec, we need to patch the code after the notification call

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

so that a redirection is forced if "something" happens in the observers.

We could either add an ad-hoc redirectResponse() method or recycle the redirectTo() method relaxing its precondition so that NS_ERROR_IN_PROGRESS is thrown only if the headers have already been processed, rather than just if the channel has been opened. In the latter case the easier way to implement the forced redirection seems to be emulating a "proper" one (by changing httpStatus to 300 and injecting a synthetic Location header). Alternatively we might refactor out the HTTP-specific bits of the redirection machinery (which is quite complex anyway because it deals with cache and cloning the original channel) and call it directly if mApiRedirectToURI is found changed after the observers have been notified. Either way I think we should skip event sink notifications in this special case in order to mimic current redirectTo() behavior.

Necko peers, what do you think?

Comment 2

[Honza Bambas (:mayhemer)]

I think it should be relatively (if not super) easy to implement this.  When I check [1], it seems we could do the same (or similar, synchronous way) after the gHttpHandler->OnExamineResponse(this) call at [2].


[1] http://hg.mozilla.org/mozilla-central/annotate/29258f59e545/netwerk/protocol/http/nsHttpChannel.cpp#l5122
[2] http://hg.mozilla.org/mozilla-central/annotate/29258f59e545/netwerk/protocol/http/nsHttpChannel.cpp#l1486

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v1

This could be the patch..  includes a test.

Comment 4

[Honza Bambas (:mayhemer)]

Jason, ping on review.

Comment 5

[Jason Duell]

Comment on attachment 8704266 [details] [diff] [review]
v1

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

So this patch supports doing redirectTo() at ProcessResponse() time.  IIUC that means an observer for on-examine-response can redirect, but an observer for on-examine-cached response (or on-examine-merged-response) won't be able to redirect?  Do we want to support late redirects for cached responses?  I don't see why not.

We definitely need to update nsIHttpChannel.idl docs for .redirectTo as part of this patch.  Right now it says "Can only be called on channels not yet opened" (which is a lie, we support until on-modify-request, which happens after asyncOpen returns.  And we're obviously allowing it even later with this patch).

::: netwerk/test/unit/test_redirect_from_script_after-open_passing.js
@@ +27,5 @@
>  Cu.import("resource://gre/modules/NetUtil.jsm");
>  
>  // the topic we observe to use the API.  http-on-opening-request might also
>  // work for some purposes.
> +redirectHook = "http-on-examine-response";

so you just changed test to do redirectTo during On-examine-response instead of on on-modify-request?  I'm guessing it would be nice to test both?

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #5)
> Comment on attachment 8704266 [details] [diff] [review]
> v1
> 
> Review of attachment 8704266 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So this patch supports doing redirectTo() at ProcessResponse() time.  IIUC
> that means an observer for on-examine-response can redirect, but an observer
> for on-examine-cached response (or on-examine-merged-response) won't be able
> to redirect?  Do we want to support late redirects for cached responses?  I
> don't see why not.

Good point.

> 
> We definitely need to update nsIHttpChannel.idl docs for .redirectTo as part
> of this patch.  Right now it says "Can only be called on channels not yet
> opened" (which is a lie, we support until on-modify-request, which happens
> after asyncOpen returns.  And we're obviously allowing it even later with
> this patch).

Also a good point.

> 
> ::: netwerk/test/unit/test_redirect_from_script_after-open_passing.js
> @@ +27,5 @@
> >  Cu.import("resource://gre/modules/NetUtil.jsm");
> >  
> >  // the topic we observe to use the API.  http-on-opening-request might also
> >  // work for some purposes.
> > +redirectHook = "http-on-examine-response";
> 
> so you just changed test to do redirectTo during On-examine-response instead
> of on on-modify-request?  I'm guessing it would be nice to test both?

Disadvantage of spliter: it's a new file, |hg cp| of test_redirect_from_script.js.  So, we do test both.  I like it more as a separate tests (can go parallel).

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #5)
> an observer
> for on-examine-cached response 

Is called asynchronously after ReadFromCache has fired off the cache pump.  We cannot do redirects after that point anymore.

> (or on-examine-merged-response) 

That seems possible.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v2

Grew up a bit...

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8713843 [details] [diff] [review]
v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=494d4334d3a9

Comment 10

[Jason Duell]

Comment on attachment 8713843 [details] [diff] [review]
v2

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

Nice patch.  Glad we can handle cache hits, too.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1789,5 @@
> +  // This would break the nsIStreamListener contract.
> +  NS_ENSURE_FALSE(mOnStartRequestCalled, NS_ERROR_NOT_AVAILABLE);
> +
> +  // Remember where we want to redirect.  Non-null means to redirect
> +  // on few places instead of starting this channel content load.

"Non-null means to redirect on few places instead of starting this channel content load."

I confess I totally don't understand that sentence :)  Maybe it should say what happens if you set it to null?  The nsIHttpChannel docs don't mention that case, BTW.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1528,5 @@
> +        }
> +        PopRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse1);
> +    }
> +
> +    // Pretend error, which means we haven't started async redirection

"Hack: ContinueProcessResponse1 uses NS_OK to detect successful redirects, so we distinguish this codepath (a non-redirect that's processing normally) by passing in a bogus error code."

(Nice hack, by the way :)

@@ +1538,5 @@
> +{
> +    if (NS_SUCCEEDED(rv)) {
> +        // redirectTo has passed through, we don't want to go on
> +        // with this channel.  It will now be canceled by the redirect
> +        // handling code where from we are now called.

"It will now be canceled by the redirect handling code that called this function."

@@ +1988,5 @@
>  nsresult
>  nsHttpChannel::ContinueAsyncRedirectChannelToURI(nsresult rv)
>  {
> +    // Since we handle mAPIRedirectToURI also after on-examine-response handler
> +    // rather drop it here to avoid any redirect loops, even just hypotetical.

hypothetical (missing 'h')

@@ +5763,5 @@
> +        }
> +        PopRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1);
> +    }
> +
> +    // Pretend error, which means we have not started async redirection

same same

@@ +5773,5 @@
> +{
> +    if (NS_SUCCEEDED(result)) {
> +        // redirectTo has passed through, we don't want to go on
> +        // with this channel.  It will now be canceled by the redirect
> +        // handling code where from we are now called.

same

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +369,5 @@
>      boolean isPrivateResponse();
>  
>      /**
>       * Instructs the channel to immediately redirect to a new destination.
> +     * Can only be called on channels that didn't call on its listener.

"Can only be called on channels that have not yet called their listener's OnStartRequest()"?

(Is there another callback that could be called earlier?)

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #10)
> Comment on attachment 8713843 [details] [diff] [review]
> v2
> 
> Review of attachment 8713843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice patch.  Glad we can handle cache hits, too.

Thanks for the review!

> 
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +1789,5 @@
> > +  // This would break the nsIStreamListener contract.
> > +  NS_ENSURE_FALSE(mOnStartRequestCalled, NS_ERROR_NOT_AVAILABLE);
> > +
> > +  // Remember where we want to redirect.  Non-null means to redirect
> > +  // on few places instead of starting this channel content load.
> 
> "Non-null means to redirect on few places instead of starting this channel
> content load."
> 
> I confess I totally don't understand that sentence :)  Maybe it should say
> what happens if you set it to null?  The nsIHttpChannel docs don't mention
> that case, BTW.

The comment tries to say that when this is set to something (non-null) we will attempt to redirect before the content would be delivered.  I will rephrase to make it more clear.

> >       * Instructs the channel to immediately redirect to a new destination.
> > +     * Can only be called on channels that didn't call on its listener.
> 
> "Can only be called on channels that have not yet called their listener's
> OnStartRequest()"?
> 
> (Is there another callback that could be called earlier?)

Not sure what you mean?

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v2.1

- updated comments, mainly the IDL one

Comment 13

[Honza Bambas (:mayhemer)]

Mike, this is just FYI, if you are using redirectTo from "on-modify-request" nothing is changing for you.

When using redirectTo() from "on-modify-request" the behavior regarding redirect veto has not changed, we still cancel the original channel and prevent the original content to load.  However, when redirectTo() is called later, we deliver the original URL content to the consumer when redirect is vetoed.

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: v2.2

(slightly better comments in the IDL)

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/448af29079c9

Comment 16

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

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f396bd8293e4 for being a possible cause of build bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=21169623&repo=mozilla-inbound

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Wes Kocher (:KWierso) from comment #16)
> I had to back this out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f396bd8293e4 for
> being a possible cause of build bustage:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=21169623&repo=mozilla-
> inbound

According [1] and in details [2] it builds fine.
According bug 1246109 comment 7 this is fixed, right?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=494d4334d3a9
[2] https://treeherder.mozilla.org/logviewer.html#?job_id=16202312&repo=try

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/969584470623

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/969584470623

Comment 20

[Vasilica Tamas, QA [:vtamas]]

Honza, is there any proper way of manually testing this bug?

Comment 21

[Giorgio Maone [:ma1]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #20)
> Honza, is there any proper way of manually testing this bug?

https://bugzilla.mozilla.org/show_bug.cgi?id=1176092#c1