Clean up SessionStore initialization

RESOLVED FIXED in Firefox 25

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

({addon-compat})

Trunk
Firefox 25
addon-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 disabled)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
Attachment #781583 - Flags: feedback?(smacleod)
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
Attachment #781583 - Flags: feedback?(smacleod) → feedback+
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!
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
Created attachment 781896 [details] [diff] [review]
Clean up SessionStore initialization, v2

https://tbpl.mozilla.org/?tree=Try&rev=61d2f733b333
Attachment #781583 - Attachment is obsolete: true
Attachment #781896 - Flags: review?(dao)
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...
Attachment #781896 - Flags: review?(dao) → review+
(Assignee)

Comment 7

4 years ago
(In reply to Dão Gottwald [:dao] from comment #6)
> the return value appears to be unused...

It is. Thanks for catching that!
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/1e1f3cd07479

Updated

4 years ago
Blocks: 898732
https://hg.mozilla.org/mozilla-central/rev/1e1f3cd07479
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25

Comment 10

4 years ago
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.
(Assignee)

Comment 11

4 years ago
(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.
Keywords: addon-compat
(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.
(Assignee)

Comment 13

4 years ago
(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.
I was assuming onemen was referring to an actual problem he ran into with TabMixPlus, but maybe that's a bad assumption!

Comment 15

4 years ago
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
(Assignee)

Updated

4 years ago
Depends on: 904477

Updated

4 years ago
Blocks: 910167
Depends on: 900910
status-firefox25: --- → disabled
(Assignee)

Updated

3 years ago
Depends on: 959130
You need to log in before you can comment on or make changes to this bug.