1173573 - Fix mysterious crash seen locally dealing with sessionstorage

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

I have to admit that I'm not entirely sure that my patch for bug 1151840 isn't implicated here, but I saw a couple of crashes in a local debug build stemming from a child process's *session* storage manager being initialized before its *local* storage manager. This matters because DOMSessionStorageManager's constructor calls DOMStorageCache::StartDatabase, which calls DOMLocalStorageManager::Self in order to create a DOMStorageDBChild.

My STR were:
1. Start Firefox (opens a non-privatebrowsing window).
2. Open a privatebrowsing window.
3. Navigate to google.com.
4. Crash.

It appears that most of the time, we create the DOMLocalStorageManager early on due to nsGlobalWindow::PreloadLocalStorage (which is called as soon as we load the first non-chrome page in a process). However, it seems like there might be a race if we run a sessionstore collection (the offending code appears to be <http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStorage.jsm?rev=85402fd3e3b4#142>) first.

I have a patch that fixes this.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible patch

Honza, I think you're the right person to review this. Let me know if that's not the case.

This simply fixes the bug by making sure we initialize both storage managers in the proper order when we need to.

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8620640 [details] [diff] [review]
Possible patch

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

Very good catch!

::: dom/storage/DOMStorageManager.cpp
@@ +644,1 @@
>      DOMStorageCache::StartDatabase();

problem here is that if anyone called StartDatabase() sooner it will still be initialized w/o ls-m (if it were not instantiated till now)

hence, this do_GetService should move inside StartDatabase() or (preferred) there should be DOMLocalStorageManager* DOMLocalStorageManager::Ensure() that would make sure the manager service is up (and simply return sSelf then).  Ensure() then will be use to init the database instead of Self() at [1].

[1] http://hg.mozilla.org/mozilla-central/annotate/be81b8d6fae9/dom/storage/DOMStorageCache.cpp#l775

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v2

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8625495 [details] [diff] [review]
Patch v2

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

Thanks!

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa7480b06109 is still running, but this really shouldn't break anything.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c76f4193401b

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c76f4193401b