1405839 - Don't set LocalStorage on the outer window

Status: NEW

(Core :: Storage: localStorage & sessionStorage, enhancement, P2)

Description

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

Right now we sometimes set mLocalStorage on the outer window rather than the inner window. This is unfortunate, as the field is an inner-window-only field.

http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/dom/base/nsGlobalWindow.cpp#3420

Comment 1

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

For context, this was an oversight/bug from the changes in bug 1285898 where a LocalStorageCache instance could no longer be safely held alive independent of an (*inner*) nsGlobalWindow and its owning LocalStorage instance.  Unfortunately, I completely missed that nsGlobalWindow::PreloadLocalStorage() was being invoked on the outer window, not the inner window.

Surprisingly, I don't think the fallout from this is so bad.  Namely:
- nsGlobalWindow::PreloadLocalStorage() is only conditionally gated on private-browsing-ness, which should hold constant for any given outerwindow.  So we should be overwriting mLocalStorage with null or an actual LocalStorage instance in all cases.
- When IPC messages are received, because we don't do any filtering (phew!), the inner-window will invoke GetLocalStorage(), maintaining the desired correctness invariant.
- The preload/precache goal actually should still work.

However, we obviously do want to be setting this only on the inner window, so we should absolutely make the change.  (Noting that :janv's LocalStorage overhaul will probably moot this specific code-path.)

Comment 2

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

Attachment: Don't set LocalStorage on the outer window

MozReview-Commit-ID: B1W0EUhMEyJ

Comment 3

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

Comment on attachment 8930746 [details] [diff] [review]
Don't set LocalStorage on the outer window

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

Thanks!  (And hooray for the split overall!)

I realized browser_localstorage_e10s has coverage for verifying the preload case already, so we should be good (unless that test starts failing ;).

Comment 4

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

(In reply to Andrew Sutherland [:asuth] from comment #3)
> I realized browser_localstorage_e10s has coverage for verifying the preload
> case already, so we should be good (unless that test starts failing ;).

Yeah, this patch seems to have totally burned treeherder. I'm going to put it on hold until I've landed the other patches in the split I think.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Nika, could you have a look please?

Comment 6

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

Moving to Web Storage. This is something we can absolutely remove one LSNG is permanently on, as it uses a different preload path, but we also should try and get rid of even if LSNG doesn't ride the train. I'm going to make this depend on the LSNG initial pref-flip bug 1517090 and the really-flip-on bug 1539835 so we don't lose it. (The previous depends on was for 'regression'-style tracking which will soon have its own field, but people can see comment 1 if they care for now.)