811669 - nsICachingChannel.cacheKey may not be set on http-on-modify-request anymore

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Georg Koppen]

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

[Patrick McManus [:mcmanus]]

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.

Comment 2

[Georg Koppen]

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

[Patrick McManus [:mcmanus]]

can you use the mechanism being developed in 800799?

Comment 4

[Georg Koppen]

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

[Patrick McManus [:mcmanus]]

whether or not this is something we want to support and how is something I'd like biesi to decide

Comment 6

[Patrick McManus [:mcmanus]]

whether or not this is something we want to support and how is something I'd like biesi to decide

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

We should try to support this. observers should be able to modify basically everything.

Comment 8

[Jason Duell]

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.

Comment 9

[Jason Duell]

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.

Comment 10

[Jason Duell]

Attachment: v1

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.

Comment 11

[Patrick McManus [: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()

Comment 12

[Jason Duell]

> 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 13

[Jason Duell]

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

Comment 14

[Alex Keybl [:akeybl]]

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?

Comment 15

[Jason Duell]

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

[Ed Morley [:emorley]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/391b9d2e45dc

Comment 18

[Jason Duell]

Getting this fixed on beta will be at the top of my list after my 2 B2G blockers are done.

Comment 19

[Georg Koppen]

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:[e-mail]/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.

Comment 20

[Georg Koppen]

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