806753 - e10s: Remove already broken support for child-side http-on-* observers

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

As mentioned in bug 558624, comment 6, we already no longer allow setting/modifying cookies in http-on-modify request observers in child processes. (supporting this correctly requires doing an IPDL round-trip to fetch all the cookies from the parent during asyncOpen so the observer can see/modify them: we saved 5% performance by not doing this any more). 

In bug 805616 we've noticed that we're setting cookies twice when set by a server, once on the parent and then on the child after we call http-on-examine observers.  This is broken--if a child-side 'examine' observer tries to prevent a cookie from being set (by deleting the set-cookie header) it will fail because the cookie has already been set on the parent.  Supporting this would also require an extra IPDL round trip (to call on-examine on the child, then ship any header mods back to parent before we actually set cookies and call OnStartRequest).

Seems like we might as well just get rid of these, rather than support broken functionality.

I've stuck a break point into the debugger and run B2G desktop, and I don't see any uses of these notifications on the child, so I'm assuming we're ok to remove.  If anyone know of cases where we need them, please let me know.

Code audit:

on-modify-request:

Fabrice has confirmed that this code runs only the child:

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/UserAgentOverrides.jsm#37

And it looks like the only callers of "XML_Fetcher" are also parent-side:

   http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/xml-fetcher.js#153

on-examine-response:  I assume NetworkMonitor is not running on child processes:

  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/WebConsoleUtils.jsm#2068

http-on-examine-cached-response and http-on-examine-merged-response:

We are already missing support for calling these on the child.

Comment 1

[Jason Duell]

Attachment: v1

I'm guessing bz is the most likely to know if this is a bad idea.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Have we contacted any of the addon authors who are known to use this stuff (tor, noscript, firebug, maybe adblock plus, maybe grep addons to find others) and asked them whether it would break them?

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Have we contacted any of the addon authors who are known to use this stuff
> (tor, noscript, firebug, maybe adblock plus, maybe grep addons to find
> others) and asked them whether it would break them?

A good implementation of parent/child process sandboxing requires that do not ever send HttpOnly cookies to the child process. That effectively means addons that need to change cookies need to do so in the parent process. So, while I definitely do think we should help addon authors adjust to this change, I think it is such an important change to make that we should definitely make it soon, especially if there is time to implement proper sandboxing support for HttpOnly cookies for B2G.

Comment 4

[Boris Zbarsky [:bzbarsky]]

This notification is used for all sorts of things completely unrelated to cookies in addons.  So I'm not sure why we're focusing on cookies.

And I agree that addons might want to watch for this notification in the parent process... but note that at least for Firebug that would totally break their break-on-XHR setup if it were not already broken by 797684.  But that's a simple example of an addon use of these notifications that's not cookie-related....

In any case, the only bad idea part of this is the compat hit, whatever that is.  Obviously it's not a problem for _us_ this change is not a problem!

Comment 5

[Jason Duell]

Addons aren't used at all in B2G, and AFIAK the only other e10s usage are plugin-processes, which I assume aren't using these notifications?  So I'd think nothing is using this code right now.  If so, we have a choice between:

1) support the APIs as we do now, and silently break the (documented) use cases for blocking removing cookies.  We could always document that they don't work in the child and (optimistically) assume people read the docs.

2) explicitly break code that tries to add observers for these events (as this patch does) so they know something's amiss.  And then we can revisit whether to enable these (and/or whether to truly fix them) when we see the uses case(s).

bz: I don't have a strong opinion either way.  What do you think?

> Supporting on-examine-request would also require an extra IPDL round trip (to
> call on-examine on the child, then ship any header mods back to parent before
> we actually set cookies and call OnStartRequest).

Actually, I think we could get away with 1) calling on-examine observers in the child, then 2) skipping setCookie on the parent; then 3) calling OnStartRequest otherwise normally, and 4) call on-examine in the child and then call SetCookie after that.   This would require an extra IPDL message for the child-side SetCookie, but an async one, which is relatively cheap.  This works as long as nothing in the parent requires the cookie database to be updated during OnStartRequest, which ought to be fine (it's certainly fine with existing code: addons could add traceableListeners that would get the OnStartRequest, but they're unlikely to be issue GetCookie requests).

If I'm right about that, then it's only on-modify-request that's expensive to support properly.

Comment 6

[Jason Duell]

s/blocking removing cookies/blocking or modifying cookies/.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, right.  I forgot that fennec is no longer using e10s.

Given that, this patch sounds fine to me in general.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 676482 [details] [diff] [review]
v1

r=me with the bug numbers fixed.

Comment 9

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4525ca87c142

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4525ca87c142