I was under the impression that sessionstore-windows-restored is the notification used for this purpose, but as it turns out, that notification is only sent during the initial restoring of the windows and tabs the first time that the browser is starting up, and because many users (both in-tree and possibly extensions as well) rely on it to detect the browser startup, we need a new topic, which I'm going to tentatively name sessionstore-browser-state-set. This is needed for bug 526194 which is a potential 3.6 blocker, so quick reviews are appreciated! :-) Patch forthcoming.

It's worth noting (as I mentioned in channel) that sessionstore-windows-restored is also used in restoring when the process hasn't quit (c.f. bug 354894). So we're already using it in a non-startup case. But yea, since we've been using this topic for a while (and I know Weave and Jetpack both use it for some sort of startup, in addition to nsBrowserGlue), we should probably just use a new topic here.

Patch + unit test.

New patch with 8 lines of context

Comment on attachment 410404 [details] [diff] [review] Patch (v1) >+ * The Initial Developer of the Original Code is >+ * Ehsan Akhgari. >+ * Portions created by the Initial Developer are Copyright (C) 2009 >+ * the Initial Developer. All Rights Reserved. Nit: You work for us now, so I'm pretty sure this is _supposed_ to be "Mozilla Corporation". I think they get the copyright on the code. >+ // executeSoon is needed here in order to let the first window be focues >+ // (see above) Nit: Spelling. "focused"

(In reply to comment #4) > (From update of attachment 410404 [details] [diff] [review]) > >+ * The Initial Developer of the Original Code is > >+ * Ehsan Akhgari. > >+ * Portions created by the Initial Developer are Copyright (C) 2009 > >+ * the Initial Developer. All Rights Reserved. > > Nit: You work for us now, so I'm pretty sure this is _supposed_ to be "Mozilla > Corporation". I think they get the copyright on the code. Oh, you're absolutely right! :-) > >+ // executeSoon is needed here in order to let the first window be focues > >+ // (see above) > > Nit: Spelling. "focused" I probably should've left the rest of the work on this patch to tomorrow... ;-)

(In reply to comment #4) > (From update of attachment 410404 [details] [diff] [review]) > >+ * The Initial Developer of the Original Code is > >+ * Ehsan Akhgari. > >+ * Portions created by the Initial Developer are Copyright (C) 2009 > >+ * the Initial Developer. All Rights Reserved. > > Nit: You work for us now, so I'm pretty sure this is _supposed_ to be "Mozilla > Corporation". I think they get the copyright on the code. Isn't it the Foundation which owns the code? That's what I remember from a conversation with Gerv.

Not sure. CCing Gerv for input here.

Comment on attachment 410417 [details] [diff] [review] Patch (v1) >diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js >--- a/browser/components/sessionstore/src/nsSessionStore.js >+++ b/browser/components/sessionstore/src/nsSessionStore.js >@@ -65,16 +65,17 @@ const STATE_QUITTING = -1; > const STATE_STOPPED_STR = "stopped"; > const STATE_RUNNING_STR = "running"; > > const PRIVACY_NONE = 0; > const PRIVACY_ENCRYPTED = 1; > const PRIVACY_FULL = 2; > > const NOTIFY_WINDOWS_RESTORED = "sessionstore-windows-restored"; >+const NOTIFY_BROWSER_STATE_SET = "sessionstore-browser-state-set"; semantic issue: we notify when browser state restoration is *complete*, not when setBrowserState is *called*, so maybe should be sessionstore-browser-state-restored (which also makes it consistent w/ the other notification). >@@ -148,16 +149,19 @@ SessionStoreService.prototype = { > _interval: 10000, > > // when crash recovery is disabled, session data is not written to disk > _resume_from_crash: true, > > // During the initial restore tracks the number of windows yet to be restored > _restoreCount: 0, > >+ // During a setBrowserState call tracks the number of windows yet to be restored >+ _browserStateCount: 0, another semantic issue: this doesn't contain the number of browser states - it containers the number of windows to be restored. actually, is there a reason we can't re-use _restoreCount here? presumably, it maps exactly the same as _browserStateCount during initial restore, and is unused after that... >@@ -2819,16 +2830,25 @@ SessionStoreService.prototype = { > this._restoreCount--; > if (this._restoreCount == 0) { > // This was the last window restored at startup, notify observers. > this._observerService.notifyObservers(null, NOTIFY_WINDOWS_RESTORED, ""); > } > } > }, > >+ _notifyIfBrowserStateSet : function sss_notifyIfBrowserStateSet() { >+ if (this._browserStateCount) { >+ if (--this._browserStateCount == 0) { >+ // This was the last window restored after a setBrowserState call, notify observers. >+ this._observerService.notifyObservers(null, NOTIFY_BROWSER_STATE_SET, ""); >+ } >+ } >+ }, ok, and to take re-use a step further, why are we not just setting a flag indicating that setBrowserState was called, and re-use _notifyIfBrowserStateSet? (possibly renaming to _sendRestoreCompletedNotifications or something more generic like that)

(In reply to comment #8) > ok, and to take re-use a step further, why are we not just setting a flag > indicating that setBrowserState was called, and re-use > _notifyIfBrowserStateSet? (possibly renaming to > _sendRestoreCompletedNotifications or something more generic like that) I think what you meant was reuse _notifyIfAllWindowsRestored? I think that's the best way to do it - most code reuse. Then we just need to set _restoreCount and a flag like you say. Then only send NOTIFY_WINDOWS_RESTORED when the flag wasn't set, and always send NOTIFY_BROWSER_STATE_SET (or whatever the final name is)

exactly. thanks for the correction :)

Ehsan: if the author of the code is a MoCo or MoFo employee or contractor, the Initial Developer is the Mozilla Foundation, but the person should feel free to add themselves to the Contributors: line if they wish. Gerv

(In reply to comment #8) > (From update of attachment 410417 [details] [diff] [review]) > >diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js > >--- a/browser/components/sessionstore/src/nsSessionStore.js > >+++ b/browser/components/sessionstore/src/nsSessionStore.js > >@@ -65,16 +65,17 @@ const STATE_QUITTING = -1; > > const STATE_STOPPED_STR = "stopped"; > > const STATE_RUNNING_STR = "running"; > > > > const PRIVACY_NONE = 0; > > const PRIVACY_ENCRYPTED = 1; > > const PRIVACY_FULL = 2; > > > > const NOTIFY_WINDOWS_RESTORED = "sessionstore-windows-restored"; > >+const NOTIFY_BROWSER_STATE_SET = "sessionstore-browser-state-set"; > > semantic issue: we notify when browser state restoration is *complete*, not > when setBrowserState is *called*, so maybe should be > sessionstore-browser-state-restored (which also makes it consistent w/ the > other notification). Actually here by "set" I meant the past participle form of the verb, like "the browser state has been set" but I guess it's ambiguous, so I changed it to sessionstore-browser-state-restored (ditto for the constant name as well). > >@@ -148,16 +149,19 @@ SessionStoreService.prototype = { > > _interval: 10000, > > > > // when crash recovery is disabled, session data is not written to disk > > _resume_from_crash: true, > > > > // During the initial restore tracks the number of windows yet to be restored > > _restoreCount: 0, > > > >+ // During a setBrowserState call tracks the number of windows yet to be restored > >+ _browserStateCount: 0, > > another semantic issue: this doesn't contain the number of browser states - it > containers the number of windows to be restored. > > actually, is there a reason we can't re-use _restoreCount here? presumably, it > maps exactly the same as _browserStateCount during initial restore, and is > unused after that... I was just trying to be safe here (not re-use a variable which I was 100% sure it's safe to use), but if it's safe, it's logical to merge them both. Done. > >@@ -2819,16 +2830,25 @@ SessionStoreService.prototype = { > > this._restoreCount--; > > if (this._restoreCount == 0) { > > // This was the last window restored at startup, notify observers. > > this._observerService.notifyObservers(null, NOTIFY_WINDOWS_RESTORED, ""); > > } > > } > > }, > > > >+ _notifyIfBrowserStateSet : function sss_notifyIfBrowserStateSet() { > >+ if (this._browserStateCount) { > >+ if (--this._browserStateCount == 0) { > >+ // This was the last window restored after a setBrowserState call, notify observers. > >+ this._observerService.notifyObservers(null, NOTIFY_BROWSER_STATE_SET, ""); > >+ } > >+ } > >+ }, > > ok, and to take re-use a step further, why are we not just setting a flag > indicating that setBrowserState was called, and re-use > _notifyIfBrowserStateSet? (possibly renaming to > _sendRestoreCompletedNotifications or something more generic like that) Done. (In reply to comment #11) > Ehsan: if the author of the code is a MoCo or MoFo employee or contractor, the > Initial Developer is the Mozilla Foundation, but the person should feel free to > add themselves to the Contributors: line if they wish. Thanks for the explanation, done!

Comment on attachment 410522 [details] [diff] [review] Patch (v2) >- // During the initial restore tracks the number of windows yet to be restored >+ // During the initial restore and setBrowserState calls tracks the number of windows yet to be restored > _restoreCount: 0, nit: 80 character line length plz >+ // backup old state >+ let oldState = ss.getBrowserState(); >+ // create a new state for testing >+ let testState = { >+ windows: [ >+ { tabs: [{ entries: [{ url: "http://example.com/" }] }], selected: 1 }, >+ { tabs: [{ entries: [{ url: "about:robots" }] }], selected: 1 }, >+ ], >+ // make sure the first window is focues, otherwise when restoring the typo: focused >+ // old state, the first window is closed and the test harness gets unloaded >+ selectedWindow: 1 >+ }; >+ >+ let observer = { >+ pass: 1, >+ observe: function(aSubject, aTopic, aData) { >+ is(aTopic, "sessionstore-browser-state-restored", >+ "The sessionstore-browser-state-restored notification was observed"); >+ >+ if (this.pass++ == 1) { >+ is(browserWindowsCount(), 2, "Two windows should exist at this point"); >+ >+ // executeSoon is needed here in order to let the first window be focues >+ // (see above) ditto r=me, otherwise. thanks!

Landed with the nits addressed. http://hg.mozilla.org/mozilla-central/rev/39de5c51bd66

Comment on attachment 410522 [details] [diff] [review] Patch (v2) a192=beltzner

this code looks wrong _sendRestoreCompletedNotifications is hit also for windows restores, and this._browserSetState = false; is always called even if we did not notify browser-state-complete. being this._restoreCount and this._browserSetState global for all windows and not per window, looks like when setBrowserState opens a window, it can cause the notification to be fired even if setBrowserState has not finished. The test often fails for me and looks like is the restored window to fire the notification. I've not yet finished tracking this but i get notifications before i'm expecting them, and for sure before setBrowserState has finished.

hrm, something is strange, the order of the code and notifications is fine but during the test run the MS dotnetassistant fires an exception at this point the notification is fired without any reason and the test receives it. but the code sends the notification only later. TEST-INFO | chrome://mochikit/content/browser/../browser/browser/components/sessionstore/test/browser/browser_526613.js | Console message: SessionStore: sendRestoreCompletedNotifications has been called TEST-INFO | chrome://mochikit/content/browser/../browser/browser/components/sessionstore/test/browser/browser_526613.js | Console message: SessionStore: this._restoreCount = 1 (at this point i expect the notification to not be called) TEST-INFO | chrome://mochikit/content/browser/../browser/browser/components/sessionstore/test/browser/browser_526613.js | Console message: [JavaScript Error: "uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.clearUserPref]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://dotnetassistant/content/bootstrap.js :: BootStrapDotNetAsssitantExtension :: line 52" data: no]"] (the framework puts in the middle) TEST-PASS | chrome://mochikit/content/browser/../browser/browser/components/sessionstore/test/browser/browser_526613.js | The sessionstore-browser-state-restored notification was observed (wtf?)

could even be that TEST-INFO are reported shifted against TEST-PASS, that would explain the fancy ordering. but the test is still failing randomly on the second setBrowserState (2 windows are detected) :(

hm, TEST-INFO are reported observing the console, so async is probably involved :( practically we can't rely on TEST-INFO ordering relative to PASS, FAIL messages. So, everything is fine, but the fact the test randomly fails.

uh, closing a windows looks like an async operation, so when session restore calls close on windows, for SetBrowserState, it should actually wait for them to be closed, at least to notify...

i've posted a couple patches in bug 527074, one to notify after all windows have been properly closed, one to use the notification in current randomly failing tests. This patch can go on by itself, in browser i doubt it could cause any problem, it's just in tests that we have problems due to high speed they setState, test, and move to next tests...

notice i can't access bug 526194, so i don't know if that's involved or if i'm touching something i should not.

(In reply to comment #22) > notice i can't access bug 526194, so i don't know if that's involved or if i'm > touching something i should not. You are CC'ed now. So you can check.

Using this notification in tests for bug 522545