Closed Bug 1449259 Opened 2 years ago Closed 2 years ago

Having devtools open can cause proxy requests with unknown tabId and other content policy type

Categories

(WebExtensions :: Request Handling, enhancement, P3)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mossop, Assigned: mossop)

Details

Attachments

(1 file)

Because devtools wants to access the original content from a request sometimes it must do an extra request when the original request came from the bytecode cache. It doesn't associate this request with any particular load context, nor does it include the content policy type.

All of this ends up making a proxy request through the webextension api.

Since the request is always going to be served by the cache, I suspect the proxy request doesn't make sense at all and we should probably just make the HTTP channel not do a proxy lookup when LOAD_ONLY_FROM_CACHE is set.
(In reply to Dave Townsend [:mossop] from comment #0)
> Since the request is always going to be served by the cache, I suspect the
> proxy request doesn't make sense at all and we should probably just make the
> HTTP channel not do a proxy lookup when LOAD_ONLY_FROM_CACHE is set.

Is this change something that would be acceptable?
Flags: needinfo?(jduell.mcbugs)
Component: WebExtensions: General → WebExtensions: Request Handling
I think that sounds fine, but Honza probably knows better than I do.
Flags: needinfo?(jduell.mcbugs) → needinfo?(honzab.moz)
Comment on attachment 8965136 [details]
Bug 1449259: Don't do proxy lookups for loads that are only going to come from the cache.

https://reviewboard.mozilla.org/r/233824/#review239626

LOAD_ONLY_FROM_CACHE fails the load when the cache entry is not present or needs validation.  So, we will never send a request to the originating server.

I think this change is OK.

::: netwerk/protocol/http/nsHttpChannel.cpp:6072
(Diff revision 1)
>      mCustomAuthHeader = mRequestHead.HasHeader(nsHttp::Authorization);
>  
>      // The common case for HTTP channels is to begin proxy resolution and return
>      // at this point. The only time we know mProxyInfo already is if we're
>      // proxying a non-http protocol like ftp.
> -    if (!mProxyInfo && NS_SUCCEEDED(ResolveProxy())) {
> +    if (!mProxyInfo && !(mLoadFlags & LOAD_ONLY_FROM_CACHE) && NS_SUCCEEDED(ResolveProxy())) {

please add LOAD_NO_NETWORK_IO as well.
Attachment #8965136 - Flags: review?(honzab.moz) → review+
Assignee: nobody → dtownsend
Priority: -- → P3
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9fb3e5aba482
Don't do proxy lookups for loads that are only going to come from the cache. r=mayhemer
Flags: needinfo?(honzab.moz)
https://hg.mozilla.org/mozilla-central/rev/9fb3e5aba482
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(dtownsend)
Flags: needinfo?(dtownsend) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.