Session restore currently walks the various tabs and such synchronously to collect restore information. With Electrolysis that's a bad idea (because you can block the entire Firefox UI on a single misbehaving tab) and should also be unnecessary: each tab could asynchronously send session updates to chrome... you could even write each tab's data in a separate file as it comes to avoid big I/O hits writing all the JSON at once.

Comment on attachment 555823 [details] [diff] [review] Patch v1 (WIP) Review of attachment 555823 [details] [diff] [review]: ----------------------------------------------------------------- I haven't looked over everything, but here's a first pass. Overall I think it's looking good, but I'm going to ask Felipe to look over things too. ::: browser/base/content/content-sessionStore.js @@ +68,5 @@ > + addEventListener("pageshow", this, true); > + addEventListener("change", this, true); > + addEventListener("input", this, true); > + addEventListener("hashchange", this, true); > + addEventListener("DOMAutoComplete", this, true); Let's put these in a const up top and just loop over that. It'll make it easier to remove as well. @@ +77,5 @@ > + XPCOMUtils.defineLazyGetter(this, "_prefBranch", function () { > + return Cc["@mozilla.org/preferences-service;1"]. > + getService(Ci.nsIPrefService).getBranch("browser."). > + QueryInterface(Ci.nsIPrefBranch2); > + }); No point in making this lazy here since we use it right away (I know the service does this...) @@ +101,5 @@ > + // (form data, scrolling, etc.). This will only happen when a tab is > + // first restored. > + let originalTarget = aEvent.originalTarget; > + if (this.__SS_restore_data && > + originalTarget && originalTarget.defaultView && originalTarget.defaultView == originalTarget.defaultView.top) { Nit: line length @@ +108,5 @@ > + sendAsyncMessage("SessionStore:documentRestored"); > + } > + break; > + case "pageshow": > + sendAsyncMessage("SessionStore:pageshow", { Is there a difference between the messages going to or from content? They don't seem to be distinguished in any way? I'm not saying that we need to, just making sure I understand. @@ +122,5 @@ > + case "SessionStore:restoreTab": > + this.restoreTab(aMessage.json); > + break; > + case "SessionStore:restoreHistory": > + this.restoreHistory(aMessage.json, 0); break; (not strictly necessary, but for consistency) @@ +131,5 @@ > + switch (aTopic) { > + case "timer-callback": > + this.updateState(); > + break; > + } A switch here seems unnecessary... @@ +143,5 @@ > + return Services.io.newURI(aString, null, null); > + }, > + > + restoreDocument: function ssl_restoreDocument() { > + let that = this; Is that being used anywhere? @@ +192,5 @@ > + function restoreTextDataAndScrolling(aContent, aData, aPrefix) { > + if (aData.formdata) > + restoreFormData(aContent.document, aData.formdata, aData.url); > + if (aData.innerHTML) { > + executeSoon(function() { executeSoon? @@ +294,5 @@ > + restoreHistory: function sss_restoreHistory(aMessageData, aCount) { > + try { > + if (!webNavigation.sessionHistory) > + throw new Error(); > + } catch (ex) { Nit: You touched it so you get to fix it. Please put the catch on a new line @@ +301,5 @@ > + this._sessionHistoryTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > + this._sessionHistoryTimer.initWithCallback(function(aTimer) { > + delete _this._sessionHistoryTimer; > + _this.restoreHistory(aMessageData, aCount + 1); > + }, 100, Ci.nsITimer.TYPE_ONE_SHOT); This was using setTimeout. Any particular reason to change it? @@ +484,5 @@ > + ***********************/ > + > + __SS_data: { > + entries: [], > + }, Please put this up at the top of the object @@ +504,5 @@ > + let node = formNodes.iterateNext(); > + if (!node) > + return null; > + > + const MAX_GENERATED_XPATHS = 100; I know this is inherited, but let's move this const up top. @@ +610,5 @@ > + delete _this._timer; > + } > + this._timer.init(this, aDelay, Ci.nsITimer.TYPE_ONE_SHOT); > + } > +#endif What's happening here? @@ +989,5 @@ > + > + /** > + * Generates an approximate XPath query to an (X)HTML node > + */ > + generate: function sss_xph_generate(aNode) { Nit: sss isn't really an applicable prefix though it really doesn't matter. Change them if you get the time, otherwise no big deal. @@ +1077,5 @@ > +// A SessionStoreSHistoryListener will be attached to each browser before it is > +// restored. We need to catch reloads that occur before the tab is restored > +// because otherwise, docShell will reload an old URI (usually about:blank). > +function SessionStoreSHistoryListener(ssl) { > + this.ssl = ssl; You're not using ssl, just sending a message. Also I don't think we need to even have this as a prototype anymore since each content script is once per browser. We needed one listener per browser in the old world, but we get that for free now. @@ +1096,5 @@ > + > + // Returning false will stop the load that docshell is attempting. > + return false; > + } > +} This is a new file and while a lot of it is copy+pasted from nsSessionStore, it's a clean slate. So let's do this... :%s/ \+$// ::: browser/base/content/content.js @@ +59,5 @@ > +XPCOMUtils.defineLazyServiceGetter(this, "SecuritySvc", > + "@mozilla.org/scriptsecuritymanager;1", "nsIScriptSecurityManager"); > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > + This seems backwards... ::: browser/components/sessionstore/src/nsSessionStore.js @@ +696,4 @@ > */ > handleEvent: function sss_handleEvent(aEvent) { > var win = aEvent.currentTarget.ownerDocument.defaultView; > + var browser = aEvent.currentTarget; you're not using browser are you? @@ +728,5 @@ > + receiveMessage: function sss_receiveMessage(aMessage) { > + var browser = aMessage.target; > + var win = browser.ownerDocument.defaultView; > + > + if (aMessage.json && aMessage.json.messageKey && aMessage.json.messageKey != browser._messageKey) nit: line length @@ +1033,5 @@ > let browser = aTab.linkedBrowser; > + browser.messageManager.addMessageListener("SessionStore:documentRestored", this); > + browser.messageManager.addMessageListener("SessionStore:pageshow", this); > + browser.messageManager.addMessageListener("SessionStore:historyReload", this); > + browser.messageManager.addMessageListener("SessionStore:updateBrowserState", this); Let's put these in a constant up top @@ +1055,5 @@ > onTabRemove: function sss_onTabRemove(aWindow, aTab, aNoNotification) { > let browser = aTab.linkedBrowser; > + browser.messageManager.removeMessageListener("SessionStore:documentRestored", this); > + browser.messageManager.removeMessageListener("SessionStore:pageshow", this); > + browser.messageManager.removeMessageListener("SessionStore:historyReload", this); I think you missed SessionStore:updateBrowserState @@ +1131,4 @@ > return; > } > > + // XXX jez why do we invalidate the cache here? I think we talked about this. I'm sure you'll run back over your XXX comments so I won't say anything for other ones. @@ +1677,2 @@ > var browser = aTab.linkedBrowser; > + var tabData = { entries: [] }; Nit: Insignificant, but why move this 2 lines? @@ +1684,2 @@ > > + if (browser.__SS_data) { If I understand this right, this will always be there? @@ +1688,5 @@ > if (!aFullData) > + tabData.entries.forEach(function(entry) { > + if (entry.url != "about:sessionrestore") > + _this._filterEntryData(entry, aTab.pinned); > + }); I think we can skip the check for about:sessionrestore here. It won't have _scheme so that will fail (might be a strict error though). We'd really want to special case all about: urls or none at all. You can also pass `this` to forEach as the last param so we can avoid the `let _this = this` shenanigans. @@ +1740,3 @@ > }, > > + _filterEntryData: function sss__filterEntryData(aEntry, aIsPinned) { Make sure you say what this is for and usage @@ +2367,4 @@ > let uri = activePageData ? activePageData.url || null : null; > browser.userTypedValue = uri; > > +#ifndef MOZ_E10S_COMPAT Please add a comment about each of these ifndef checks (why each is needed, criteria for deleting the whole block, are these things done in the content-script?, etc). Anything that will help in the future. @@ +2460,5 @@ > }, 0); > > + }, > + > + _historyRestored: function sss__historyRestored(aBrowser, aMessageData) { You want to remove the messageListener right? @@ +2498,4 @@ > * > * @returns true/false indicating whether or not a load actually happened > */ > + restoreTab: function sss_restoreTab(aTab, aCallback) { aCallback isn't used... left over from a previous iteration? @@ +2537,5 @@ > } > > + if (aBrowser.__SS_restoreTabCallback) > + aBrowser.__SS_restoreTabCallback(aMessageData.didStartLoad); > + delete aBrowser.__SS_restoreTabCallback; This isn't set anywhere... left over code? @@ +3389,5 @@ > // The browser is no longer in any sort of restoring state. > delete browser.__SS_restoreState; > > + delete browser._messageKey; > + Isn't this still needed for other messages?

There's a bunch of stuff in browser/base/content/content-sessionStore.js "polluting" the shared scope. You should either create your own scope (e.g. '(function () { [your code] })();') or move your code to browser/components/sessionstore/src/content.js (bug 673569 will make it have its own scope).

Comment on attachment 555823 [details] [diff] [review] Patch v1 (WIP) Review of attachment 555823 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/content-sessionStore.js @@ +46,5 @@ > +const STATE_QUITTING = -1; > + > +const PRIVACY_NONE = 0; > +const PRIVACY_ENCRYPTED = 1; > +const PRIVACY_FULL = 2; I wonder if we should add these consts to nsISessionStore instead of having them hanging here as globals @@ +55,5 @@ > +eg: browser.docShell["allow" + aCapability] = false; > + > +XXX keep these in sync with all the attributes starting > + with "allow" in /docshell/base/nsIDocShell.idl > +*/ nit: I know you just moved this comment to here, but can it be improved a bit? It looks like a TODO item @@ +82,5 @@ > + > + // this pref is only read at startup, so no need to observe it > + this._sessionhistory_max_entries = > + this._prefBranch.getIntPref("sessionhistory.max_entries"); > + (From the updated file) > this._browserID = "" + Date.now() + Math.random(); Can the currentInnerWindowID be used here? Example usage: mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/bindings/browser.js#23 @@ +113,5 @@ > + persisted: aEvent.persisted > + }); > + this.updateState(); > + break; > + } (From the updated patch) > case "pageshow": > // we add the handler here to avoid disabling caching I don't understand what that means > case "beforeunload": > // by the time we receive the frame script unload event, some data cannot be retrieved, > // so we cache it here first. we don't seem to get a document unload event on tab close, > // so we use the beforeunload event instead. > this.__SS_final_data = this._collectData(); please comment the intentional fall-through > case "unload": > if (aEvent.target == messageManager) { > this.__SS_final_data._browserID = this._browserID; > this._processMessageManager.sendSyncMessage("SessionStore:updateBrowserStateFinal", this.__SS_final_data); > } > break; > } Please add a brief comment explaining why the process manager is used. When do we get the unload event then (given the comment above)? Should we file a bug about unload not being fired, or is it a problem of timing (when unload happens it's too late?) Can you use sendAsyncMessage instead? Why the target == messageManager check, is that to skip sub-frames unload/beforeunload? @@ +610,5 @@ > + delete _this._timer; > + } > + this._timer.init(this, aDelay, Ci.nsITimer.TYPE_ONE_SHOT); > + } > +#endif (From the updated file) function callback and _this variable are unused now. Why the _inPrivateBrowsing check? @@ +748,5 @@ > + } > + else if (history && history.count > 0) { > + for (var j = 0; j < history.count; j++) { > + let entry = this._serializeHistoryEntry(history.getEntryAtIndex(j, false), > + aFullData, docShell.isAppTab); is docShell.isAppTab actually working? I bet that in true e10s it isn't. Not that fixing it should be covered in this bug, but are there tests on this that would catch if it is indeed not working? This is the data entry point for all the aIsPinned parameters in other functions, right? ::: browser/base/content/content.js @@ +38,5 @@ > > +var Cc = Components.classes; > +var Ci = Components.interfaces; > +var Cu = Components.utils; > + why change the consts? @@ +46,1 @@ > is this the variable used only in the unload listener from the other file? if so, let's not define it globally, but rather as a field in that object @@ +73,5 @@ > + > +function executeSoon(aFunc) { > + Services.tm.currentThread.dispatch(aFunc, Ci.nsIThread.DISPATCH_NORMAL); > +} > + just an observation for myself: this is probably not the best place to add helper functions like this; I think we'll want a new file in the future.. and in this rather executeSoon should be part of the functions offered by the frameloader. I'll file a bug on this @@ +74,5 @@ > +function executeSoon(aFunc) { > + Services.tm.currentThread.dispatch(aFunc, Ci.nsIThread.DISPATCH_NORMAL); > +} > + > +#include content-sessionStore.js I'd prefer an independent loadFrameScript for this file, specially to keep all its variables in a different scope (after 673569). But IIRC dao had a point about just using #includes. For now a good test would be to wrap the whole content-sessionStore.js file in an anonymous function, to make sure it won't leak any consts and vars to other scripts (and that nothing is relying on that behavior). ::: browser/components/sessionstore/src/nsSessionStore.js @@ +2312,5 @@ > > + // assign a unique key so that all messages generated by this call > + // to restoreHistoryPrecursor can be associated with it > + let messageKey = "" + Date.now() + Math.random(); > + not really important, but could this be a simpler counter? @@ +2367,4 @@ > let uri = activePageData ? activePageData.url || null : null; > browser.userTypedValue = uri; > > +#ifndef MOZ_E10S_COMPAT Yeah, and if possible file bugs about each of them (at least, each root cause, e.g. "lack of currentURI support"), to reference the bug number in the comments. These bugs should block bug 663042.

(In reply to Felipe Gomes (:felipe) from comment #5) > I'd prefer an independent loadFrameScript for this file, specially to keep > all its variables in a different scope (after 673569). But IIRC dao had a > point about just using #includes. I didn't say we should under all circumstances use #include. There's a separate sessionstore component and it doesn't seem to make a lot of sense for one half of the code to live in browser/base/.

(In reply to Paul O'Shannessy [:zpao] from comment #3) > > @@ +108,5 @@ > > + sendAsyncMessage("SessionStore:documentRestored"); > > + } > > + break; > > + case "pageshow": > > + sendAsyncMessage("SessionStore:pageshow", { > > Is there a difference between the messages going to or from content? They > don't seem to be distinguished in any way? I'm not saying that we need to, > just making sure I understand. > Their naming doesn't distinguish them in any way. I haven't found it necessary to do so, at least not with the number of messages SS needs. > @@ +143,5 @@ > > + return Services.io.newURI(aString, null, null); > > + }, > > + > > + restoreDocument: function ssl_restoreDocument() { > > + let that = this; > > Is that being used anywhere? Nope, removed. > @@ +192,5 @@ > > + function restoreTextDataAndScrolling(aContent, aData, aPrefix) { > > + if (aData.formdata) > > + restoreFormData(aContent.document, aData.formdata, aData.url); > > + if (aData.innerHTML) { > > + executeSoon(function() { > > executeSoon? I've added it as a function to content.js, though it probably should be moved elsewhere. There isn't a global setTimeout function available, so this seemed like a good alternative. > @@ +301,5 @@ > > + this._sessionHistoryTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > > + this._sessionHistoryTimer.initWithCallback(function(aTimer) { > > + delete _this._sessionHistoryTimer; > > + _this.restoreHistory(aMessageData, aCount + 1); > > + }, 100, Ci.nsITimer.TYPE_ONE_SHOT); > > This was using setTimeout. Any particular reason to change it? > Once again, there isn't a setTimeout available. Mobile does something similar. > @@ +484,5 @@ > > + ***********************/ > > + > > + __SS_data: { > > + entries: [], > > + }, > > Please put this up at the top of the object Actually this was completely unnecessary and has been removed. > @@ +610,5 @@ > > + delete _this._timer; > > + } > > + this._timer.init(this, aDelay, Ci.nsITimer.TYPE_ONE_SHOT); > > + } > > +#endif > > What's happening here? > I disabled delayed updating during that last iteration, hence the `#if 0`. I've since added it back in. > @@ +1684,2 @@ > > > > + if (browser.__SS_data) { > > If I understand this right, this will always be there? I think this was true in patch v1, but not right now. > > + if (aBrowser.__SS_restoreTabCallback) > > + aBrowser.__SS_restoreTabCallback(aMessageData.didStartLoad); > > + delete aBrowser.__SS_restoreTabCallback; > > This isn't set anywhere... left over code? > It was actually supposed to get set... fixed that. > @@ +3389,5 @@ > > // The browser is no longer in any sort of restoring state. > > delete browser.__SS_restoreState; > > > > + delete browser._messageKey; > > + > > Isn't this still needed for other messages? Nope, it's only used for the restoring process. Since the control flow while restoring is chrome->content->chrome, we need it to match up the two chrome functions. For content events like input and change, it's just content->chrome, so the _messageKey isn't used. It might be good to use the messageKey for other chrome->content->chrome operations though, like getTabState, but we should generate a new messageKey for those calls anyway, so deleting _messageKey here is correct. (In reply to Felipe Gomes (:felipe) from comment #5) > Comment on attachment 555823 [details] [diff] [review] > Patch v1 (WIP) > > Review of attachment 555823 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +82,5 @@ > > + > > + // this pref is only read at startup, so no need to observe it > > + this._sessionhistory_max_entries = > > + this._prefBranch.getIntPref("sessionhistory.max_entries"); > > + > > (From the updated file) > > > this._browserID = "" + Date.now() + Math.random(); > Can the currentInnerWindowID be used here? Example usage: > mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/bindings/ > browser.js#23 > Yeah I think that we could use that. I think we should follow mobile's lead and add the contentWindowId property to the browser object so other services can use it too. > @@ +113,5 @@ > > + persisted: aEvent.persisted > > + }); > > + this.updateState(); > > + break; > > + } > > (From the updated patch) > > case "pageshow": > > // we add the handler here to avoid disabling caching > I don't understand what that means That comment was supposed to have been deleted. Oops. > > case "beforeunload": > > // by the time we receive the frame script unload event, some data cannot be retrieved, > > // so we cache it here first. we don't seem to get a document unload event on tab close, > > // so we use the beforeunload event instead. > > this.__SS_final_data = this._collectData(); > > please comment the intentional fall-through Not intentional. Oops x2. > > case "unload": > > if (aEvent.target == messageManager) { > > this.__SS_final_data._browserID = this._browserID; > > this._processMessageManager.sendSyncMessage("SessionStore:updateBrowserStateFinal", this.__SS_final_data); > > } > > break; > > } > > Please add a brief comment explaining why the process manager is used. > > When do we get the unload event then (given the comment above)? Should we > file a bug about unload not being fired, or is it a problem of timing (when > unload happens it's too late?) > > Why the target == messageManager check, is that to skip sub-frames > unload/beforeunload? > The check is to skip the unload events that originate from page navigation. I think that we should get two unload events when we close the tab -- one from the page unloading and one from the message manager unloading. However I only seem to get an unload event from the latter, hence the complicated business with beforeunload. > Can you use sendAsyncMessage instead? > Not sure -- I'll try it and let you know. > Why the _inPrivateBrowsing check? > > @@ +748,5 @@ > > + } > > + else if (history && history.count > 0) { > > + for (var j = 0; j < history.count; j++) { > > + let entry = this._serializeHistoryEntry(history.getEntryAtIndex(j, false), > > + aFullData, docShell.isAppTab); > > is docShell.isAppTab actually working? I bet that in true e10s it isn't. Not > that fixing it should be covered in this bug, but are there tests on this > that would catch if it is indeed not working? > > This is the data entry point for all the aIsPinned parameters in other > functions, right? Yeah, it doesn't work in e10s, but seeing as it's a property on the docShell, I assumed someone was going to fix it up for e10s :P and yes, it's what is used for aIsPinned in the other functions. I'll have to check on whether we have tests that catch it if it fails. > > ::: browser/base/content/content.js > @@ +38,5 @@ > > > > +var Cc = Components.classes; > > +var Ci = Components.interfaces; > > +var Cu = Components.utils; > > + > > why change the consts? > It was giving me a bunch of redeclaration-errors on the error console. I wasn't sure whether those errors were affecting my other code, but I decided to play it safe and change them to vars for now. > @@ +46,1 @@ > > var messageManager = this; > > is this the variable used only in the unload listener from the other file? > if so, let's not define it globally, but rather as a field in that object > I guess we could do this, but it seems like it would be useful to have a global way to refer to the message manager. Maybe we could make it a global var in all frame scripts, like content and docShell? > ::: browser/components/sessionstore/src/nsSessionStore.js > @@ +2312,5 @@ > > > > + // assign a unique key so that all messages generated by this call > > + // to restoreHistoryPrecursor can be associated with it > > + let messageKey = "" + Date.now() + Math.random(); > > + > > not really important, but could this be a simpler counter? > Yes, but then we'd have to maintain a counter variable on the SessionStore service. I'm not sure which is simpler code-wise. I haven't had the time to address all the comments, particularly the ones on scoping, but I'll try to do so as soon as possible.

The last patch didn't include all the changes that I meant to include.

So, what's the status of the patch? Is there some subset that can be used to make Session Restore usefully asynchronous without e10s?

Mmmhh... two months without reply. I suspect that this bug is kind of dead. Never mind, we now have plans for the reimplementation of session restore.

As-summarized this is WONTFIX, but it would definitely be useful to glance over the patches to see whether anything is still relevant.

Does this mean session restore is going to be dropped once e10s lands?

No, we already started making parts of SessionRestore e10s compatible, in a more iterative approach.

(In reply to Tim Taubert [:ttaubert] from comment #13) > No, we already started making parts of SessionRestore e10s compatible, in a > more iterative approach. Tim, What bug is this other work being done in? I'm asking as I'd like to annotate anywhere we disable tests due to SessionRestore in e10s with the bug number.

Bug 942374 should fix the last pieces to make to restoration part of SessionStore work.