1360052 - Request cancellation bug with chrome.webRequest

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P2)

Description

[Alexei]

Attachment: Basic WebExtensions extension to help demonstrate the chrome.webRequest bug

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36

Steps to reproduce:

Cancelling a 3xx redirect request that also includes a Set-Cookie header still produces a cookie, even though the request gets cancelled. No cookies get set in this scenario in Chrome.

To reproduce, please review and install the attached WebExtension and then visit https://dnt-test.trackersimulator.org/.well-known/dnt-policy.txt. A cookie will get set even though the request was cancelled and no redirect took place.

The URL above responds with a 302 status code and a Location header. It also includes a Set-Cookie header.

The attached extension filters requests inside a blocking chrome.webRequest.onHeadersReceived listener. Redirects get cancelled and Set-Cookie headers get stripped.

I have not checked if chrome.webRequest.onBeforeRequest is similarly affected, it may or not be.


Actual results:

Cookie(s) get set.


Expected results:

No cookie(s) get set.

Comment 2

[Alexei]

I don't think this is a duplicate, as this is still an issue on firefox-54.0b2 (Beta) and firefox-55.0a1.en-US.linux-x86_64 (Nightly).

Comment 3

[Shane Caraveo (:mixedpuppy)]

I agree, bug 1345893 was specific to handling suspended channels during http-on-modify-request (onBeforeRequest), this appears to be specific to http-on-examine-* and may be unrelated to suspending the channel (ie. suspend is handled for on-examine-response at least).  

Observers apparently not handling async suspend in nsHttpChannel:

http-on-opening-request (gHttpHandler->OnOpeningRequest in httpchannel)
http-on-examine-merged-response (gHttpHandler->OnExamineMergedResponse in httpchannel)

I'm not sure yet if the issue here is async handling of merged-response, or if cookie handling happens in a different order on Firefox than on Chrome.

Because onHeadersReceived is the first potential place to examine a response, I think a webextension should have the opportunity to do something before cookies are handled.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1360052 handle cancel during http-on-examine-response,

Review commit: https://reviewboard.mozilla.org/r/135618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135618/

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8863897 [details]
Bug 1360052 handle cancel during http-on-examine-response,

Honza, I haven't written a test for this (using the addon in this bug to test manually), just want feedback to see if this is fine then I'll add a test.

Specifically we're not handling cancel right after http-on-examine-response, so cookies still get set, then the cancel is handled at some point afterwards.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8863897 [details]
Bug 1360052 handle cancel during http-on-examine-response,

https://reviewboard.mozilla.org/r/135618/#review139250

::: netwerk/protocol/http/nsHttpChannel.cpp:2089
(Diff revision 1)
>          return NS_OK;
>      }
>  
> +    // Check if request was cancelled during http-on-examine-response.
> +    if (mCanceled) {
> +        return mStatus;

No.  Please do this:

        if (mCanceled) {
            return CallOnStartRequest();
        }

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8863897 [details]
Bug 1360052 handle cancel during http-on-examine-response,

rb is sooooooooooooo stupid!

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8863897 [details]
Bug 1360052 handle cancel during http-on-examine-response,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135618/diff/1-2/

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8863897 [details]
Bug 1360052 handle cancel during http-on-examine-response,

https://reviewboard.mozilla.org/r/135618/#review140114

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0087592da59a
handle cancel during http-on-examine-response, r=mayhemer

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0087592da59a