930202 - Restoring during async collection can cause problems

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: cancel-pending-collections

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.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: aliasing-fix

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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: test-async-collect-restore

This test fails without the two patches above.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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 5

[Tim Taubert [:ttaubert] (inactive)]

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 6

[Tim Taubert [:ttaubert] (inactive)]

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 7

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 8

[Ed Morley [:emorley]]

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

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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 10

[Tim Taubert [:ttaubert] (inactive)]

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 11

[Tim Taubert [:ttaubert] (inactive)]

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 :)

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 13

[Phil Ringnalda (:philor)]

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.

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Ioana (away)]

(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?

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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