1319951 - modify DOMStorageManager to put back the private browser boolean in the session storage API for now, since Origin Attributes aren't always right

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect, P1)

Description

[Dave Huseby [:huseby]]

In a previous changeset https://hg.mozilla.org/mozilla-central/rev/34acf7fe34f4 we removed the private browser boolean from the DOMStorageManager API.  While adding asserts to verify that the changes were correct (Bug 1316075) Ehsan discovered that the correct thing to do is to keep the private browser boolean in the DOMStorageManager API because it isn't always in sync with the origin attributes on the principal that is also passed to the DOMStorageManager API.  See: https://bgz.la/1316075#c17 for more details.

This bug is for tracking that work.

Comment 1

[Dave Huseby [:huseby]]

This bug fixes a mistake we made in the conversion away from the private browsing boolean.  The TL;DR is that we need to pass the boolean into the session storage API because it doesn't always match the origin attributes on the principal, but for good reasons.

Can you assign a priority on this one?  If it were me, I'd say P1 because it needs to get back ported all the way to beta.

Comment 2

[:ehsan akhgari]

Yes, we need to do this sooner than later.

Comment 3

[Tanvi Vyas[:tanvi]]

Yes, this is a P1.  Lets do it and uplift it where we need to.  Dave can you take this and work on it asap, or are you swamped?  If you are swamped, we need to find someone else to fix this asap.

Comment 4

[Dave Huseby [:huseby]]

Tanvi, now that I know the answer to 1309070, I can make this my next task.  I'll start in on it tomorrow.

Comment 5

[Tanvi Vyas[:tanvi]]

[Tracking Requested - why for this release]: Private Browsing Mode behavior was changed in https://hg.mozilla.org/mozilla-central/rev/34acf7fe34f4, bug 1283281 and now potentially does not properly segregate normal and private data in DomStorageManager.

Comment 6

[Gerry Chang [:gchang]]

Track 51+ as Private Browsing Mode behavior is not handled correctly.

Comment 8

[:ehsan akhgari]

Attachment: Partially revert the changes from bug 1283281 to handle the case where DOM Storage is accessed with the system principal exactly like before

Comment 10

[:ehsan akhgari]

ping?

Comment 11

[Andrea Marchesini [:baku]]

I'm still trying to understand why we need all of this. Why do we end up using the system principal with SessionStorage?
Ping me on IRC, so I can continue the review.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46225906354e
Partially revert the changes from bug 1283281 to handle the case where DOM Storage is accessed with the system principal exactly like before; r=baku

Comment 13

[:ehsan akhgari]

Comment on attachment 8820430 [details] [diff] [review]
Partially revert the changes from bug 1283281 to handle the case where DOM Storage is accessed with the system principal exactly like before

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1283281.
[User impact if declined]: See https://bgz.la/1316075#c17.  It's not immediately obvious to me that this has any meaningful impact for our users, but I prefer to be safe here.
[Is this code covered by automated tests?]:  Given the above, it's hard to say.  The DOM Storage code however is definitely tested.
[Has the fix been verified in Nightly?]: Just landed.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None (but I think I need to do manual uplifts due to some recent renaming in this code.)
[Is the change risky?]: No
[Why is the change risky/not risky?]: It reverts us to our behavior before bug 1283281.
[String changes made/needed]: None.

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/46225906354e

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 8820430 [details] [diff] [review]
Partially revert the changes from bug 1283281 to handle the case where DOM Storage is accessed with the system principal exactly like before

go back to using private browsing flag for dom storage, beta52+

Clearing aurora approval flag since we're past merge day and are about to release 51.

Comment 16

[Carsten Book [:Tomcat]]

needs rebasing for beta 

warning: conflicts while merging dom/base/nsGlobalWindow.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/storage/DOMStorage.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/storage/DOMStorage.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/storage/DOMStorageCache.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/storage/DOMStorageManager.cpp! (edit, then use 'hg resolve --mark')

Comment 17

[:ehsan akhgari]

https://hg.mozilla.org/releases/mozilla-beta/rev/11198ed324d9caec967600d469a63d96f971c040

Comment 18

[Andrei Vaida [:avaida]]

Setting qe-verify- per Comment 13.

Comment 19

[Tanvi Vyas[:tanvi]]

Do we need another bug to add back the asserts and fix the issues with Origin Attributes?  And then a third bug to actually do the conversion of DOMStorageManager once the mismatches are fixed?

Comment 20

[:ehsan akhgari]

(In reply to Tanvi Vyas - not reading bugmail, PM me [:tanvi] from comment #19)
> Do we need another bug to add back the asserts and fix the issues with
> Origin Attributes?  And then a third bug to actually do the conversion of
> DOMStorageManager once the mismatches are fixed?

No.  The point behind this bug was that the removal of the boolean flag and the assertion added before were both the wrong thing to do.

Comment 21

[Tanvi Vyas[:tanvi]]

(In reply to :Ehsan Akhgari from comment #20)
> (In reply to Tanvi Vyas - not reading bugmail, PM me [:tanvi] from comment
> #19)
> > Do we need another bug to add back the asserts and fix the issues with
> > Origin Attributes?  And then a third bug to actually do the conversion of
> > DOMStorageManager once the mismatches are fixed?
> 
> No.  The point behind this bug was that the removal of the boolean flag and
> the assertion added before were both the wrong thing to do.

Okay, so that means we are sticking with the boolean here and will not convert to Origin Attributes?  What happens when we have multiple private modes?  OriginAttribute.privateBrowsingMode = 1, 2, 3, etc.  Should we worry about this then and just leave the boolean for now?

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Tanvi Vyas - not reading bugmail, PM me [:tanvi] from comment #21)
> Okay, so that means we are sticking with the boolean here and will not
> convert to Origin Attributes?  What happens when we have multiple private
> modes?  OriginAttribute.privateBrowsingMode = 1, 2, 3, etc.  Should we worry
> about this then and just leave the boolean for now?

My understanding of this bug is that we need to track private-browsing separately from the OriginAttributes exclusively because in chrome docshells the system principal can never end up with a private browsing id even though it's operating in a private browsing context.  For normal content principals localStorage's use of nsIPrincipal::equals can tell privateBrowsingId's 1 and 2 apart.

If I'm right about that, then the issue only arises if there's chrome-principal'ed code operating in a private browsing context using localStorage (which seems like a bad idea) where there's a semantic importance to different browsing contexts.  If I'm wrong about that, we need to revisit my de-bitrotting of multi-e10s localstorage support in bug 1285898 comment 20.

Comment 23

[:ehsan akhgari]

(In reply to Tanvi Vyas - not reading bugmail, PM me [:tanvi] from comment #21)
> (In reply to :Ehsan Akhgari from comment #20)
> > (In reply to Tanvi Vyas - not reading bugmail, PM me [:tanvi] from comment
> > #19)
> > > Do we need another bug to add back the asserts and fix the issues with
> > > Origin Attributes?  And then a third bug to actually do the conversion of
> > > DOMStorageManager once the mismatches are fixed?
> > 
> > No.  The point behind this bug was that the removal of the boolean flag and
> > the assertion added before were both the wrong thing to do.
> 
> Okay, so that means we are sticking with the boolean here and will not
> convert to Origin Attributes?

Yes.

> What happens when we have multiple private
> modes?  OriginAttribute.privateBrowsingMode = 1, 2, 3, etc.

We would add the privateBrowsingMode value to the partitioning we currently do for cached storage data (basically we'd need to extend StorageCache::mData).  The isolation should still work fine due to what Andrew said in comment 22.

> Should we worry
> about this then and just leave the boolean for now?

We should worry about the multi-PB-ID case later.  As things stand, we have tons of bugs in our code which we'd need to fix to support that.

(In reply to Andrew Sutherland [:asuth] from comment #22)
> My understanding of this bug is that we need to track private-browsing
> separately from the OriginAttributes exclusively because in chrome docshells
> the system principal can never end up with a private browsing id even though
> it's operating in a private browsing context.  For normal content principals
> localStorage's use of nsIPrincipal::equals can tell privateBrowsingId's 1
> and 2 apart.

Right.  But also see the above point about caching.

> If I'm right about that, then the issue only arises if there's
> chrome-principal'ed code operating in a private browsing context using
> localStorage (which seems like a bad idea)

Why?  I'm pretty sure we've had things like about:home use it.  Also add-ons.

> where there's a semantic
> importance to different browsing contexts.  If I'm wrong about that, we need
> to revisit my de-bitrotting of multi-e10s localstorage support in bug
> 1285898 comment 20.

I didn't really follow the grand plans in that bug, but the patch there looks fine to me.  That being said, I'm not able to parse what you mean here...

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

(In reply to :Ehsan Akhgari from comment #23)
> > If I'm right about that, then the issue only arises if there's
> > chrome-principal'ed code operating in a private browsing context using
> > localStorage (which seems like a bad idea)
> 
> Why?  I'm pretty sure we've had things like about:home use it.  Also add-ons.

localStorage under private browsing has poor persistence guarantees and they just got worse.  Data evaporates when there are no more nsGlobalWindow-owned Storage instances around and no 20-second-timer KeepAlive instance around.  And the KeepAlive mechanism had to be removed for e10s because change broadcasts are tied to nsGlobalWindows, not to StorageCache instances.  Which means anything that kept a StorageCache alive without having a window to apply changes to it would result in loss of coherency between processes.  (The precache mechanism, important for avoiding blocking the main thread, has been kept operational by adjusting it to cause the window to save the Storage instance to mLocalStorage if precaching is appropriate.)  See bug 1285898 for more context.

The good news is browser/'s about:home only uses IndexedDB and mobile/android/'s about:home is Java powered and doesn't touch it either.

I'm about to file/link follow-ups to bug 1285898 and I'll make sure to include one that covers how private-browsing localstorage is wonky so people can be aware without having to grok the codebase.

Comment 25

[:ehsan akhgari]

OK I understand now.  Thanks in advance for following up on this.