1439750 - Move private browsing tabs out of web content processes

Status: NEW

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Nika Layzell [:nika] (ni? for response)]

This patch adds a new remote type for private browsing which ensures that private browsing tabs are never loaded in the same process as other web content.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Move private browsing tabs into their own remote type

I'm just attaching this for now. I haven't written a test for this yet, which I should probably get around to.

MozReview-Commit-ID: KDtGN64bZnS

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8952540 [details] [diff] [review]
Part 1: Move private browsing tabs into their own remote type

I did not mean to flag you for review yet ^_^ - oops

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Nika Layzell [:mystor] from comment #2)
> Comment on attachment 8952540 [details] [diff] [review]
> Part 1: Move private browsing tabs into their own remote type
> 
> I did not mean to flag you for review yet ^_^ - oops

That's cool - hope it's okay that I've already started looking at the patch. :) I suggest considering putting this logic inside E10SUtils.getRemoteTypeForURI, and pass the window as an argument to check for private state, just to centralize a little. How does that sound?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #3)
> (In reply to Nika Layzell [:mystor] from comment #2)
> > Comment on attachment 8952540 [details] [diff] [review]
> > Part 1: Move private browsing tabs into their own remote type
> > 
> > I did not mean to flag you for review yet ^_^ - oops
> 
> That's cool - hope it's okay that I've already started looking at the patch.
> :) I suggest considering putting this logic inside
> E10SUtils.getRemoteTypeForURI, and pass the window as an argument to check
> for private state, just to centralize a little. How does that sound?

Are you OK with leaving it outside of getRemoteTypeForURI? Moving it into that code is actually more involved (as WEB_REMOTE_TYPE is returned in many places), and for most purposes the distinction between webRemoteType and privateRemoteType isn't important.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Nika Layzell [:mystor] from comment #4)
> Are you OK with leaving it outside of getRemoteTypeForURI? Moving it into
> that code is actually more involved (as WEB_REMOTE_TYPE is returned in many
> places), and for most purposes the distinction between webRemoteType and
> privateRemoteType isn't important.

Hm. Maybe I'm confused here - I guess I want to make absolutely sure that we're covering all cases where we assign a <browser> to a particular process type. It would be unfortunate if sometimes a <browser> inside a private browsing window doesn't actually get assigned to this new process type because we forgot some weird edge-case.

Is my anxiety unfounded?

It seems like the process type is actually calculated here: https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/toolkit/modules/E10SUtils.jsm#45-79

In each place where we're about to return WEB_REMOTE_TYPE, should we not ensure first that the window is not private?

Or am I kinda misunderstanding what we're doing here?

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Unassigning from myself as I don't have time to work on this right now.