531519 - getBrowserState sometimes returns two entries for one window

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Dão Gottwald [:dao]]

Attachment: problematic browser state

http://hg.mozilla.org/mozilla-central/rev/4a7f5376270a exposed a constant failure on all platforms:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_526613.js | number of open browser windows according to getBrowserState - Got 3, expected 2

This happens even when running browser_526613.js alone.

I think the second window (from _windows) and the third one (from _statesToRestore) are the same, but I'm not sure.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch for discussion

I didn't want to disable the test, so I pushed this in order to fix the failure:
http://hg.mozilla.org/mozilla-central/rev/8f943a699370

I don't think this is the right fix, though. It seems like _windows is in a state that it shouldn't be in in the first place.

Comment 2

[Simon Bünzli]

Comment on attachment 414913 [details] [diff] [review]
patch for discussion

This looks about right, although slightly fragile. I'd prefer it if windows that still have their actual state stored in _statesToRestore had their tracked data in _windows marked with a flag that's added in sss_onLoad and removed right before the data is deleted from _statesToRestore (or something to that end).

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

yeah, this makes more sense...

Comment 4

[Simon Bünzli]

Comment on attachment 415142 [details] [diff] [review]
patch

>+    if (aWindow.__SS_restoreID)

Please make that !this._isWindowLoaded(aWindow) like we do everywhere else. r+=me with this change.

Comment 5

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/8d196765d647

Comment 6

[u279076]

Are there steps to reproduce this bug from a QA perspective (i.e. How can I
verify?)?