In order to let tests finish we need to clear out any dead browsers from the set of pending browsers in windowClosed as we will not be receiving messages from them.

Gijs feel free to redirect.

Comment on attachment 8786896 [details] [diff] [review] Don't wait for dead browsers in BrowserTestUtils.windowClosed Review of attachment 8786896 [details] [diff] [review]: ----------------------------------------------------------------- This assumes that: - at least one of the browsers in the window will be non-dead when windowClosed is called and get a sessionstore:update message - by the time the last of such sessionstore:update messages arrives, all the (other) browsers will already be dead. - the compartment dying will never result in the sessionstore:update message not being sent at all. Can you clarify if/why these things are true? Assuming all these things are true, I think the patch is OK, but I don't have confidence that all of them are true, so I'm delegating to mconley, and hopefully I'll learn from his feedback. :-)

Comment on attachment 8786896 [details] [diff] [review] Don't wait for dead browsers in BrowserTestUtils.windowClosed (In reply to :Gijs Kruitbosch from comment #3) > Comment on attachment 8786896 [details] [diff] [review] > Don't wait for dead browsers in BrowserTestUtils.windowClosed > > Review of attachment 8786896 [details] [diff] [review]: > ----------------------------------------------------------------- > > This assumes that: > - at least one of the browsers in the window will be non-dead when > windowClosed is called and get a sessionstore:update message > - by the time the last of such sessionstore:update messages arrives, all the > (other) browsers will already be dead. > - the compartment dying will never result in the sessionstore:update message > not being sent at all. > > Can you clarify if/why these things are true? > > Assuming all these things are true, I think the patch is OK, but I don't > have confidence that all of them are true, so I'm delegating to mconley, and > hopefully I'll learn from his feedback. :-) per IRC, s/mconley/billm/ . :-)

Comment on attachment 8786896 [details] [diff] [review] Don't wait for dead browsers in BrowserTestUtils.windowClosed Review of attachment 8786896 [details] [diff] [review]: ----------------------------------------------------------------- on restore messages within a second or two.Hi Eric, I don't think this code is correct. The way session restore works is that it waits until it has received an update message from every window, which might be long after the window is gone if the content process is running slowly. So even if the <browser> wrapper gets nuked by your nuking patch, we might still need to wait for a response from the content process. Here's a possible workaround: Besides the message.target property (which points to the <browser> element), we also have a message.targetFrameLoader property that points to the nsIFrameLoader that the message came from. Interestingly, while <browser> elements live in the compartment of the chrome window that owns them, an nsIFrameLoader is kind of stateless. That is, each compartment gets its own reflector of the C++ object. So while nuking will kill off the reference from BrowserTestUtils.jsm to the <browser> element of the closed window, it will leave the frameloader untouched. So one solution here is to save the frameloaders we're waiting on rather than the <browsers>. Then you can use the targetFrameLoader property instead of the target property. It's possible that there's more session restore code that would need such changes. Another option is to delay nuking until session restore is finished with the window. That's less likely to break things. And typically we'll get back our sessi

There's pending feedback on the parent bug about whether there's a better way to nuke ccw's in a session restore friendly way. In the meantime I can test Bill's suggestion to at least see if that works. ni? myself to get back to this.

In order to let tests finish we need to wait for frameLoaders in windowClosed rather than browsers in case the browser instance is destroyed.

Turned out to be easier than expected (I think), patch is up.

Comment on attachment 8791829 [details] [diff] [review] Wait for frameLoaders instead of browsers in BrowserTestUtils.windowClosed Review of attachment 8791829 [details] [diff] [review]: ----------------------------------------------------------------- I don't think this is correct. The patch in bug 1276366 should fix this in a more general way.

(In reply to Bill McCloskey (:billm) from comment #9) > Comment on attachment 8791829 [details] [diff] [review] > Wait for frameLoaders instead of browsers in BrowserTestUtils.windowClosed > > Review of attachment 8791829 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't think this is correct. The patch in bug 1276366 should fix this in a > more general way. I think unfortunately that patch doesn't fix this issue. I'll see if I can repro locally.

Eric, it seems like this might be helpful in getting traction on bug 1276366 again, which seems like a good thing to fix for the whole quantum / faster+leaner thing. Is there something I can do to help / what's the current status of this work?

(In reply to :Gijs from comment #12) > Eric, it seems like this might be helpful in getting traction on bug 1276366 > again, which seems like a good thing to fix for the whole quantum / > faster+leaner thing. Is there something I can do to help / what's the > current status of this work? It's on hiatus, I agree it's useful work. Basically comment 10 is where we're at. Bill thought our patch adding a "nuke blocker" for session restore should make this unnecessary. I found that was not the case. To start work again we'd need to rebase the patches from the parent bug and then see if things work without this patch. They probably don't so apply this patch and see if that fixes things (I don't think it did completely). The patch series from https://treeherder.mozilla.org/#/jobs?repo=try&revision=9039fee0e0dd is a starting point.