Bug 1202634 - [e10s] links with target=_blank in private browsing popup open in non-private window

Yikes, this is bad. We should get a test written for this!

I need to better understand how private browsing is applied. It seems to have to do with load contexts. And we've got load contexts in several places:

/**
* Class that provides nsILoadContext info in Parent process.  Typically copied
* from Child via SerializedLoadContext.
*
* Note: this is not the "normal" or "original" nsILoadContext.  That is
* typically provided by nsDocShell.  This is only used when the original
* docshell is in a different process and we need to copy certain values from
* it.
*
* Note: we also generate a new nsILoadContext using LoadContext(uint32_t aAppId)
* to separate the safebrowsing cookie.

*/

Interesting… so we have a special LoadContext  for multi-process.

00000011011001110

10000000000000000

const unsigned long CHROME_PRIVATE_WINDOW         = 0x00010000;

So, two cases - window was opened from content (via TabParent::RecvCreateWindow), or window was opened from parent (ContentParent::CreateBrowserOrApp).

I think we actually only care about enforcement in the former - because the user can choose to open windows of whatever type from the PARENT process. It's the requests from the content process that we have to police.

Back in bug 1114299, I made it so that we don't accept the private browsing or remoteness chrome flags from content, because it appeared as if the parent was doing the right thing and figuring out whether or not the opened window from content should be private, or remote, or both.

What I didn't realize is that TabParent's get their own special nsILoadContext's, and that the construction of these nsILoadContext's rely on the chromeFlags that are passed into the TabParent constructor. When those TabParent's are created from content for a pop-up window, because we don't pass up the remoteness/private browsing chrome flags, the TabParent LoadContext gets the wrong values. This appears to be the root of bug 1202634.

I don't know if it's wise to relax the restrictions, and allow the remote / private browsing flags from content again. If we want to keep that restriction, that means that I'm going to need a way for the TabParent constructor to understand that content has requested a new window should be opened, and for the TabParent constructor to somehow get the appropriate flags from the parent process. If that's the approach, it's not clear to me where to do that.

Here's where we send the message to construct the TabParent from TabChild: https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/dom/ipc/TabChild.cpp#1181

Would it be advisable to have nsIContentParent somehow realize that this is a PopupIPCTabContext, and use the information from that context to set the right private browsing / remoteness flags on the created TabParent in nsIContentParent::AllocPBrowserParent?

I'm only mildly confident about this. :/ I've feedback?'d billm on my patch.

I've written a regression test! And tested that it indeed fails without my patch. Phew.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f49da4f23e9

So billm's given this r+ on this, but I want to make sure I understand how the top-level nsDocShell in a newly created popup knows to be in private browsing mode...

Ahhhh - here's the relevant code:

// Otherwise, propagate the privacy status of the parent window, if
// available, to the child.
if ( ! isPrivateBrowsingWindow &&
! (chromeFlags & nsIWebBrowserChrome :: CHROME_NON_PRIVATE_WINDOW)) {
nsCOMPtr < nsIDocShellTreeItem > parentItem;
GetWindowTreeItem(aParent, getter_AddRefs(parentItem));
nsCOMPtr < nsILoadContext > parentContext = do_QueryInterface(parentItem);
if (parentContext) {
isPrivateBrowsingWindow = parentContext -> UsePrivateBrowsing();
}
}

// We rely on CalculateChromeFlags to decide whether remote (out-of-process)
// tabs should be used.
bool isRemoteWindow =
!! (chromeFlags & nsIWebBrowserChrome :: CHROME_REMOTE_WINDOW);

if (isNewToplevelWindow) {
nsCOMPtr < nsIDocShellTreeItem > childRoot;
newDocShellItem -> GetRootTreeItem(getter_AddRefs(childRoot));
nsCOMPtr < nsILoadContext > childContext = do_QueryInterface(childRoot);
if (childContext) {
childContext -> SetPrivateBrowsing(isPrivateBrowsingWindow);
childContext -> SetRemoteTabs(isRemoteWindow);
}
} else if (windowIsNew) {
nsCOMPtr < nsILoadContext > childContext = do_QueryInterface(newDocShellItem);
if (childContext) {
childContext -> SetPrivateBrowsing(isPrivateBrowsingWindow);
childContext -> SetRemoteTabs(isRemoteWindow);
}

}

Okay, that makes me feel a bit better (though this whole new-window-opening thing is really, really, really complex).

Let's land this puppy.