Bug 1171708 - [e10s] Stop using CPOWs for window closing

TabState.flushWindow is sending down a sync CPOW message.

Collect parent process info right away when window is closing (collect window data - TabState.collect)

billm added code that makes is to that the frameloaders are kept alive until the last message gets sent up

Be careful not to remove the message listeners until the last messages are sent up.

Ohhhhhh man - it’s possible to pass a third argument to addMessageListener on a frameloader that makes it so that we can hear about messages from a frameloader that has GONE AWAY.

Very interesting.

So here’s the challenge:

onClose collects a bunch of things from the window that is closing. Some of that stuff also involves collecting the individual tab states for each browser tab. Since that’s now asynchronous, we have to return to the event loop and the frameloaders are kept alive until the last tab state update comes up and the TabStateFlusher promise gets resolved.

Here’s the kicker though - by the time that occurs, the DOM for the window has disappeared. So any information gathering that relies on the DOM from the window has to occur _before_ the async stuff gets kicked off, and then sewn together.

Things that need to be collected or run before DOM goes away:

_collectWindowData

The winData.title

Things that need to be collected from content asynchronously:

The state for each tab in the window

So here’s the problem - _collectWindowData relies on DOM stuff, and needs to be run before the DOM for the window goes away. One of the things it does, however, is to update the “winData” stashed in this._windows with tabData… but that tabData is likely out of date at that point.

Ideas:

Collect tabData twice

Collect the window state without flushing, with the understanding that the tabData might be out of date. Then, flush the tabs asynchronously. When the promise resolves, update the tabData for each window. Finally then detach the message listeners and add the window to the DyingWindowCache.

Pros: Straight-forward solution that doesn’t look like it'll rock the boat too much with what SessionStore does in other code-paths.

Cons: Some redundancy - I end up collecting the tabData once, and then again once the flush is complete.

Change _collectWindowData

Collect the sync window data only, and perhaps have some kind of Promise-y

This seems risky. I’d have to audit all of the consumers of this function to make sure they’re still going to behave correctly after this change. And then the consumers of those functions. It sounds like a fragile strategy.

It also doesn’t look like any effort has gone into making sure that _collectWindowData returns the most up-to-date information, so making this Promise-y and async doesn’t sound like the right choice.

Write _collectWindowDataAsync

Write an alternative _collectWindowData function that will do all of the DOM related collection first, and then do the flush before getting the tabData. This gets rid of the redundancy if we collect the tabData twice, but then we have the redundancy of having two _collectWindowData methods.

What is the DyingWindowCache?

"// A map storing a closed window's state data until it goes aways (is GC'ed).

// This ensures that API clients can still read (but not write) states of

// windows they still hold a reference to but we don’t."

Translation : a WeakMap for consumers of this window data to make sure they can still read window data while they still hold a reference.

Problem: The update message after the flush seems to be pretty unreliable. It doesn’t look as if we’re holding the frameloaders alive until those messages come up.

GUHHH.. A new problem.

It looks like, at least for groupMessageManagers… we’re not dispatching the SessionStore:update to the listener, because… because it looks like the listener is gone? It’s kinda hard to tell, since there are several message managers and they all look the same. :/

Plan: Add an optional “name” for group message managers that I can see while debugging - and that way, in the handlers for SessionStore:update, I can see whether or not the message managers that deal with it have that name. If so, then I need to somehow figure out when that message manager gets its listeners cleared!

Let’s see if I can prove this with a regression test.

The code to keep sending messages after the frameloader has been removed from the DOM is here: https://bugzilla.mozilla.org/show_bug.cgi?id=1126089

dom/base/test/browser_messagemanager_unload.js is where the original testing for that stuff went.

Fuuuuuuuuu - … if I modify that test to use the group message manager, things be busted. I think I need a better understanding the group message manager.

From https://dxr.mozilla.org/mozilla-central/rev/6077f51254c69a1e14e1b61acba4af451bf1783e/dom/base/nsIMessageManager.idl#50


  *  Parent side            
             Child side


  * -------------                       ------------


  *  global MMg


  *   |


  *   +-->window MMw1


  *   |    |


  *   |    +-->frame MMp1_1<------------>frame MMc1_1


  *   |    |


  *   |    +-->frame MMp1_2<------------>frame MMc1_2


  *   |    |


  *   |    +-->group MMgr1


  *   |    |    |


  *   |    |    +-->frame MMp2_1<------->frame MMc2_1


  *   |    |    |


  *   |    |    +-->frame MMp2_2<------->frame MMc2_2


  *   |    |


  *   |    +-->group MMgr2


  *   |    |    ...


  *   |    |


  *   |    ...


  *   |

Right, so that matches my mental model...

AH wait. I was wrong! The parent is receiving the message! The problem is that we’re bailing out at this chunk of code:

case "SessionStore:update" :
// |browser.frameLoader| might be empty if the browser was already
// destroyed and its tab removed. In that case we still have the last
// frameLoader we know about to compare.
let frameLoader = browser.frameLoader ||
this ._lastKnownFrameLoader.get(browser.permanentKey);

// If the message isn't targeting the latest frameLoader discard it.
if (frameLoader != aMessage.targetFrameLoader) {

return ;

}

So… so why is that the case?

Ah cripes

It looks like XULFrameLoaderCreated, when it’s fired, compares target.tagName with “browser”, which is fine with dynamically created browsers - but for the initial browser that’s defined in the markup, this will fail to match, since the tagname will be “xul:browser”. :(( We should use localname instead.

Ok, so that works.

Now I’m back where I wanted to be - I’ve got these flushings occurring… and I want to save the final message from the child. How will I do it?

For closed tabs, there’s this _closedTabs WeakMap on SessionStore that maps permanentKey’s to an Object containing tab information that can then be saved.

I should probably do something similar.

Gather the window information, and stash it in a _closedWindows WeakSet, keyed on windows (?).

That should map to an object with two things: window information we were able to gather synchronously, and a WeakSet of browsers, along with a count of how many :update’s we’re expecting?

As the :update’s come in, decrement the ...

OH WAIT, let me get this down before I call it a day.

We already collect this winData thing, and shove it in _closedWindows. Let’s map browser.permanentKey’s to each tab’s entry in the tabs part of that _closedWindows thing.

As the messages from each tab come in, update those entries, and queue up a write. I guess.

Okay! I think this kinda works! Maybe!

But now I’ve get test failures (non-e10s). Let’s take a look:

31 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_broadcast.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_broadcast.js:79 - TypeError: can't convert undefined to object

BLARGH. So I guess I do need to wait until the final messages come up. Shit.

So, here’s what I want.

I think I want something like TabStateFlusher.waitForFinalFlushes(aWindow)...

WAIT. No, I was understanding this wrong. I was sending the flush request down to the child, but before it could react to it, it sent the final update message up. I need to make my _closedWindowTabs thing handle that case.

Aaaaaand we’re passing!

363 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_cleaner.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_cleaner.js:80 - TypeError: state._closedWindows[0] is undefined

Stack trace:

test_open_and_close@chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_cleaner.js:80:3

Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23

this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7

Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11

this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7

Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:451:5

Tester_execTest@chrome://mochikit/content/browser-test.js:757:9

Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:677:7

SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:735:59

364 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_cleaner.js | Uncaught exception - [Exception... "Invalid index: not in the closed windows" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_undoCloseWindow :: line 2026" data: no]

365 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_cleaner.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_cleaner.js:116 - TypeError: state._closedWindows[0] is undefined

Stack trace:

test_old_data@chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_cleaner.js:116:3

Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:21

this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7

Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11

this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7

this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7

testScope/test_executeSoon/<.run@chrome://mochikit/content/browser-test.js:969:9

Tester_execTest@chrome://mochikit/content/browser-test.js:757:9

Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:677:7

SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:735:59

366 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_cleaner.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_cleaner.js:140 - TypeError: state._closedWindows[0] is undefined

Stack trace:

test_cleanup@chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_cleaner.js:140:3