This is useful when we receive a message while a tab is being torn down, as in bug 1126089. Since the <browser> element can be re-used, we need a way to know whether the message is coming from the <browser>'s new or old frameloader. This patch adds msg.targetFrameLoader so we can check. See bug 1109875 comment 18 and 19 for more motivation. I had to add a CacheFrameLoader method to nsInProcessTabChildGlobal. It's basically the same as the CacheFrameLoader method on TabParent.

Comment on attachment 8593405 [details] [diff] [review] patch Review of attachment 8593405 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/test/browser_messagemanager_unload.js @@ +82,5 @@ > let index = 0; > browser.messageManager.addMessageListener("Test:Event", (msg) => { > ok(msg.target === browser, "<browser> is correct"); > + ok(msg.targetFrameLoader === frameLoader, "frameLoader is correct"); > + ok(browser.frameLoader === null, "browser frameloader null during teardown"); Wait, if browser.frameLoader=null after calling gBrowser.removeTab(), how can SessionStore.receiveMessage() check that those aren't messages from a previous frameLoader? receiveMessage() would use |browser.permanentKey| to update the right cache bucket, but would only do that if |msg.targetFrameLoader == msg.target.frameLoader|, the last check wouldn't be possible if I understand this correctly?

Well, the check in receiveMessage would look like this: if (msg.target.frameLoader && msg.target.frameLoader != msg.targetFrameLoader) { return; } There are three cases: 1. The message is from the <browser>'s current frameloader. 2. The <browser>'s frameloader is null. 3. The <browser> has a new frameloader because we switched remoteness, and the message is from the old frameloader. In cases 1 and 2, we don't want to ignore the message, and the code above doesn't. In case 3, we do want to ignore the message. In that case, browser.frameLoader is non-null (since we now have a new frameloader) and it will be different than msg.targetFrameLoader, so we will ignore the message.

Looks like I missed Tim on IRC. Does comment 2 sound okay?

Comment on attachment 8593405 [details] [diff] [review] patch > [uuid(a9e07e89-7125-48e3-bf73-2cbae7fc5b1c)] > interface nsIInProcessContentFrameMessageManager : nsIContentFrameMessageManager > { > [notxpcom] nsIContent getOwnerContent(); >+ [notxpcom] void CacheFrameLoader(in nsIFrameLoader aFrameLoader); update uuid and cacheFrameLoader (lowercase C) >+ // We keep a strong reference to the frameloader after we've started >+ // teardown. This allows us to dispatch message manager messages during this >+ // time. >+ nsCOMPtr<nsIFrameLoader> mFrameLoader; This could use some other name since it is valid only during shutdown, but since TabParent has mFrameLoader too... fine. I don't understand why nsInProcessTabChildGlobal::DoSendBlockingMessage would work. mFrameLoader is null usually there, right? Because of that, r-

(In reply to Bill McCloskey (:billm) from comment #2) > There are three cases: > 1. The message is from the <browser>'s current frameloader. > 2. The <browser>'s frameloader is null. > 3. The <browser> has a new frameloader because we switched remoteness, and > the message is from the old frameloader. So in case (2), wouldn't we at this point also accept messages from former frameLoaders? If browser.frameLoader=null as soon as we call gBrowser.removeTab() then there's definitely the possibility of receiving the wrong messages if we switched a browser's remoteness shortly before? Wouldn't it be easier if browser.frameLoader was always pointing to the current/latest frameLoader as long as there are pending messages? That way code could simply check whether msg.target.frameLoader == msg.targetFrameLoader to ignore old messages.

OK, I see what you mean. Keeping the <browser>'s frameloader property valid after it's taken out of the document is kind of hard. I talked to Olli and he suggested it would be easier to do it in the frontend. When we add a <browser> to the document (in addTab I guess) we could add an expando that tracks the browser's most recent frameloader (or maybe use a weakmap). When we do a docshell swap or a remoteness change, we'd have to update it. Kind of a pain, but doing it at the platform level is even harder.

Comment on attachment 8594052 [details] [diff] [review] patch v2 >+already_AddRefed<nsIFrameLoader> >+nsInProcessTabChildGlobal::GetFrameLoader() >+{ >+ nsCOMPtr<nsIFrameLoaderOwner> owner = do_QueryInterface(mOwner); >+ nsCOMPtr<nsIFrameLoader> fl = owner->GetFrameLoader(); You need to null check owner. So perhaps nsCOMPtr<nsIFrameLoader> fl = owner ? owner->GetFrameLoader() : nullptr;