Last Comment Bug 898308 - Clean up SessionStore initialization
: Clean up SessionStore initialization
Status: RESOLVED FIXED
: addon-compat
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 25
Assigned To: Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
:
Mentors:
Depends on: 900910 904477 959130
Blocks: 898732 910167
  Show dependency treegraph
 
Reported: 2013-07-26 01:47 PDT by Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
Modified: 2014-01-17 21:03 PST (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled


Attachments
Clean up SessionStore initialization (15.37 KB, patch)
2013-07-26 01:56 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
smacleod: feedback+
Details | Diff | Splinter Review
Clean up SessionStore initialization, v2 (15.91 KB, patch)
2013-07-26 13:18 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
dao+bmo: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-07-26 01:47:41 PDT
Currently, SessionStore is initialized by the delayedStartup() routine. Every time a browser window is opened we call ss.init() that then lazily initializes the service and starts tracking the window.

That's certainly a bad API, nsBrowserGlue should just call ss.init() once and browser.js should then use ss.promiseInitialized.then() to initialize stuff that depends on ss being initialized.
Comment 1 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-07-26 01:56:13 PDT
Created attachment 781583 [details] [diff] [review]
Clean up SessionStore initialization

Steven, as you did somewhat related work in bug 881049, I'd really like to get your thoughts on this.
Comment 2 Steven MacLeod [:smacleod] 2013-07-26 08:51:18 PDT
Comment on attachment 781583 [details] [diff] [review]
Clean up SessionStore initialization

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

Looks good to me! Thanks for cleaning up the whole "promise" vs "deferred" naming I missed in my patch :D
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-07-26 09:04:03 PDT
Comment on attachment 781583 [details] [diff] [review]
Clean up SessionStore initialization

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+XPCOMUtils.defineLazyModuleGetter(this, "SessionStore",

>diff --git a/browser/base/content/test/social/browser_social_window.js b/browser/base/content/test/social/browser_social_window.js

>+let tmp = {};
>+Cu.import("resource:///modules/sessionstore/SessionStore.jsm", tmp);
>+let {SessionStore} = tmp;

This could just instead refer to the browser.js global you added, right?

(I wonder why Social depends on sessionstore initialization...)

(I also wonder a bit whether the introduction of the "SessionStore" global will cause extension conflicts, but I suppose that's probably unlikely.)

Looks like a nice cleanup!
Comment 4 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-07-26 13:16:48 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)
> >+let tmp = {};
> >+Cu.import("resource:///modules/sessionstore/SessionStore.jsm", tmp);
> >+let {SessionStore} = tmp;
> 
> This could just instead refer to the browser.js global you added, right?

Good idea... :)

> (I wonder why Social depends on sessionstore initialization...)

Hmmm. Maybe it actually doesn't. I couldn't find any code referring to it. Maybe we came to the wrong conclusions with all the initial test failures in bug 881049. I'll try to make it not depend on SessionStore initialization anymore and see what try says.

> (I also wonder a bit whether the introduction of the "SessionStore" global
> will cause extension conflicts, but I suppose that's probably unlikely.)

Yeah, I wondered that, too. We'll see I guess.
Comment 5 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-07-26 13:18:42 PDT
Created attachment 781896 [details] [diff] [review]
Clean up SessionStore initialization, v2

https://tbpl.mozilla.org/?tree=Try&rev=61d2f733b333
Comment 6 Dão Gottwald [:dao] 2013-07-26 15:13:05 PDT
Comment on attachment 781896 [details] [diff] [review]
Clean up SessionStore initialization, v2

>+  init: function (aWindow) {
>+    if (this._initialized) {
>+      throw new Error("SessionStore.init() must only be called once!");
>+    }
>+
>+    if (!aWindow) {
>+      throw new Error("SessionStore.init() must be called with a valid window.");
>+    }
>+
>     TelemetryTimestamps.add("sessionRestoreInitialized");
>     OBSERVING.forEach(function(aTopic) {
>       Services.obs.addObserver(this, aTopic, true);
>     }, this);
> 
>     this._initPrefs();
>-
>+    this._initialized = true;
>     this._disabledForMultiProcess = this._prefBranch.getBoolPref("tabs.remote");
> 
>     // this pref is only read at startup, so no need to observe it
>     this._sessionhistory_max_entries =
>       this._prefBranch.getIntPref("sessionhistory.max_entries");
> 
>-    gSessionStartup.onceInitialized.then(
>-      this.initSession.bind(this)
>-    );
>+    // Wait until nsISessionStartup has finished reading the session data.
>+    return gSessionStartup.onceInitialized.then(() => {
>+      // Parse session data and start restoring.
>+      this.initSession();
>+
>+      // Start tracking the given (initial) browser window.
>+      if (!aWindow.closed) {
>+        this.onLoad(aWindow);
>+      }
>+
>+      // Let everyone know we're done.
>+      this._deferredInitialized.resolve();
>+    });
>   },

the return value appears to be unused...
Comment 7 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-07-26 16:37:31 PDT
(In reply to Dão Gottwald [:dao] from comment #6)
> the return value appears to be unused...

It is. Thanks for catching that!
Comment 8 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-07-26 22:43:52 PDT
https://hg.mozilla.org/integration/fx-team/rev/1e1f3cd07479
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-07-27 18:59:09 PDT
https://hg.mozilla.org/mozilla-central/rev/1e1f3cd07479
Comment 10 tabmix.onemen 2013-07-28 07:16:12 PDT
BrowserGlue._onFirstWindowLoaded is broken if extension call SessionStore.init before BrowserGlue._onFirstWindowLoaded

we need a way to now if SessionStore already initialized.

you can set SessionStore.init not to throw if it already initialized and post console message instead.
Comment 11 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-07-29 04:19:31 PDT
(In reply to onemen.one from comment #10)
> BrowserGlue._onFirstWindowLoaded is broken if extension call
> SessionStore.init before BrowserGlue._onFirstWindowLoaded

Extensions are not supposed to call that.

> we need a way to know if SessionStore already initialized.

Please use SessionStore.promiseInitialized. That's the right place to initialize stuff that depends on SessionStore being ready.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-07-29 09:56:15 PDT
(In reply to Tim Taubert [:ttaubert] from comment #11)
> (In reply to onemen.one from comment #10)
> > BrowserGlue._onFirstWindowLoaded is broken if extension call
> > SessionStore.init before BrowserGlue._onFirstWindowLoaded
> 
> Extensions are not supposed to call that.

Is this well documented? Do we know to what extent they already do call it, and whether their use cases are covered by other existing APIs (IIRC SessionStore.promiseInitialized is relatively new, how did things work previously)?

Seems like it might be prudent to guard against this case if we have reason to believe it will happen in practice.
Comment 13 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-07-29 10:05:29 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)
> Is this well documented? Do we know to what extent they already do call it,
> and whether their use cases are covered by other existing APIs (IIRC
> SessionStore.promiseInitialized is relatively new, how did things work
> previously)?

From https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsISessionStore#init%28%29

"Note: This function is intended for use only by the browser; extensions shouldn't call it."

> Seems like it might be prudent to guard against this case if we have reason
> to believe it will happen in practice.

Yeah, I wouldn't mind removing the error and just fail silently though I haven't heard of any actual add-on breaking, yet. Comment #10 is just stating that add-ons might break.
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-07-29 14:26:01 PDT
I was assuming onemen was referring to an actual problem he ran into with TabMixPlus, but maybe that's a bad assumption!
Comment 15 :Paolo Amadini 2013-08-08 10:25:53 PDT
In bug 902866, we detected a missing error handler for a promise here:

gSessionStartup.onceInitialized.then(() => {
  // ...
});

Should read:

gSessionStartup.onceInitialized.then(() => {
  // ...
}).then(null, Cu.reportError);

It would be cool if we could detect those cases automatically at runtime or with
static analysis, for the moment let's spread the word about error handling in
promises and keep an eye on that when reviewing!

https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise#Handling_errors_and_common_pitfalls

Note You need to log in before you can comment on or make changes to this bug.