TabState.collect spawns a task to collect tab data. While this is happening, we can restore a tab. Right now there are a few problems if that happens.

If we restore in the middle of the TabState.collect, here's what seems to happen: 1. We might collect some bad data for history, page style, etc. since those can happen before the restore. 2. However, this._collectBaseTabData will collect good data since it will return browser.__SS_data, which is what we're trying to restore. 3. However, we'll then overwrite the results of _collectBaseTabData with the bad data we collected for history and stuff. This data will then go in the cache. This patch cancels any ongoing collections when we start session restore. It should fix the problem.

There's another problem, though. When we return __SS_data from _collectBaseTabData, we return a reference to it. Then we we fill in the (bad) values we collected for history and pageStyle and stuff, we're directly changing browser.__SS_data. So we'll end up restoring the wrong thing. I think it would be a lot simpler to make a copy of __SS_data when we return it from _collectBaseTabData. That way we don't have to worry about what happens to it later. This should be a fairly uncommon path (only happens when collection and restoration happen simultaneously) so the performance difference shouldn't matter much.

This test fails without the two patches above.

(In reply to Bill McCloskey (:billm) from comment #3) > Created attachment 821295 [details] [diff] [review] > test-async-collect-restore > > This test fails without the two patches above. I forgot: I should mention why I changed the timeout in waitForSaveState. I was occasionally getting failures because of timeout. I didn't look too closely at why it was happening, but I'm guessing that it was a GC or something. Given how janky debug builds can be, 100ms for a timeout doesn't seem to realistic to me.

Comment on attachment 821292 [details] [diff] [review] cancel-pending-collections Review of attachment 821292 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionStore.jsm @@ +4315,5 @@ > + * > + * @param tab > + * tabbrowser tab > + */ > + cancelPendingCollections: function (tab) { The name is a little misleading as we don't actually cancel any pending collections. Those collections will still resolve their promises, we just won't put data into the cache.

Comment on attachment 821294 [details] [diff] [review] aliasing-fix Now that's a nice find. I'm quite confident this will fix bug 922427 for us - which I was investigating lately and suspected an issue similar to that. With the patch applied I can't reproduce that anymore, thanks!

Comment on attachment 821295 [details] [diff] [review] test-async-collect-restore Review of attachment 821295 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/test/browser.ini @@ +149,5 @@ > [browser_739805.js] > [browser_819510_perwindowpb.js] > [browser_833286_atomic_backup.js] > [browser_916390_form_data_loss.js] > +[browser_930202.js] Can you please give the test a more descriptive name? You can put the bug number in the head of the test somewhere.

Once this lands, please can you also try re-enabling these two disabled tests? * Bug 921984 * Bug 922427

Comment on attachment 821294 [details] [diff] [review] aliasing-fix Review of attachment 821294 [details] [diff] [review]: ----------------------------------------------------------------- Stealing, Steven said he might not be available most of this week. ::: browser/components/sessionstore/src/SessionStore.jsm @@ +4365,5 @@ > return tabData; > } > if (browser.__SS_data && browser.__SS_tabStillLoading) { > // use the data to be restored when the tab hasn't been completely loaded > + tabData = JSON.parse(JSON.stringify(browser.__SS_data)); Can you please add a comment here that explains what we're doing here and why?

Comment on attachment 821295 [details] [diff] [review] test-async-collect-restore Review of attachment 821295 [details] [diff] [review]: ----------------------------------------------------------------- r=me with a better name for the test file.

Comment on attachment 821292 [details] [diff] [review] cancel-pending-collections Review of attachment 821292 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionStore.jsm @@ +4315,5 @@ > + * > + * @param tab > + * tabbrowser tab > + */ > + cancelPendingCollections: function (tab) { 'untrack' sounds weird. 'discard' is only somewhat right as we discard the data and will not put it in the cache but we still use it to resolve the promise. OTOH, maybe we shouldn't spend too much time on naming this. It will be going away when we broadcast session state anyway :)

I changed the name to dropPendingCollections. Hope that's okay. https://hg.mozilla.org/integration/mozilla-inbound/rev/01488c58bca1 https://hg.mozilla.org/integration/mozilla-inbound/rev/680347522c38 https://hg.mozilla.org/integration/mozilla-inbound/rev/6e895ec63744

Backed out the tests in https://hg.mozilla.org/integration/mozilla-inbound/rev/2e81a5d9952d - three out of five OS X 10.7 debug runs agreed, https://tbpl.mozilla.org/php/getParsedLog.php?id=30059488&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=30060992&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=30062241&tree=Mozilla-Inbound, that magic timeout values are a bad idea, and 1000ms isn't magic for them.

https://hg.mozilla.org/mozilla-central/rev/01488c58bca1 https://hg.mozilla.org/mozilla-central/rev/680347522c38

(In reply to Bill McCloskey (:billm) from comment #0) > TabState.collect spawns a task to collect tab data. While this is happening, > we can restore a tab. Right now there are a few problems if that happens. How can QA trigger this situation reliably in order to reproduce this bug?

I don't remember. I don't think it's useful to verify this bug. We've already rewritten the code involved.