618151 - Overwriting state can lead to unrestored tabs

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Apparently browser.stop is async so we can still get load events from a tab that's being overwritten with a new state.

If a load event happens after we set __SS_data on the browser (in restoreHistoryPrecursor) but before we call restoreTab on that tab, we'll enter onTabLoad, where we delete __SS_data. __SS_data is used by restoreTab (and is expected to be there) so we then break.

I've somehow managed to reproduce this reliably in my test for bug 615394, so making that into a simpler test should be easy enough. Simply bailing out in onTabLoad if __SS_restoreState is truthy seems to do the trick. Patch sometime soon.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Other sources (read: gavin) say stop isn't async, but that load might still be
triggered after stop has been called. Regardless of how it's happening, it's happening.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

Reduced the tests from bug 615394 that was causing the issue. The test times out without the change.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Blocks a blocker so just making blocking explicit.

This is causing a consistent timeout with browser_600545.js... looking into it.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Fixed the timeout issue. We only want to bail out of onTabLoad if __SS_restoreState == TAB_STATE_NEEDS_RESTORE, not TAB_STATE_RESTORING. Fixed the timeout issue.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 498646 [details] [diff] [review]
Patch v0.2

r=me. file a bug on the unidentified-and-uncool load events that are happening after stop?

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/98b8d44e0096 and filed bug 621586.

Comment 7

[u279076]

Is there a way I can verify this bug?

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I haven't hit this in real life, but maybe calling setBrowserState while in the middle of restoring would do it. Otherwise the test is passing.