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

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.)

MozReview-Commit-ID: B1W0EUhMEyJ

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 ;).

(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.