Closed
Bug 811669
Opened 12 years ago
Closed 12 years ago
nsICachingChannel.cacheKey may not be set on http-on-modify-request anymore
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: gk, Assigned: jduell.mcbugs)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mcmanus
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20100101 Firefox/10.0
Steps to reproduce:
I try to segment the cache according to the domain of the originating page, i.e. bind third party resources to the first party domain, in order to avoid this user tracking vector. See: http://safecache.com/ for the underlying idea. That works currently by setting the cacheKey accordingly.
Actual results:
Resolving bug 797684 breaks this. I get:
###!!! ABORT: 'SetCacheKey' called after AsyncOpen: ../../../../netwerk/protocol/http/nsHttpChannel.cpp +5333 (set NECKO_ERRORS_ARE_FATAL=0 in your environment to convert this error into a warning.): file ../../../../netwerk/protocol/http/nsHttpChannel.cpp, line 5333
Expected results:
It should still be possible to set the cacheKey from extension land (probably via http-on-modify-request observers).
Comment 1•12 years ago
|
||
I'm not quite sure how to fix your problem, but as background: http-on-modify-request is no longer called synchronously out of asyncopen (actually it never was 100% of the time) because it needs to have the proxy information available at that time and determining the proxy info is now an asynchronous operation. This change was a large jank reducer.
Reporter | ||
Comment 2•12 years ago
|
||
To be clear: I don't necessarily want to set the cacheKey on http-on-modify-request. If it would be/is possible somewhere else from extension land I would be/am fine.
Comment 3•12 years ago
|
||
can you use the mechanism being developed in 800799?
Reporter | ||
Comment 4•12 years ago
|
||
I'd say, no, as that new notification is only broadcasted if a XHR is about to get sent. And XHR's are only a fraction of all HTTP requests.
Comment 5•12 years ago
|
||
whether or not this is something we want to support and how is something I'd like biesi to decide
Flags: needinfo?(cbiesinger)
Comment 6•12 years ago
|
||
whether or not this is something we want to support and how is something I'd like biesi to decide
Comment 7•12 years ago
|
||
We should try to support this. observers should be able to modify basically everything.
Flags: needinfo?(cbiesinger)
Assignee | ||
Comment 8•12 years ago
|
||
How about some new http-on-XXX-request call (that gets called synchronously during asyncOpen, but w/o proxy info avail)?
See bug 800799 too.
Assignee | ||
Comment 9•12 years ago
|
||
Actually, after discussing on IRC patrick and I realized that it's still quite possible for the cacheKey to be set here--we're just artificially failing it if AsyncOpen has completed. I'm writing a patch to change that check so it still allows setting during OMR.
Assignee | ||
Comment 10•12 years ago
|
||
I audited all the call that we were protecting with ENSURE_CALLED_BEFORE_ASYNC_OPEN. All of them except HttpBaseChannel::SetForceAllowThirdPartyCookie are fine being set during our new, later OMR time.
Assignee: nobody → jduell.mcbugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #685395 -
Flags: review?(mcmanus)
Comment 11•12 years ago
|
||
Comment on attachment 685395 [details] [diff] [review]
v1
Review of attachment 685395 [details] [diff] [review]:
-----------------------------------------------------------------
you might want to rename requestobserverscalled now that you've added the new one that is called out of asyncOpen()
Attachment #685395 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 12•12 years ago
|
||
> you might want to rename requestobserverscalled
sigh--but I named it that precisely with the idea in mind that there are now more than 1 request observer types :P Not sure of a better name (ideas?). For now just added a comment to .h declaration that makes it explicit that it signifies that all http-on-***-request observers have been called.
https://hg.mozilla.org/integration/mozilla-inbound/rev/391b9d2e45dc
Assignee | ||
Comment 13•12 years ago
|
||
Comment on attachment 685395 [details] [diff] [review]
v1
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 797684
User impact if declined: a large set of HTTP channel functions will be broken for no good reason for addons that try to call them during http-on-modify-request. We don't have a lot of complaints yet, but I would bet on more bustage if we don't fix this.
Risk to taking this patch (and alternatives if risky): low.
String or UUID changes made by this patch: none
Attachment #685395 -
Flags: approval-mozilla-beta?
Attachment #685395 -
Flags: approval-mozilla-aurora?
Comment 14•12 years ago
|
||
Comment on attachment 685395 [details] [diff] [review]
v1
Given this is a regression from a regression fix, we really need to figure out how to stop the madness. What can you test to make doubly sure we've resolved these issues entirely?
Attachment #685395 -
Flags: approval-mozilla-beta?
Attachment #685395 -
Flags: approval-mozilla-beta+
Attachment #685395 -
Flags: approval-mozilla-aurora?
Attachment #685395 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/3897f5792c0b
https://hg.mozilla.org/releases/mozilla-beta/rev/b4df4a2aee3e
> What can you test to make doubly sure we've resolved these issues entirely?
We could have detected these if we had better function call coverage within OMR observers--filed bug 818163 for that.
Comment 16•12 years ago
|
||
Backed out of beta for:
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit_ipc/test_httpcancel_wrap.js | test failed (with xpcshell return code: 0), see following log:
child: TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_httpcancel.js | Error should have been thrown before getting here - See following stack:
..etc
Beta-backout:
https://hg.mozilla.org/releases/mozilla-beta/rev/985265a08b5b
Comment 17•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Assignee | ||
Comment 18•12 years ago
|
||
Getting this fixed on beta will be at the top of my list after my 2 B2G blockers are done.
Updated•12 years ago
|
Reporter | ||
Comment 19•12 years ago
|
||
Almost all issues are fixed by your patch, thanks. But at least one remains as I still get exceptions (occasionally):
[Exception... "Component returned failure code: 0x804b0049 (NS_ERROR_ALREADY_OPENED) [nsICachingChannel.cacheKey]" nsresult: "0x804b0049 (NS_ERROR_ALREADY_OPENED)" location: "JS frame :: resource://jondofox/safeCache.jsm :: safeCache.bypassCache :: line 253" data: no]
It seems that all these exceptions belong to 3rd party URLs like
http://Session:401631783@ipcheck.info/auth.css.php (probably loaded in an Iframe).
If that helps, fine. If not I try to come up with a minimal extension that demonstrates that issue and makes it easier reproducible for you.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 20•12 years ago
|
||
(In reply to Georg Koppen from comment #19)
> [Exception... "Component returned failure code: 0x804b0049
> (NS_ERROR_ALREADY_OPENED) [nsICachingChannel.cacheKey]" nsresult:
> "0x804b0049 (NS_ERROR_ALREADY_OPENED)" location: "JS frame ::
> resource://jondofox/safeCache.jsm :: safeCache.bypassCache :: line 253"
> data: no]
While preparing a minimal testcase I realized that this issue has nothing to to with your patch as this affects Firefox 17 as well. I'll need to do a bit more research to understand what is going on. Resetting this bug to RESOLVED WORKSFORME as I am not able to set it back to RESOLVED FIXED. Sorry for the noise.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → WORKSFORME
Updated•12 years ago
|
Resolution: WORKSFORME → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•