Closed Bug 1171708 Opened 10 years ago Closed 10 years ago

[e10s] Stop using CPOWs for window closing

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Version:
34 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 45
Tracking Flags:
Tracking Status
e10s m8+ ---
firefox45 --- fixed

People

(Reporter: billm, Assigned: mconley)

References

Details

Attachments

(9 files)

MozReview Request: Bug 1171708 - Add flushWindow method to TabStateFlusher. r?billm
40 bytes, text/x-review-board-request
billm
: review+
Details
MozReview Request: Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r?billm
40 bytes, text/x-review-board-request
billm
: review+
Details
MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm
40 bytes, text/x-review-board-request
billm
: review+
Details
MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm
40 bytes, text/x-review-board-request
billm
: review+
Details
MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm
40 bytes, text/x-review-board-request
billm
: review+
Details
MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm
40 bytes, text/x-review-board-request
billm
: review+
Details
MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm
40 bytes, text/x-review-board-request
billm
: review+
Details
MozReview Request: Bug 1171708 - Add tests for async window flushing. r=billm
40 bytes, text/x-review-board-request
billm
: review+
Details
Bugnotes
12.89 KB, application/zip
Details
I don't think we have a bug filed for this. I just want something to refer to. I think these are the last uses of CPOWs in session store. We can break this into separate bugs for window closing and quitting if it helps. I think this is important because, when a tab hangs, users often respond by trying to close the window or quit so that they can start over.
Yes, thanks for filing this. When bug 1167508 landed this will indeed be the last step to get rid of CPOWs in sessionstore. Splitting this might make sense, I'll think about it a little more.
Assignee: nobody → ttaubert
tracking-e10s: --- → m8+
I'm not sure if it's useful, but I should mention that we have some platform code that sends a "content-child-shutdown" observer notification right before a content process is about to exit. Messages sent at this time will be received by the chrome process and will still have time to write things to disk (as long as they do it right away).
Blocks: 1109869
Summary: [e10s] Stop using CPOWs for window/application closing → [e10s] Stop using CPOWs for window closing
Unassigning for the time being.
Assignee: ttaubert → nobody
Assignee: nobody → wmccloskey
Blocks: 1195295
Assignee: wmccloskey → mconley
Bug 1171708 - [WIP] Have SessionStore asynchronous collect window information on close. r?billm
Comment on attachment 8684318 [details] MozReview Request: Bug 1171708 - Add flushWindow method to TabStateFlusher. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24535/diff/1-2/
Attachment #8684318 - Flags: review?(wmccloskey)
Comment on attachment 8684318 [details] MozReview Request: Bug 1171708 - Add flushWindow method to TabStateFlusher. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24535/diff/2-3/
Attachment #8684318 - Flags: review?(wmccloskey)
Comment on attachment 8684318 [details] MozReview Request: Bug 1171708 - Add flushWindow method to TabStateFlusher. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24535/diff/2-3/
Attachment #8684318 - Attachment description: MozReview Request: Bug 1171708 - [WIP] Have SessionStore asynchronous collect window information on close. r?billm → MozReview Request: Bug 1171708 - Add flushWindow method to TabStateFlusher. r?billm
Attachment #8684318 - Flags: review?(wmccloskey)
Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r?billm We were using tagName before, which is fine for the dynamically created browsers in new tabs, but not fine for the initial browser tab, which has a tagName of "xul:browser" instead of "browser". Using localName makes sure that we don't get the XML namespace included with the node name.
Attachment #8684339 - Flags: review?(wmccloskey)
Bug 1171708 - [WIP] Have SessionStore asynchronous collect window information on close. r?billm
Comment on attachment 8684318 [details] MozReview Request: Bug 1171708 - Add flushWindow method to TabStateFlusher. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24535/diff/3-4/
Comment on attachment 8684339 [details] MozReview Request: Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24537/diff/1-2/
Comment on attachment 8684340 [details] MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24539/diff/1-2/
Comment on attachment 8684318 [details] MozReview Request: Bug 1171708 - Add flushWindow method to TabStateFlusher. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24535/diff/4-5/
Attachment #8684318 - Flags: review?(wmccloskey)
Comment on attachment 8684339 [details] MozReview Request: Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24537/diff/2-3/
Attachment #8684339 - Flags: review?(wmccloskey)
Comment on attachment 8684340 [details] MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24539/diff/2-3/
Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm
Hi Mike, I'm worried about the code here: https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1229-1255 It's examining the tab state that's currently in the cache at the time the window is closed and making decisions based on it (mostly whether to save the window in this._closedWindows). If we get new data in the update message, I think that decision might change. Tim's patch for tab closing accounts for those changes when the update message is received: https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#730-753 I think your patch needs to do something like that too.
Comment on attachment 8685450 [details] MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24787/diff/1-2/
Attachment #8685451 - Attachment description: MozReview Request: Potentially fix last intermittent orange → MozReview Request: Bug 1171708 - Add windowClosed helper to BrowserTestUtils. r?billm
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24789/diff/1-2/
Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r?billm
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24789/diff/2-3/
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24863/diff/1-2/
Comment on attachment 8684340 [details] MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24539/diff/3-4/
Attachment #8684340 - Attachment description: MozReview Request: Bug 1171708 - [WIP] Have SessionStore asynchronous collect window information on close. r?billm → MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm
Comment on attachment 8685450 [details] MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24787/diff/2-3/
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24789/diff/3-4/
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24863/diff/2-3/
Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r?billm
Comment on attachment 8684318 [details] MozReview Request: Bug 1171708 - Add flushWindow method to TabStateFlusher. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24535/diff/4-5/
Attachment #8684318 - Flags: review?(wmccloskey)
Attachment #8684339 - Flags: review?(wmccloskey)
Comment on attachment 8684339 [details] MozReview Request: Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24537/diff/2-3/
Comment on attachment 8685450 [details] MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24787/diff/3-4/
Attachment #8685450 - Flags: review?(wmccloskey)
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24789/diff/4-5/
Attachment #8685451 - Flags: review?(wmccloskey)
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24863/diff/3-4/
Attachment #8685630 - Flags: review?(wmccloskey)
Comment on attachment 8686261 [details] MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24979/diff/1-2/
Attachment #8686261 - Flags: review?(wmccloskey)
Comment on attachment 8684340 [details] MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24539/diff/4-5/
Attachment #8684340 - Flags: review?(wmccloskey)
Comment on attachment 8685450 [details] MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24787/diff/3-4/
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24789/diff/4-5/
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24863/diff/3-4/
Comment on attachment 8686261 [details] MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24979/diff/1-2/
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24789/diff/5-6/
Attachment #8685451 - Attachment description: MozReview Request: Bug 1171708 - Add windowClosed helper to BrowserTestUtils. r?billm → MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24863/diff/4-5/
Comment on attachment 8686261 [details] MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24979/diff/2-3/
Bug 1171708 - Add tests for async window flushing. r?billm
Attachment #8686825 - Flags: review?(wmccloskey)
Sorry if you were already in the midst of reviewing that stack, billm. If so, I only changed the windowClosed BrowserTestUtils changeset, and then added a new test for that corner case you were talking about yesterday. Here's the interdiff: https://reviewboard.mozilla.org/r/24533/diff/9-10/
Comment on attachment 8684318 [details] MozReview Request: Bug 1171708 - Add flushWindow method to TabStateFlusher. r?billm https://reviewboard.mozilla.org/r/24535/#review22707
Attachment #8684318 - Flags: review?(wmccloskey) → review+
Comment on attachment 8684339 [details] MozReview Request: Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r?billm https://reviewboard.mozilla.org/r/24537/#review22317 ::: browser/components/sessionstore/SessionStore.jsm:880 (Diff revision 2) > - if (target.tagName == "browser" && target.frameLoader && target.permanentKey) { > + if (target.localName == "browser" && target.frameLoader && target.permanentKey) { Should we check the namespace too?
Attachment #8684339 - Flags: review?(wmccloskey) → review+
Comment on attachment 8684340 [details] MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm https://reviewboard.mozilla.org/r/24539/#review22319 This is looking really good, but needs a bit more work. The design is cleaner than what we have for tab closing. We should have a follow-up to make the tab closing code look more like this code. ::: browser/components/sessionstore/SessionStore.jsm:661 (Diff revision 3) > // have no tab assigned, e.g. the ones that preload about:newtab pages. Maybe update this comment and the one for NOTAB_MESSAGES. ::: browser/components/sessionstore/SessionStore.jsm:757 (Diff revision 5) > + if (this._closedWindowTabs.has(browser.permanentKey)) { > + let tabData = this._closedWindowTabs.get(browser.permanentKey); > + TabState.copyFromCache(browser, tabData); > + // For closed window tabs, we really only care about the last flush > + // we requested, since we will be detaching the message listeners for > + // the window as soon as the last flush update comes in. > + this._closedWindowTabs.delete(browser.permanentKey); > + } This code doesn't feel right to be. Is it really needed? Couldn't the flushWindow callback instead call copyFromCache itself on every browser in the window? It has all the data it needs already I think, so we could get rid of \_closedWindowTabs entirely. If this code *is* needed, then I think we only want to do this is aMessage.data.isFinal. Otherwise a random update message that was sent before we asked the window to be closed, but received after closing, would trigger this code too early. Another issue is that this code seems to happen to late. We have already resolved the flush promise at this point. So if this is the last tab in the window then we've already run the flushWindow callback. We have already made the decision about whether to save the window--before this copyFromCache call. ::: browser/components/sessionstore/SessionStore.jsm:1352 (Diff revision 5) > + this._closedWindows.unshift(winData); We should insert the window in the list so that it's sorted in order of closedAt. We do that for tab closing. I think you can just copy the sort call we use for that. ::: browser/components/sessionstore/SessionStore.jsm:1706 (Diff revision 5) > + this._closedWindowTabs.delete(closedTab.permanentKey); I don't understand why this is needed. It looks like removeClosedTabData is only called for tabs that were closed individually. I don't see how we could get a call to, e.g., undoClosedTab for a tab in a window that was closed.
Attachment #8684340 - Flags: review?(wmccloskey)
Comment on attachment 8685450 [details] MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm https://reviewboard.mozilla.org/r/24787/#review22711 Generally this looks fine. ::: browser/components/sessionstore/test/browser_354894_perwindowpb.js:196 (Diff revision 4) > + TabStateFlusher.flushWindow(newWin).then(() => { Why is this needed? ::: browser/components/sessionstore/test/browser_394759_behavior.js:39 (Diff revision 4) > - waitForExplicitFinish(); > + yield TabStateFlusher.flushWindow(win); Seems like this line shouldn't be needed if BTU.closeWindow is waiting for the last update. ::: browser/components/sessionstore/test/browser_464199.js:72 (Diff revision 4) > + TabStateFlusher.flushWindow(newWin).then(() => { What's this for? I would expect that site forgetting happens in the parent, so we shouldn't need to sync up here. ::: browser/components/sessionstore/test/browser_597071.js:31 (Diff revision 4) > - yield promiseWindowClosed(win); > + yield TabStateFlusher.flushWindow(win); Again, it seems like BTU.closeWindow should be sufficient. ::: browser/components/sessionstore/test/browser_broadcast.js:77 (Diff revision 4) > - yield closeWindow(win); > + yield TabStateFlusher.flushWindow(win); Same remark as above. ::: browser/components/sessionstore/test/browser_cleaner.js:97 (Diff revision 4) > + yield TabStateFlusher.flushWindow(newWin); I think the state should always be up-to-date after promiseTabRestored (or any restoration operation).
Attachment #8685450 - Flags: review?(wmccloskey) → review+
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm https://reviewboard.mozilla.org/r/24789/#review22713
Attachment #8685451 - Flags: review?(wmccloskey) → review+
https://reviewboard.mozilla.org/r/24787/#review22711 > Why is this needed? I guess it's really not, since I rewrote the test in a later patch. Well spotted!
Comment on attachment 8684318 [details] MozReview Request: Bug 1171708 - Add flushWindow method to TabStateFlusher. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24535/diff/4-5/
Comment on attachment 8684339 [details] MozReview Request: Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24537/diff/2-3/
Attachment #8684340 - Flags: review?(wmccloskey)
Comment on attachment 8684340 [details] MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24539/diff/5-6/
Comment on attachment 8685450 [details] MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24787/diff/4-5/
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24789/diff/6-7/
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24863/diff/5-6/
Comment on attachment 8686261 [details] MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24979/diff/3-4/
Comment on attachment 8686825 [details] MozReview Request: Bug 1171708 - Add tests for async window flushing. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25091/diff/1-2/
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24789/diff/6-7/
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24863/diff/5-6/
Comment on attachment 8686261 [details] MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24979/diff/3-4/
Comment on attachment 8686825 [details] MozReview Request: Bug 1171708 - Add tests for async window flushing. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25091/diff/1-2/
Comment on attachment 8684339 [details] MozReview Request: Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24537/diff/3-4/
Comment on attachment 8684340 [details] MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24539/diff/6-7/
Comment on attachment 8685450 [details] MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24787/diff/5-6/
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24789/diff/7-8/
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24863/diff/6-7/
Comment on attachment 8686261 [details] MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24979/diff/4-5/
Comment on attachment 8686825 [details] MozReview Request: Bug 1171708 - Add tests for async window flushing. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25091/diff/2-3/
https://reviewboard.mozilla.org/r/24537/#review22317 > Should we check the namespace too? Unfortunately, we don't get the prefix for dynamically created browsers. We could add it I suppose, but I don't really think it's worth the effort, TBH. Feel free to push back if you disagree.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #75) > https://reviewboard.mozilla.org/r/24537/#review22317 > > > Should we check the namespace too? > > Unfortunately, we don't get the prefix for dynamically created browsers. We > could add it I suppose, but I don't really think it's worth the effort, TBH. > Feel free to push back if you disagree. Do you mean that element.namespaceURI is null or something? That would be really bad if that's true.
(In reply to Bill McCloskey (:billm) from comment #76) > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #75) > > https://reviewboard.mozilla.org/r/24537/#review22317 > > > > > Should we check the namespace too? > > > > Unfortunately, we don't get the prefix for dynamically created browsers. We > > could add it I suppose, but I don't really think it's worth the effort, TBH. > > Feel free to push back if you disagree. > > Do you mean that element.namespaceURI is null or something? That would be > really bad if that's true. No, sorry - that's correctly set. It's the prefix[1] property that is null. Would you prefer that I compare the namespaceURI against the http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul namespace? [1]: https://developer.mozilla.org/en-US/docs/Web/API/Node/prefix
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #77) > Would you prefer that I compare the namespaceURI against the > http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul namespace? Yes, please do. It can't hurt.
Attachment #8685630 - Flags: review?(wmccloskey) → review+
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm https://reviewboard.mozilla.org/r/24863/#review22777 Just trying to get a better sense of when we really need to flush... ::: browser/components/sessionstore/test/browser_354894_perwindowpb.js:164 (Diff revision 7) > - // Reset the window type > + return TabStateFlusher.flushWindow(win); Doesn't the use of BTU.windowClosed in closeWindowForRestoration obviate the need for this flush? It seems like this window is getting closed before we ever need to guarantee that the data is correct. Maybe I'm missing something though. ::: browser/components/sessionstore/test/browser_354894_perwindowpb.js:200 (Diff revision 7) > - // Close the window > + * 2. Add some tabs Ridiculous nit: there are extra spaces after the period in some of these items. ::: browser/components/sessionstore/test/browser_354894_perwindowpb.js:313 (Diff revision 7) > - // close the window > + yield TabStateFlusher.flushWindow(popup2); I think this can be removed since you're waiting for popup2 to be closed a few lines below. ::: browser/components/sessionstore/test/browser_354894_perwindowpb.js:426 (Diff revision 7) > - browserWindowsCount([0, 1], "browser windows while running testOpenCloseRestoreFromPopup"); > + let { winstates } = getBrowserWindowsCount(); The old test also checked that there were no open browser windows. Why not check that too? ::: browser/components/sessionstore/test/browser_354894_perwindowpb.js:429 (Diff revision 7) > + let newWinPromise = BrowserTestUtils.domWindowOpened(); This doesn't seem to be used.
Attachment #8686261 - Flags: review?(wmccloskey) → review+
Comment on attachment 8686261 [details] MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm https://reviewboard.mozilla.org/r/24979/#review22791 ::: browser/components/sessionstore/test/browser_490040.js:46 (Diff revision 5) > - testWithState(state); > + yield pushPrefs(["browser.sessionstore.max_windows_undo", I don't think this is correct. We're potentially adding multiple windows to the saved closed tabs (although it looks like shouldBeAdded is true in only one case, we shouldn't assume that). I think we should set this pref before each loop iteration (and re-read the current number of closed windows).
Comment on attachment 8686825 [details] MozReview Request: Bug 1171708 - Add tests for async window flushing. r=billm https://reviewboard.mozilla.org/r/25091/#review22793 ::: browser/components/sessionstore/test/browser_async_window_flushing.js:36 (Diff revision 3) > + yield promiseContentMessage(browser, "ss-test:OnHistoryReplaceEntry"); What's this for? ::: browser/components/sessionstore/test/browser_async_window_flushing.js:58 (Diff revision 3) > + is(currentClosedWindows, initialClosedWindows, > + "We should not have added the window to the closed windows array"); This seems really racy. If the window happens to have started loading example.com at this point and reported it back to the parent, we will save this tab, won't we?
Attachment #8686825 - Flags: review?(wmccloskey)
https://reviewboard.mozilla.org/r/24863/#review22777 > Doesn't the use of BTU.windowClosed in closeWindowForRestoration obviate the need for this flush? It seems like this window is getting closed before we ever need to guarantee that the data is correct. Maybe I'm missing something though. I guess I just wanted to make sure that the window was truly ready before we start the test, so that any tests that use setupTest don't have to worry about potentially flushing the window. But yeah, we can remove it. No biggie.
https://reviewboard.mozilla.org/r/25091/#review22793 > What's this for? This is to ensure that the content has actually started loading the example.com page before continuing the test. > This seems really racy. If the window happens to have started loading example.com at this point and reported it back to the parent, we will save this tab, won't we? As discussed in IRC, I've added a testing pref that disables the automatic updates coming from the child, so that we don't have the case where an update might come up while we wait for the domwindowclosed notification.
Comment on attachment 8684318 [details] MozReview Request: Bug 1171708 - Add flushWindow method to TabStateFlusher. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24535/diff/5-6/
Comment on attachment 8684339 [details] MozReview Request: Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24537/diff/4-5/
Comment on attachment 8684340 [details] MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24539/diff/7-8/
Comment on attachment 8685450 [details] MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24787/diff/6-7/
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24789/diff/8-9/
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24863/diff/7-8/
Attachment #8685630 - Attachment description: MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r?billm → MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm
Comment on attachment 8686261 [details] MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24979/diff/5-6/
Attachment #8686261 - Attachment description: MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r?billm → MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm
Attachment #8686825 - Attachment description: MozReview Request: Bug 1171708 - Add tests for async window flushing. r?billm → MozReview Request: Bug 1171708 - Add tests for async window flushing. r=billm
Attachment #8686825 - Flags: review?(wmccloskey)
Comment on attachment 8686825 [details] MozReview Request: Bug 1171708 - Add tests for async window flushing. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25091/diff/3-4/
I'm afraid I had to update some more tests for window flushing. Here's the interdiff: https://reviewboard.mozilla.org/r/24787/diff/6-7/
Comment on attachment 8685450 [details] MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm Sorry to put this back in the queue, but I think I made enough substantial changes and additions to warrant it. :/ Interdiff: https://reviewboard.mozilla.org/r/24787/diff/6-7/
Attachment #8685450 - Flags: review+ → review?(wmccloskey)
Blocks: 1185674
Comment on attachment 8684340 [details] MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24539/diff/8-9/
Attachment #8684340 - Flags: review?(wmccloskey)
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24863/diff/8-9/
Comment on attachment 8686261 [details] MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24979/diff/6-7/
Comment on attachment 8686825 [details] MozReview Request: Bug 1171708 - Add tests for async window flushing. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25091/diff/4-5/
https://reviewboard.mozilla.org/r/24539/#review22319 > I don't understand why this is needed. It looks like removeClosedTabData is only called for tabs that were closed individually. I don't see how we could get a call to, e.g., undoClosedTab for a tab in a window that was closed. Primarily, it's because of the call to `removeClosedTabData` from `forgetClosedTab`, which is something that SessionStore exposes. If we're in the middle of a window flush, and something asks to forget a tab, this sidesteps us accidentally putting it back into the data object in `_closedWindows` once the flush completes. Or am I misunderstanding?
Comment on attachment 8684340 [details] MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24539/diff/8-9/
Attachment #8684340 - Flags: review?(wmccloskey)
Comment on attachment 8685450 [details] MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24787/diff/7-8/
Comment on attachment 8685451 [details] MozReview Request: Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24789/diff/9-10/
Comment on attachment 8685630 [details] MozReview Request: Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24863/diff/9-10/
Comment on attachment 8686261 [details] MozReview Request: Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24979/diff/7-8/
Comment on attachment 8686825 [details] MozReview Request: Bug 1171708 - Add tests for async window flushing. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25091/diff/5-6/
Hey billm - sorry for spamming this bug like crazy. I think the last set of patches are ready for review now.
Attachment #8684340 - Flags: review?(wmccloskey) → review+
Comment on attachment 8684340 [details] MozReview Request: Bug 1171708 - Have SessionStore asynchronous collect window information on close. r?billm https://reviewboard.mozilla.org/r/24539/#review22899 ::: browser/components/sessionstore/SessionStore.jsm:764 (Diff revision 9) > + Please remove the empty line. ::: browser/components/sessionstore/SessionStore.jsm:1222 (Diff revision 9) > - // update all window data for a last time > + let closedTabMap = new WeakMap(); Looks like we don't need this. ::: browser/components/sessionstore/SessionStore.jsm:1259 (Diff revision 9) > + // 2) Flush the window Need a period. ::: browser/components/sessionstore/SessionStore.jsm:1277 (Diff revision 9) > - let isLastWindow = > + // We can still access tabbrowser.browsers, thankfully Period. ::: browser/components/sessionstore/SessionStore.jsm:1292 (Diff revision 9) > + this.maybeSaveClosedWindow(winData); It seems like it's still possible to save the tab data here even if the tab was removed from \_closedWindowTabs before the flush. I guess you can fix that in a follow-up though.
Attachment #8686825 - Flags: review?(wmccloskey) → review+
Comment on attachment 8686825 [details] MozReview Request: Bug 1171708 - Add tests for async window flushing. r=billm https://reviewboard.mozilla.org/r/25091/#review22901 ::: browser/components/sessionstore/SessionStore.jsm:188 (Diff revision 6) > +/** Can you add some extra newlines around these? It feels a bit cramped.
Comment on attachment 8685450 [details] MozReview Request: Bug 1171708 - Fix SessionStore tests to account for async window flushing. r?billm https://reviewboard.mozilla.org/r/24787/#review22903 ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:349 (Diff revisions 6 - 7) > - resolve(); > + // Give the TabStateFlusher a chance to react to this final > + // update too before we resolve. > + TestUtils.executeSoon(resolve); So, uh, what's going on here? I thought promise resolution always returned to the event loop first anyway. ::: browser/components/sessionstore/SessionStore.jsm:375 (Diff revision 7) > + _windowFlushPromises: new Set(), Nobody seems to read from this thing. ::: browser/components/sessionstore/SessionStore.jsm:1297 (Diff revision 7) > + Extra newline here. ::: browser/components/sessionstore/test/browser_590563.js:20 (Diff revision 7) > - registerCleanupFunction(() => win.close()); > + registerCleanupFunction(function* () { > + yield BrowserTestUtils.closeWindow(win); > + }); You have a simpler way of doing this elsewhere (return the promise). ::: browser/components/sessionstore/test/browser_701377.js:35 (Diff revision 7) > - registerCleanupFunction(() => win.close()); > + registerCleanupFunction(function*() { > + yield BrowserTestUtils.closeWindow(win); > + }); Same here.
Attachment #8685450 - Flags: review?(wmccloskey) → review+
https://reviewboard.mozilla.org/r/24539/#review22899 > It seems like it's still possible to save the tab data here even if the tab was removed from \_closedWindowTabs before the flush. I guess you can fix that in a follow-up though. Done, filed bug 1225921.
https://reviewboard.mozilla.org/r/24787/#review22903 > So, uh, what's going on here? I thought promise resolution always returned to the event loop first anyway. I kinda threw everything into clearing a bunch of orange I was seeing in my try pushes. This is probably a leftover that isn't necessary, but I'll push to try first just to be sure. > Nobody seems to read from this thing. Yeah, whoops, leftovers. Thanks. > Extra newline here. I just removed the windowFlushPromises thing, which I was using to diagnose some failing tests.
https://reviewboard.mozilla.org/r/24787/#review22903 > I kinda threw everything into clearing a bunch of orange I was seeing in my try pushes. This is probably a leftover that isn't necessary, but I'll push to try first just to be sure. Try push failed with this removed, and passed with this put back. So yes, it is true that we go to the event loop with promise resolution, but both TabStateFlusher.flushWindow and this method are using that same infrastructure, and in the case of some tests, this message listener is being attached first, that means its getting serviced first, which means that it resolves first before the TabStateFlusher.flushWindow promise can resolve. I think that's what's happening, anyhow. This is why I'm deferring here. I'm going to put this back.
https://hg.mozilla.org/integration/fx-team/rev/82c9750d88e204516d04eaa91209e7bec691b57c Bug 1171708 - Add flushWindow method to TabStateFlusher. r=billm https://hg.mozilla.org/integration/fx-team/rev/451741a8b932024db0c5e54184667b6db970813a Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r=billm https://hg.mozilla.org/integration/fx-team/rev/4778fe17087aa893eef9084b1098bd882bb0c973 Bug 1171708 - Have SessionStore asynchronous collect window information on close. r=billm https://hg.mozilla.org/integration/fx-team/rev/32196b22b4544981f5658ce028e7566bff3fbca4 Bug 1171708 - Fix SessionStore tests to account for async window flushing. r=billm https://hg.mozilla.org/integration/fx-team/rev/c4b23403e71f42a7eee8ead3bcca414a9c1f1b49 Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r=billm https://hg.mozilla.org/integration/fx-team/rev/7031b88f02001c51b4f8f22396567813a3109548 Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm https://hg.mozilla.org/integration/fx-team/rev/9f55b32f4440f80b2a7cea14b35110cc522ef1fe Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm https://hg.mozilla.org/integration/fx-team/rev/a1ac4564029cec65bed930613865e0111adc7c7f Bug 1171708 - Add tests for async window flushing. r=billm
https://hg.mozilla.org/integration/fx-team/rev/c67eefd6cf4d5de1738d1c4aa66cd3436fe664b0 Bug 1171708 - Add flushWindow method to TabStateFlusher. r=billm https://hg.mozilla.org/integration/fx-team/rev/6098e098d9ebea5c54d97b71e575483d8e2c9f6f Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r=billm https://hg.mozilla.org/integration/fx-team/rev/020345f46fbdbeb9ce88bea645ce00b365ad903b Bug 1171708 - Have SessionStore asynchronous collect window information on close. r=billm https://hg.mozilla.org/integration/fx-team/rev/9a48c1d222485e6f5b42a9512a634f8c550188e6 Bug 1171708 - Fix SessionStore tests to account for async window flushing. r=billm https://hg.mozilla.org/integration/fx-team/rev/e1fcd236bdd367332fd0ef0ebb3815ea5a48cf37 Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r=billm https://hg.mozilla.org/integration/fx-team/rev/96830bc690b69dee05da219d99571e71d7fdcc1b Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm https://hg.mozilla.org/integration/fx-team/rev/c60c33f9b5732dfdc3e01fbec2905de31d441f7e Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm https://hg.mozilla.org/integration/fx-team/rev/cae56980cbec07da0f7c28be592bff3461086520 Bug 1171708 - Add tests for async window flushing. r=billm
Whoops - pretty careless of me - I forgot to put the TestUtils.executeSoon(resolve); bit in BrowserTestUtils.jsm in the "Fix SessionStore tests to account for async window flushing. r=billm", despite putting the comment about it back in. Sorry!
Flags: needinfo?(mconley)
https://hg.mozilla.org/integration/fx-team/rev/bc29e2e5fac327b36b25e8d38107e09adc0a91b9 Backed out 8 changesets (bug 1171708) for bustage in it's own tests ON A CLOSED TREE
Depends on: 1226333
Alright, I've split out the intermittently failing test into bug 1226333. Hopefully we can get it landed soon. In the meantime, going to try to land these again.
https://hg.mozilla.org/integration/fx-team/rev/1d66c53d96d8739c21e82876cbe6748b0e69464a Bug 1171708 - Add flushWindow method to TabStateFlusher. r=billm https://hg.mozilla.org/integration/fx-team/rev/7a6196e9c10c106f24433ff55b23ebb276b1da60 Bug 1171708 - Use localName when detecting <xul:browsers> for XULFrameLoaderCreated. r=billm https://hg.mozilla.org/integration/fx-team/rev/006d7ca07176c2c3569fceda8e4f48154de5130d Bug 1171708 - Have SessionStore asynchronous collect window information on close. r=billm https://hg.mozilla.org/integration/fx-team/rev/769ebcea6374fb7c7c0ff3a1e56475f8d5a5afa4 Bug 1171708 - Fix SessionStore tests to account for async window flushing. r=billm https://hg.mozilla.org/integration/fx-team/rev/89d4ae9701b0925a66619d5c68407dfb88d366cb Bug 1171708 - Add windowClosed and domWindowClosed helpers to BrowserTestUtils. r=billm https://hg.mozilla.org/integration/fx-team/rev/7fbd7f0ef3649de6ff4d2a4c280698065f2cf5e5 Bug 1171708 - Rewrite browser_354894_perwindowpb.js to be more Task-y. r=billm https://hg.mozilla.org/integration/fx-team/rev/8044df74f4203006f1619222a5f83c39806887f0 Bug 1171708 - Rewrite browser_490040.js to be more Task-y. r=billm
Depends on: 1227444
Blocks: 1238178
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: