Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Description • 11 years ago

This is the restore side of making session restore work in e10s.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 1 • 11 years ago

This is the main patch. I tried splitting it up in different ways (one patch per message, or doing a module patch and a message patch like bug 910646), but things are so intertwined that I couldn't find a good way to do it. Here's what it does:

* Adds a module, ContentRestore, that takes care of doing all the restoration tasks for the child.

* Uses messages from SessionStore.jsm to content-sessionStore.jsm to do the main restore tasks: restoreHistory, restoreTabContent, and restoreDocument.

* Movies the SessionStoreSHistoryListener and gRestoreTabsProgressListener from SessionStore.jsm to ContentListener.jsm.

* Since the parent and child aren't necessarily in sync, the parent has a number (the epoch) that it increments every time it starts a new restore. It uses the number to discard stale data from the child.

* Copies a lot of the session restore state to weakmaps in ContentRestore module. The state is still present in fields like __SS_data and __SS_restore_data because I didn't want to break add-ons. But this patch should make it a lot easier to remove these fields when we decide to do that.

The actual logic for restoring is pretty similar to how it worked before. Some of it seems a little odd to me. For example, the way that restores are finished is a bit strange (some state is deleted by resetTabRestoringState and other stuff by restoreDocument). I tried to clean this code up, but I ran into a lot of problems and gave up. So things work mostly the same as they do now, but with messages.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 2 • 11 years ago

This patches two tests that depend very tightly on the behavior of session restore.

In browser_581593.js, the test assumes that the "input" event will happen after "SSTabRestored". With my changes, the SSTabRestored event comes later. Previously we triggered SSTabRestored on a load event. Now we get the load event in content-sessionStore.js, which sends a message to SessionStore.jsm and only then do we fire SSTabRestored. The additional message delay causes SSTabRestored to happen after "input". There might be a better way to fix this, but I just eliminated the extra listener for "input".

In browser_687710_2.js, we call setTabState and then expect webNavigation.sessionHistory to reflect the new state immediately. There's no way we can do that using messaging, so I just changed the test to wait for SSTabRestored.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 3 • 11 years ago

There are a bunch of tests that add their own nsIWebProgressListener. When it fires, they observe the internal state of SessionStore.jsm and check that it matches what they expect.

With the patch in comment 1, SessionStore.jsm no longer updates its internal state directly based on nsIWebProgressListener. The WP listener lives on the content side and it sends messages to the parent. So there's an extra message delay between when the test's WP listener fires and when SessionStore.jsm updates its internal state.

I decided to switch these tests so that they use a custom listener rather than an nsIWebProgressListener. Now they get a notification when SessionStore.jsm receives the SessionStore:restoreTabContentComplete that causes it to update its internal state.

I tried to have the tests listen on SessionStore:restoreTabContentComplete themselves. However, their listener fires after SessionStore.jsm's listener, and the tests want to go first.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 4 • 11 years ago

This patch changes the tests to use the new callback that was added in the previous patch.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 5 • 11 years ago

Enable session restore in e10s. \o/

	Tim Taubert [:ttaubert] (inactive)
	Comment 6 • 11 years ago

Bill, sorry for the delay. I'll try to review those ASAP.

	Tim Taubert [:ttaubert] (inactive)
	Comment 7 • 11 years ago

Comment on attachment 8337161 [details] [diff] [review]
rm-disable-for-e10s

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

Starting with the easy stuff first ;) r=me

	Tim Taubert [:ttaubert] (inactive)
	Comment 8 • 11 years ago

Comment on attachment 8337152 [details] [diff] [review]
restore-via-msgs

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

This looks promising, I hope we can clean up the process a little. I wanted to play around with it but unsurprisingly this needs some rebasing love :)

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +156,5 @@
> +        let reloadCallback = () => {
> +          // Inform SessionStore.jsm about the reload. It will send
> +          // restoreTabContent in response.
> +          sendAsyncMessage("SessionStore:reloadRestoringTab", {epoch: epoch});
> +        };

Wouldn't it be better if we could tell the parent that we started the restore ourselves for whatever reason? The message ping-pong seems unnecessary.

@@ +163,5 @@
> +      case "SessionStore:restoreTabContent":
> +        epoch = ContentRestore.getRestoreEpoch(docShell);
> +        let finishCallback = () => {
> +          // Tell SessionStore.jsm that it may want to restore some more tabs,
> +          // since it restores a max of 3 at a time.

Nit: the number is in MAX_CONCURRENT_TAB_RESTORES and may in theory change.

::: browser/components/sessionstore/src/ContentRestore.jsm
@@ +9,5 @@
> +const Cu = Components.utils;
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;
> +
> +Cu.import("resource://gre/modules/Services.jsm", this);

Nit: this seems unused

@@ +46,5 @@
> + * will send a message to SessionStore.jsm, which may cause other tabs to be
> + * restored.
> + *
> + * When the page has finished loading, a "load" event will trigger in
> + * content-sessionStore.jsm, which will call restoreDocument. At that point,

Nit: content-sessionStore.js

@@ +58,5 @@
> + * sent back to SessionStore.jsm include the epoch. This way, SessionStore.jsm
> + * can discard messages that relate to restores that it has canceled (by
> + * starting a new restore, say).
> + */
> +this.ContentRestore = Object.freeze({

Would it be easier to make "ContentRestore" a "class"? The frame script could then do:

let gContentRestore = new ContentRestore(docShell);

The pattern is a little different from what we do usually but there isn't much value in sharing anyway if we assume that each tab could have its own process. We wouldn't need to pass docShells and wouldn't have to use that many WeakMaps. I did exactly that in bug 921942 for FrameTree.jsm.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ -633,5 @@
>  
>      var win = aEvent.currentTarget.ownerDocument.defaultView;
>      let browser;
>      switch (aEvent.type) {
> -      case "load":

Don't forget to remove the add/removeEventListener() calls in onTabAdd/Remove().

@@ +2654,5 @@
>  
> +      // Start a new epoch and include the epoch in the beginRestore
> +      // message. If a message is received that relates to a previous epoch, we
> +      // discard it.
> +      browser.__SS_restoreEpoch = this._nextRestoreEpoch++;

I don't like adding more properties like that to the browsers and exposing more internals. If anything starts using that (for whatever reason) we're stuck again when trying to rewrite code. We should use a WeakMap - I was planning to do something like that in bug 921942 which becomes somewhat obsolete with all the code moving to the frame script.

@@ +2665,5 @@
>        tab.setAttribute("pending", "true");
>  
> +      browser.messageManager.sendAsyncMessage("SessionStore:beginRestore",
> +                                              {tabData: tabData,
> +                                               epoch: browser.__SS_restoreEpoch});

So why exactly do we need this extra step? This kind of made sense in a world where all tabs except the first were restored after a few ticks. But now that we send an async message we won't stop the load or call setCurrentURI() on the same tick anymore. Can we just merge this with SessionStore:restoreHistory?

@@ +2739,5 @@
>    restoreHistory: function (window, tab, tabData, restoreImmediately) {
>      let browser = tab.linkedBrowser;
>      let history = browser.webNavigation.sessionHistory;
>  
> +    browser.messageManager.sendAsyncMessage("SessionStore:restoreHistory", {});

Nit: do we need to pass an empty object?

@@ +2801,5 @@
> +
> +    let afterRestoreTabContent = ([epoch, didStartLoad]) => {
> +      if (epoch != browser.__SS_restoreEpoch) {
> +        // We don't want restoreNextTab to do any more work, so return true.
> +        return true;

I wonder if we could, instead of letting restoreTabContent() return a boolean/promise, kick off next restores from receiveMessage() if (this._tabsRestoringCount < MAX_CONCURRENT_TAB_RESTORES)? We could just call this.restoreNextTab() again which is just a no-op if we would exceed the concurrency limit. This would remove the need for the somewhat awkward promise hassle here I hope.

@@ +2850,5 @@
>      if (tab) {
> +      this.restoreTabContent(tab).then((didStartLoad) => {
> +        // If we don't start a load in the restored tab (eg, no entries) then we
> +        // want to attempt to restore the next tab.
> +        if (!didStartLoad)

(see above)

	Tim Taubert [:ttaubert] (inactive)
	Comment 9 • 11 years ago

Comment on attachment 8337158 [details] [diff] [review]
add-wp-callback

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

(In reply to Bill McCloskey (:billm) from comment #3)
> I tried to have the tests listen on SessionStore:restoreTabContentComplete
> themselves. However, their listener fires after SessionStore.jsm's listener,
> and the tests want to go first.

Which tests exactly are that? It would be great to maybe find a better way to test that?

	Tim Taubert [:ttaubert] (inactive)
	Comment 10 • 11 years ago

Comment on attachment 8337158 [details] [diff] [review]
add-wp-callback

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +675,5 @@
> +          }
> +
> +          // browser.__SS_restoreState might be something other than
> +          // TAB_STATE_RESTORING if the onRestored callback changed
> +          // the state (by calling setWindowState for example).

Should we support that? Shouldn't we rather fix the test(s)?

	Tim Taubert [:ttaubert] (inactive)
	Comment 11 • 11 years ago

Comment on attachment 8337160 [details] [diff] [review]
wp-test-changes

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

Cancelling for now until we have a better idea of how to fix those tests.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 12 • 11 years ago

(In reply to Tim Taubert [:ttaubert] from comment #10)
> Comment on attachment 8337158 [details] [diff] [review]
> add-wp-callback
> 
> Review of attachment 8337158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +675,5 @@
> > +          }
> > +
> > +          // browser.__SS_restoreState might be something other than
> > +          // TAB_STATE_RESTORING if the onRestored callback changed
> > +          // the state (by calling setWindowState for example).
> 
> Should we support that? Shouldn't we rather fix the test(s)?

browser_586068-window_state_override.js is one such test. Doing setWindowState from within the progress listener is pretty central to the test, so I don't know how we would fix it.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 13 • 11 years ago

(In reply to Tim Taubert [:ttaubert] from comment #9)
> Comment on attachment 8337158 [details] [diff] [review]
> add-wp-callback
> 
> Review of attachment 8337158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Bill McCloskey (:billm) from comment #3)
> > I tried to have the tests listen on SessionStore:restoreTabContentComplete
> > themselves. However, their listener fires after SessionStore.jsm's listener,
> > and the tests want to go first.
> 
> Which tests exactly are that? It would be great to maybe find a better way
> to test that?

Mostly the tests that start with browser_586068*. These tests use the progress listener code and they get passed in a count of how many tabs are in the NEEDS_RESTORE and RESTORING states. If I move the callback, then a tab that they expected to be RESTORING will already be completed. I'm not sure what the best way is to deal with this. Perhaps you have ideas?

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 14 • 11 years ago

Thanks for the great suggestions. I've made all these changes.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 15 • 11 years ago

When I removed the setTimeout call in between restoreTabs and restoreHistory, I got some test failures. The main problem now is that SSWindowStateReady now fires before calls like functions like duplicateTab return. This causes a few issues, which I addressed by changing the tests.

1. Tests that were saving the tab returned from duplicateTab and using it in the SSWindowStateReady callback don't work anymore.

2. Tests calling removeTab from SSWindowStateReady cause duplicateTab to break when the listener returns and the tab is gone.

3. I ran into the same test failure as you did in bug 865674 (for test browser_588426.js). I just copied what you did there.

In general I'm unsure of when SSWindowStateReady is even supposed to fire. The semantics aren't really defined anywhere. Luckily not many add-ons use it.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 16 • 11 years ago

Here's a rolled up patch that applies on top of 8648aa476eef.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 18 • 11 years ago

Review ping?

	Tim Taubert [:ttaubert] (inactive)
	Comment 19 • 11 years ago

I'm on it. Will take some more time to think about a few edge cases but you will have it tonight or tomorrow.

	Tim Taubert [:ttaubert] (inactive)
	Comment 20 • 11 years ago

Comment on attachment 8342812 [details] [diff] [review]
restore-via-msgs v2

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

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +16,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
>  Cu.import("resource://gre/modules/Timer.jsm", this);
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "ContentRestore",
> +  "resource:///modules/sessionstore/ContentRestore.jsm");

I think we can just use Cu.import() and move that to the |new ContentRestore| line below.

@@ +85,5 @@
> +        // If we're in the process of restoring, this load may signal
> +        // the end of the restoration.
> +        let epoch = gContentRestore.getRestoreEpoch();
> +        if (epoch && event.originalTarget && event.originalTarget.defaultView &&
> +            event.originalTarget.defaultView == event.originalTarget.defaultView.top) {

Maybe:

let window = event.originalTarget && event.originalTarget.defaultView;
if (epoch && window && window == window.top) {

@@ +131,5 @@
>    },
>  
> +  receiveMessage: function ({name, data}) {
> +    let id = data.id || 0;
> +    let epoch;

Looks like the declaration of |epoch| can be moved below?

@@ +171,5 @@
> +        };
> +
> +        // We need to pass the value of didStartLoad back to SessionStore.jsm.
> +        let didStartLoad = gContentRestore.restoreTabContent(finishCallback);
> +        sendAsyncMessage(name, {id: id, data: [epoch, didStartLoad]});

How about sending data like {id: id, data: {epoch: epoch, didStartLoad: didStartLoad}}?

::: browser/components/sessionstore/src/ContentRestore.jsm
@@ +7,5 @@
> +this.EXPORTED_SYMBOLS = ["ContentRestore"];
> +
> +const Cu = Components.utils;
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;

Nit: we don't seem to use Cc.

@@ +288,5 @@
> +   * resetRestore.
> +   */
> +  purgeSessionHistory: function () {
> +    this._tabData = null;
> +  },

The loop sending "SessionStore:purgeSessionHistory" does also call _resetTabRestoringState() if the tab is currently in some restore state. Isn't that and the fact that purgeSessionHistory() is just a subset of resetRestore() enough to get rid of that function completely?

@@ +297,5 @@
> +   *
> +   * This function is called externally (if a restore is canceled) and
> +   * internally (when the loads for a restore have finished). In the latter
> +   * case, it's called before restoreDocument, so it cannot clear
> +   * _restoringDocuments.

Nit: "... it cannot clear _tabData."

@@ +354,5 @@
> +  OnHistoryReload: function(reloadURI, reloadFlags) {
> +    this.callback();
> +
> +    // Cancel the load.
> +    return false;

this.callback() sends an async message and we wait for an async message in return. After that ping pong we remove the history listener. We should make sure to call the callback only once no matter how often I manage to kick off a reload until the message ping pong has finished.

OTOH we could also kick off a restore from the content script side maybe? It would override the max_concurrent limit anyway. We could use a sync message to notify the parent process immediately.

@@ +391,5 @@
> +      this.callback();
> +    }
> +  },
> +
> +  onLocationChange: function(webProgress, request, location, flags) {},

Nit: we can omit the arguments here.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2827,5 @@
> +
> +        // If we don't start a load in the restored tab (eg, no entries) then we
> +        // want to attempt to restore the next tab.
> +        this.restoreNextTab();
> +      }

Instead of checking "didStartLoad" couldn't we just pretend the tab has loaded and send "SessionStore:restoreTabContentComplete" to achieve the same?

@@ +2829,5 @@
> +        // want to attempt to restore the next tab.
> +        this.restoreNextTab();
> +      }
> +
> +      return didStartLoad;

The return value seems unused.

@@ +2834,4 @@
>      }
>  
> +    let promise = Messenger.send(aTab, "SessionStore:restoreTabContent");
> +    promise.then(afterRestoreTabContent, Cu.reportError);

It would be great to not use "Messenger" here. I actually would like to get rid of that after moving shistory to broadcasting. Can't we just have a message listener for something like "SessionStore:restoreTabContentStarted"?

	Tim Taubert [:ttaubert] (inactive)
	Comment 21 • 11 years ago

Comment on attachment 8342814 [details] [diff] [review]
sync-test-changes

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

LGTM but I'll wait for possible main patch changes.

	Tim Taubert [:ttaubert] (inactive)
	Comment 22 • 11 years ago

(In reply to Bill McCloskey (:billm) from comment #12)
> > > +          // browser.__SS_restoreState might be something other than
> > > +          // TAB_STATE_RESTORING if the onRestored callback changed
> > > +          // the state (by calling setWindowState for example).
> > 
> > Should we support that? Shouldn't we rather fix the test(s)?
> 
> browser_586068-window_state_override.js is one such test. Doing
> setWindowState from within the progress listener is pretty central to the
> test, so I don't know how we would fix it.

I'm almost sure the test doesn't need to do that exactly on the callback and could use executeSoon() for that. I would try adapting the test. The small check doesn't hurt too much though, so I would appreciate if we could try to not include it but I would also not waste too much time on that.

(In reply to Bill McCloskey (:billm) from comment #13)
> > (In reply to Bill McCloskey (:billm) from comment #3)
> > > I tried to have the tests listen on SessionStore:restoreTabContentComplete
> > > themselves. However, their listener fires after SessionStore.jsm's listener,
> > > and the tests want to go first.
> > 
> > Which tests exactly are that? It would be great to maybe find a better way
> > to test that?
> 
> Mostly the tests that start with browser_586068*. These tests use the
> progress listener code and they get passed in a count of how many tabs are
> in the NEEDS_RESTORE and RESTORING states. If I move the callback, then a
> tab that they expected to be RESTORING will already be completed. I'm not
> sure what the best way is to deal with this. Perhaps you have ideas?

Ok, I understand. Can we, instead of having a callback property, maybe dispatch custom events when gDebuggingEnabled=true? That would be a better API for testing, IMO.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 23 • 11 years ago

Thanks, I've made these changes.

> @@ +297,5 @@
> > +   *
> > +   * This function is called externally (if a restore is canceled) and
> > +   * internally (when the loads for a restore have finished). In the latter
> > +   * case, it's called before restoreDocument, so it cannot clear
> > +   * _restoringDocuments.
> 
> Nit: "... it cannot clear _tabData."

No, it does clear _tabData. It can't clear _restoringDocument because that will be used later on by restoreDocument.

Regarding reloadRestoringTab:
> this.callback() sends an async message and we wait for an async message in
> return. After that ping pong we remove the history listener. We should make
> sure to call the callback only once no matter how often I manage to kick off
> a reload until the message ping pong has finished.
> 
> OTOH we could also kick off a restore from the content script side maybe? It
> would override the max_concurrent limit anyway. We could use a sync message
> to notify the parent process immediately.

I added a check on browser.__SS_restoreState in the message handler for SessionStore:reloadRestoringTab. That should ensure that we don't call restoreTabContent twice.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 24 • 11 years ago

You were right. I was able to use executeSoon in the test.

Rather than using events, I used notifyObservers. The problem with events is that they have to be for a specific browser or window, and these tests want to know about everything.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 26 • 11 years ago

I rebased over the scrolling patch. I was able to remove RestoreData since it's all saved in the content process now.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 27 • 11 years ago

This is another test change that's needed because SSWindowStateReady fires sooner now that the setTimeout has been removed from restoreTabs. In this particular case, it fires inside undoCloseWindow. If we wait for it later on, we'll just time out.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 28 • 11 years ago

I rebased this again. Tim, please do get to this soon. I spent two days tracking down bug 960246, which this patch somehow uncovered. The longer it takes to review, the more time I have to spend debugging stuff like that.

	Tim Taubert [:ttaubert] (inactive)
	Comment 29 • 11 years ago

Comment on attachment 8360639 [details] [diff] [review]
restore-via-msgs v5

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

Unfortunately, there are two failures when running this locally:

TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_581593.js | Test timed out
TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_687710_2.js | history.count - Got 0, expected 2

Other than that I didn't see any problems, sorry for letting you wait that long. Can you fold the existing patches and fix the two tests in a separate patch or is that harder for you to maintain? I had some trouble getting the patches in the right order.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +32,5 @@
>  Cu.import("resource:///modules/sessionstore/FrameTree.jsm", this);
>  let gFrameTree = new FrameTree(this);
>  
> +Cu.import("resource:///modules/sessionstore/ContentRestore.jsm", this);
> +let gContentRestore = new ContentRestore(this);

I think it would make sense to use XPCOMUtils.defineLazyGetter() here as this might never be used.

@@ +151,5 @@
> +      case "SessionStore:restoreHistory":
> +        let reloadCallback = () => {
> +          // Inform SessionStore.jsm about the reload. It will send
> +          // restoreTabContent in response.
> +          sendAsyncMessage("SessionStore:reloadRestoringTab", {epoch: data.epoch});

The tab isn't restoring at that point. Maybe call it ":reloadPendingTab"?

::: browser/components/sessionstore/src/ContentRestore.jsm
@@ +354,5 @@
> +  OnHistoryGotoIndex: function(index, gotoURI) { return true; },
> +  OnHistoryPurge: function(numEntries) { return true; },
> +
> +  OnHistoryReload: function(reloadURI, reloadFlags) {
> +    this.callback();

It seems like we should only ever call the callback once. If I have a pending tab and call browser.reload() five times the parent process receives five messages and will also send the same amount of messages back to the frame script. We could maybe just null the callback after calling it for the first time.

Also it seems possible that one could reload the browser and at the same time select the tab. This would as well cause two restoreDocument() messages to be sent. AFAICT this shouldn't matter because _restoringDocument is cleared anyway.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +671,5 @@
> +          }
> +        }
> +        break;
> +      case "SessionStore:restoreDocumentComplete":
> +        let tab = browser.__SS_restore_tab;

Nit: maybe define "tab" closer to where it is used.

	Tim Taubert [:ttaubert] (inactive)
	Comment 30 • 11 years ago

browser_687710_2.js should be easy to fix. It just needs to wait until the tab's history has been restored, which doesn't happen synchronously anymore.

	Tim Taubert [:ttaubert] (inactive)
	Comment 31 • 11 years ago

browser_581593.js should either listen for SSTabRestored and input in parallel and wait until both have been fired or we should just not listen for the input event. The original bug doesn't say anything about an input event and just waiting for the tab to be restored should really be enough here.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 32 • 11 years ago

(In reply to Tim Taubert [:ttaubert] from comment #29)
> Comment on attachment 8360639 [details] [diff] [review]
> restore-via-msgs v5
> 
> Review of attachment 8360639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unfortunately, there are two failures when running this locally:
> 
> TEST-UNEXPECTED-FAIL |
> browser/components/sessionstore/test/browser_581593.js | Test timed out
> TEST-UNEXPECTED-FAIL |
> browser/components/sessionstore/test/browser_687710_2.js | history.count -
> Got 0, expected 2
> 
> Other than that I didn't see any problems, sorry for letting you wait that
> long. Can you fold the existing patches and fix the two tests in a separate
> patch or is that harder for you to maintain? I had some trouble getting the
> patches in the right order.

Thanks very much Tim. These two failures should be fixed in the "other-tests" patch. The order of them is as follows:

other-tests
restore-via-msgs
add-wp-callback
wp-test-changes
sync-test-changes
rm-disable-for-e10s
more-sync-changes
panorama-fix

I'll post a rolled up patch in a few minutes.

> It seems like we should only ever call the callback once. If I have a pending tab and call
> browser.reload() five times the parent process receives five messages and will also send
> the same amount of messages back to the frame script. We could maybe just null the callback
> after calling it for the first time.
>
> Also it seems possible that one could reload the browser and at the same time select the
> tab. This would as well cause two restoreDocument() messages to be sent. AFAICT this
> shouldn't matter because _restoringDocument is cleared anyway.

In both cases, the parent guards against sending multiple restore messages to the child. The code is written so that browser.__SS_restoreState == TAB_STATE_NEEDS_RESTORE whenever we enter restoreTabContent. (Ideally, restoreTabContent would assert this, but we don't really have a way to do this in JS. However, you should be able to verify this is true at each callsite.) Then restoreTabContent sets browser.__SS_restoreState to TAB_STATE_RESTORING. So multiple restores on the same tab should not be possible.

In the first case you mentioned (browser.reload() called 5 times), the child would send 5 SessionStore:reloadRestoringTab messages to the parent. It would ignore all but the first one, since it checks browser.__SS_restoreState.

In the second case (browser.reload() called 5 times and the tab being selected by the user), we're also safe because onTabSelect checks browser.__SS_restoreState before calling restoreTabContent.

I can be defensive and null out the callback as well, but it doesn't really seem necessary to me. Please tell me what you prefer.

I'll make the other changes you requested. My latest try push still has some orange in mochitest-2, so I still have to figure that out. I suspect it's unrelated.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 33 • 11 years ago

Here's another rolled up patch. It's based on dd2cf81c56b7.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 34 • 11 years ago

I think I fixed the last remaining orange (it turned out to be related to bug 960246) so I just need to know what you want to do about the reload stuff.

	Tim Taubert [:ttaubert] (inactive)
	Comment 35 • 11 years ago

(In reply to Bill McCloskey (:billm) from comment #32)
> I can be defensive and null out the callback as well, but it doesn't really
> seem necessary to me. Please tell me what you prefer.

Fair enough. I don't think we should guard against this and agree that nulling out the callback isn't necessary here.

Can you please land this on fx-team or mark it as checkin-needed so that someone will land it on fx-team? Otherwise it takes quite some time for a roundtrip from inbound to fx-team :) Thanks for all the work you've done here!

	Tim Taubert [:ttaubert] (inactive)
	Comment 36 • 11 years ago

Or wait. If you have to land this together with all the other fixes for loadFrameScript() etc it probably doesn't make sense to make you wait until they have made their way to fx-team. In that case just push it all to inbound and ignore me :)

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 37 • 11 years ago

I landed on fx-team along with the dependent patches. The chance of a merge conflict for those patches is very low.

Main patch and all the test changes:
https://hg.mozilla.org/integration/fx-team/rev/22d173f73496

Enable in e10s:
https://hg.mozilla.org/integration/fx-team/rev/5d45e6b035a3

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 38 • 11 years ago

Also, here are my try pushes if any orange appears. The first one has a mochitest-2 problem that's fixed in the second one.

https://tbpl.mozilla.org/?tree=Try&rev=7a76f91ff749
https://tbpl.mozilla.org/?tree=Try&rev=7a9eeb92f997

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 39 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/22d173f73496
https://hg.mozilla.org/mozilla-central/rev/5d45e6b035a3

	u279076
	Comment 40 • 11 years ago

How are we tracking these e10s changes? Is there a whiteboard tag or something?