Closed
Bug 1449259
Opened 6 years ago
Closed 6 years ago
Having devtools open can cause proxy requests with unknown tabId and other content policy type
Categories
(WebExtensions :: Request Handling, enhancement, P3)
WebExtensions
Request Handling
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.
Assignee | ||
Comment 1•6 years ago
|
||
(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)
Updated•6 years ago
|
Component: WebExtensions: General → WebExtensions: Request Handling
Assignee | ||
Comment 2•6 years ago
|
||
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•6 years ago
|
||
I think that sounds fine, but Honza probably knows better than I do.
Flags: needinfo?(jduell.mcbugs) → needinfo?(honzab.moz)
Comment hidden (mozreview-request) |
Comment 5•6 years 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) |
Updated•6 years ago
|
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
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(honzab.moz)
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9fb3e5aba482
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 9•6 years 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•6 years ago
|
Flags: needinfo?(dtownsend)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(dtownsend) → qe-verify-
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•