Port Bug 531519 [getBrowserState sometimes returns two entries for one window] to Seamonkey

RESOLVED FIXED in seamonkey2.1a1

Status

SeaMonkey
Session Restore
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Misak Khachatryan, Assigned: Misak Khachatryan)

Tracking

Trunk
seamonkey2.1a1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.60 KB, patch
Misak Khachatryan
: review+
Misak Khachatryan
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
Created attachment 438335 [details] [diff] [review]
patch

From parent bug:

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.

I ran test on my Fedora 12, no failed test seen.
Attachment #438335 - Flags: superreview?(neil)
Attachment #438335 - Flags: review?(neil)

Comment 1

7 years ago
Comment on attachment 438335 [details] [diff] [review]
patch

>+      if (this._windows[ix]._restoring) // window data is still in _statesToRestore
>+        continue;
Does checking (this._windows[ix].__SS_restoreID in this._statesToRestore) work? (That would avoid adding the extra _restoring flag.)
(Assignee)

Comment 2

7 years ago
Created attachment 438368 [details] [diff] [review]
patch v2

Well, seems to work, but as our browser_526613.js didn't fail on my Fedora 12, I'm requesting feedback from  Dão Gottwald, maybe he can add useful comment here.
Attachment #438335 - Attachment is obsolete: true
Attachment #438368 - Flags: superreview?(neil)
Attachment #438368 - Flags: review?(neil)
Attachment #438368 - Flags: feedback?(dao)
Attachment #438335 - Flags: superreview?(neil)
Attachment #438335 - Flags: review?(neil)

Comment 3

7 years ago
Comment on attachment 438368 [details] [diff] [review]
patch v2

Let's prefer to go with this version if possible.
Attachment #438368 - Flags: superreview?(neil)
Attachment #438368 - Flags: superreview+
Attachment #438368 - Flags: review?(neil)
Attachment #438368 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Comment on attachment 438368 [details] [diff] [review]
patch v2

Looks wrong to me. __SS_restoreID is set on dom windows, this._windows[ix] is not a dom window.
Attachment #438368 - Flags: feedback?(dao) → feedback-
(Assignee)

Updated

7 years ago
Attachment #438368 - Attachment is obsolete: true
(Assignee)

Comment 5

7 years ago
Comment on attachment 438335 [details] [diff] [review]
patch

Thank You very much Dão, unobsoleting previous patch and setting r+ and sr+, as discussed with Neil on IRC.
Attachment #438335 - Attachment is obsolete: false
Attachment #438335 - Flags: superreview+
Attachment #438335 - Flags: review+
(Assignee)

Comment 6

7 years ago
checking ping
(Assignee)

Comment 7

7 years ago
Pushed http://hg.mozilla.org/comm-central/rev/59177a9718ca
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Target Milestone: --- → seamonkey2.1a1
(Assignee)

Comment 8

7 years ago
Hmm, looks like i checked in wrong patch ...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

7 years ago
Backed out and pushed right patch:
http://hg.mozilla.org/comm-central/rev/7db435639b90
http://hg.mozilla.org/comm-central/rev/5fccef727c57
http://hg.mozilla.org/comm-central/rev/55ef3b8211e8
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.