Closed Bug 1319951 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Private Browsing, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox51 + wontfix
firefox52 + fixed
firefox53 + fixed

People

(Reporter: huseby, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

(Whiteboard: [OA])

Crash Data

Attachments

(1 file)

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.
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.
Flags: needinfo?(tanvi)
Yes, we need to do this sooner than later.
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.
Flags: needinfo?(tanvi) → needinfo?(huseby)
Priority: -- → P1
Summary: modify DOMStorageManager to put back the private browser boolean in the session storage API → modify DOMStorageManager to put back the private browser boolean in the session storage API for now, since Origin Attributes aren't always right
Tanvi, now that I know the answer to 1309070, I can make this my next task.  I'll start in on it tomorrow.
Flags: needinfo?(huseby)
Assignee: nobody → huseby
Status: NEW → ASSIGNED
Blocks: 1323262
[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.
Track 51+ as Private Browsing Mode behavior is not handled correctly.
Duplicate of this bug: 1322760
Assignee: huseby → ehsan
Duplicate of this bug: 1325514
Crash Signature: [@ nsGlobalWindow::GetSessionStorage ]
ping?
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
Attachment #8820430 - Flags: review?(amarchesini) → review+
Pushed by eakhgari@mozilla.com:
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 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.
Attachment #8820430 - Flags: approval-mozilla-beta?
Attachment #8820430 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/46225906354e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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.
Attachment #8820430 - Flags: approval-mozilla-beta?
Attachment #8820430 - Flags: approval-mozilla-beta+
Attachment #8820430 - Flags: approval-mozilla-aurora?
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')
Flags: needinfo?(ehsan)
Setting qe-verify- per Comment 13.
Flags: qe-verify-
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?
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
(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?
Flags: needinfo?(ehsan)
(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.
(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...
Flags: needinfo?(ehsan)
(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.
OK I understand now.  Thanks in advance for following up on this.
You need to log in before you can comment on or make changes to this bug.