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

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, enhancement, P3)

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Dave Townsend [:mossop]]

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

Comment 2

[Dave Townsend [:mossop]]

Forgot to include the link to the code that does this: https://searchfox.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#462

Comment 3

[Jason Duell]

I think that sounds fine, but Honza probably knows better than I do.

Comment 4

[Dave Townsend [:mossop]]

Attachment: Bug 1449259: Don't do proxy lookups for loads that are only going to come from the cache.

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

Comment 5

[Honza Bambas (:mayhemer)]

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.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8965136 [details]
Bug 1449259: Don't do proxy lookups for loads that are only going to come from the cache.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9fb3e5aba482

Comment 9

[Victor Carciu]

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.