Closed Bug 1226333 Opened 4 years ago Closed 4 years ago

Add tests for async window flushing

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: mconley, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1171708 gave us asynchronous window flushing for window close. Some tests were written, but those tests failed intermittently. We should figure out why the tests fail intermittently and reland them.
Comment on attachment 8689689 [details]
MozReview Request: Bug 1226333 - Add tests for async window flushing. r=billm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25689/diff/1-2/
Attachment #8689689 - Attachment description: MozReview Request: Bug 1226333 - Add tests for async window flushing. r=billm → MozReview Request: Bug 1226333 - Add tests for async window flushing.
Comment on attachment 8689689 [details]
MozReview Request: Bug 1226333 - Add tests for async window flushing. r=billm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25689/diff/2-3/
Attachment #8689689 - Attachment description: MozReview Request: Bug 1226333 - Add tests for async window flushing. → MozReview Request: Bug 1226333 - Add tests for async window flushing. r=billm.
Finally got these to not fail intermittently. The key was to set the state in the parent to send the browser to example.com in test_remove_uninteresting_window (see the interdiff: https://reviewboard.mozilla.org/r/25689/diff/2-3/).

billm, you already signed off on this, but it got backed out due to the failures. Now that I've fixed them, still feel okay signing off on this?
Flags: needinfo?(wmccloskey)
Okay, I finally understand what's happening here.

My patch adds this pref, browser.sessionstore.debug.no_auto_updates, which tries to side-step potential races by having the parent ignore non-flush, non-final updates from the child.

The problem with that is that if a message comes up from the child and is ignored, *the child still thinks it sent it*, so further updates from the child (from a flush, for example) won't include the data that it thinks it had already sent.

I believe the solution is to move the browser.sessionstore.debug.no_auto_updates pref from SessionStore.jsm into content-sessionStore.js, where it will just prevent us from setting timeouts (and cancel pending timeouts) on the MessageQueue until re-enabled.
Flags: needinfo?(wmccloskey)
Comment on attachment 8689689 [details]
MozReview Request: Bug 1226333 - Add tests for async window flushing. r=billm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25689/diff/3-4/
Attachment #8689689 - Flags: review?(wmccloskey)
Comment on attachment 8689689 [details]
MozReview Request: Bug 1226333 - Add tests for async window flushing. r=billm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25689/diff/4-5/
Comment on attachment 8689689 [details]
MozReview Request: Bug 1226333 - Add tests for async window flushing. r=billm.

https://reviewboard.mozilla.org/r/25689/#review24559

Thanks for tracking this down!
Attachment #8689689 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/1a8e8ab6ee3d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.