Closed Bug 930202 Opened 6 years ago Closed 6 years ago

Restoring during async collection can cause problems

Categories

(Firefox :: Session Restore, defect)

27 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: billm, Assigned: billm)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files)

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.
Attachment #821292 - Flags: review?(smacleod)
Attached patch aliasing-fixSplinter Review
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.
Attachment #821294 - Flags: review?(smacleod)
This test fails without the two patches above.
Attachment #821295 - Flags: review?(smacleod)
(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.
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → 27 Branch
Blocks: 922427
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?
Attachment #821294 - Flags: review?(smacleod) → review+
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.
Attachment #821295 - Flags: review?(smacleod) → review+
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 :)
Attachment #821292 - Flags: review?(smacleod) → review+
https://hg.mozilla.org/mozilla-central/rev/01488c58bca1
https://hg.mozilla.org/mozilla-central/rev/680347522c38
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
(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?
Flags: needinfo?(wmccloskey)
I don't remember. I don't think it's useful to verify this bug. We've already rewritten the code involved.
Flags: needinfo?(wmccloskey)
Whiteboard: [qa-]
Depends on: 972243
You need to log in before you can comment on or make changes to this bug.