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

RESOLVED FIXED in Firefox 61

Status

enhancement
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

unspecified
mozilla61
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

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

Comment 3

a year ago
I think that sounds fine, but Honza probably knows better than I do.
Flags: needinfo?(jduell.mcbugs) → needinfo?(honzab.moz)

Comment 5

a year ago
mozreview-review
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+
Comment hidden (mozreview-request)
Assignee: nobody → dtownsend
Priority: -- → P3

Comment 7

a year ago
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)

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9fb3e5aba482
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Comment 9

a year ago
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.

Updated

a year ago
Flags: needinfo?(dtownsend)
Flags: needinfo?(dtownsend) → qe-verify-

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.