1088457 - [e10s] probably can't get docshell from nsChannelClassifier

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

nsChannelClassifier sends an event to the docshell's nsISecurityEventSink in order to signal that the shield icon should show up. I'm guessing that's going to be a problem.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Ugh. :jimm says that there's not a great way to get to the docshell from the parent (related bug:https://bugzil.la/1073957), and tanvi is having similar problems in https://bugzilla.mozilla.org/show_bug.cgi?id=1084504. The affected function is nsChannelClassifier::SetBlockedTrackingContent. Instead of running this from necko I will most probably have to move this logic into the callers of NS_NewChannel that may be affected by tracking protection.

These were modified in https://bugzilla.mozilla.org/show_bug.cgi?id=1044181 and https://bugzilla.mozilla.org/show_bug.cgi?id=1029887 and include all loaders that listen for NS_ERROR_TRACKING_URI: https://mxr.mozilla.org/mozilla-central/search?string=NS_ERROR_TRACKING_URI

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

I can think of two ways to fix this (as I understand the problem):

  * Connect PHttpChannel to PBrowsers in order to allow code in the parent to figure out which frameloader (which exists in the parent) is responsible for the load. As I understand it, that should be enough for Monica's use-case here. Right now channels are tied to PContent objects, which isn't terribly useful. Jason, do you think that would work?

  * The nsChannelClassifier can send a message via IPDL from the parent to the child via the HttpChannel that it's flagging. The child, upon receiving the message could then send a message up to the parent via the docshell. We could even reuse the nsISecurityEventSink machinery in the child with a JS implementation to then communicate directly with browser.js in the parent.

I don't know if there's anything that absolutely has to be done in the child so if not, it'd be nice to save the round-trip and stay in the parent with the first option.

Comment 3

[Jason Duell]

It would be great to know which PBrowser a channel is associated with. We pass in a PBrowser in all of the channel constructors in PNecko.ipdl, but we don't keep it around.  We could stash it, or maybe there's an IPDL way to link them that I don't know about?

One wrinkle: make sure you handle redirects correctly.  In a nutshell, redirects work by 

1) the new nsHttpChannel (or nsFTPChannel) gets created in the parent. 

2) On the parent we wind up keeping track of the new channel with a hashtable that maps a "channel ID" to new, redirected-to channel.   

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParentListener.cpp#143

3) We then create a new IPDL channel for the new channel (i.e. the HttpChannelChild that will map to the new redirected-to channel on the parent) and it winds up using the "channel ID" to find the channel on the parent.  

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#922

In this case the IPDL constructor does not currently pass a PBrowser as part of the IPDL args, just the "channel ID".  See

   http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/PNecko.ipdl#36

We could change that so that those requests also pass a PBrowser, or we could have the original parent class pass it to the redirected-to parent class.  Or as mentioned maybe there's some IPDL way to do all this.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wip

This seems to work. I'll post more about it when I've tested it more.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Fabrice, I believe that my wip patch should work equally well on b2g as desktop Firefox.

Comment 6

[[:fabrice] Fabrice Desré]

(In reply to Blake Kaplan (:mrbkap) from comment #5)
> Fabrice, I believe that my wip patch should work equally well on b2g as
> desktop Firefox.

Cool, I'll test. Is there any log I can turn on to check if things get blocked?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This version adds some additional comments and assertions. I found that the channel classification stuff was running in both the parent and the child, so I fixed that.

This patch went through about 5 designs. I was hoping to keep everything in the parent process, but it was hard to find the nsISecurityEventSink impl from the parent. Doing so indirectly would have been possible (as in desktop Firefox it's the browser element that owns the message manager), but that would have made the patch Firefox specific and, even with that, we would have had trouble with iframes and nested content processes. Furthermore, it was going to be hard, if not impossible, to get ahold of the SecureUIImpl in order to get the existing security state flags. This patch responds to all of the tracking protection events in the child (where we have all the information we need and can easily send messages to the right object in the parent) but does the actual checking in the parent. Eventually, it would be good to avoid the round-trip to the child for security stuff, but for the moment, this should be fine.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Oh, with this patch, browser_trackingUI.js passes but leaks. I'm debugging the leak now.

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8520041 [details] [diff] [review]
patch v1

Review of attachment 8520041 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, but I am not a necko peer. Thanks for fixing this. The assertions about child/parent make the e10s case a lot clearer. Does this mean that attaching PBrowser to the channel is out of scope?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8520041 [details] [diff] [review]
patch v1

I think I also need a Necko peer review.

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8520041 [details] [diff] [review]
patch v1

Review of attachment 8520041 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with one minor change.

I don't think Jason needs to look at this; canceling his review.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +826,5 @@
>  
>  NS_IMETHODIMP
> +HttpChannelParent::NotifyTrackingProtectionDisabled()
> +{
> +  unused << SendNotifyTrackingProtectionDisabled();

Wrap this with "if (!mIPCClosed)".

Comment 12

[Jason Duell]

Thanks Steve!

Comment 13

[Jason Duell]

> nsCOMPtr<nsIDOMWindow> win;
> nsCOMPtr<mozIThirdPartyUtil> thirdPartyUtil =
>   do_GetService(THIRDPARTYUTIL_CONTRACTID, &rv);
> NS_ENSURE_SUCCESS(rv, NS_OK);
> rv = thirdPartyUtil->GetTopWindowForChannel(channel, getter_AddRefs(win));
	
So we used to pass mSuspendedChannel here instead of channel. I assume they generally (or always) point to the same channel and so this is just cleanup/bikeshedding?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Jason Duell [:jduell] (needinfo? me for lower latency) from comment #13)
> So we used to pass mSuspendedChannel here instead of channel. I assume they
> generally (or always) point to the same channel and so this is just
> cleanup/bikeshedding?

There was only one caller of this function before and it always passed mSuspendedChannel. I made this function static, so it could no longer use member variables and had to be changed to use its parameter in all cases.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/747bd9334e1c

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/747bd9334e1c
https://hg.mozilla.org/mozilla-central/rev/9acd143ca8a6