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)

18 Branch
x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox18 --- affected
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: gk, Assigned: jduell.mcbugs)

References

Details

Attachments

(1 file)

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).
Blocks: 797684
OS: Windows 7 → All
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.
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.
can you use the mechanism being developed in 800799?
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.
whether or not this is something we want to support and how is something I'd like biesi to decide
Flags: needinfo?(cbiesinger)
whether or not this is something we want to support and how is something I'd like biesi to decide
We should try to support this. observers should be able to modify basically everything.
Flags: needinfo?(cbiesinger)
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.
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.
Attached patch v1 (deleted) — Splinter Review
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 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+
> 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
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 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+
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.
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Getting this fixed on beta will be at the top of my list after my 2 B2G blockers are done.
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 → ---
(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 ago12 years ago
Resolution: --- → WORKSFORME
Resolution: WORKSFORME → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: