Closed Bug 1155224 Opened 5 years ago Closed 5 years ago

Add a targetFrameLoader property to message manager messages

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8593405 - Flags: review?(bugs)
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?
Flags: needinfo?(ttaubert)
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-
Attachment #8593405 - Flags: review?(bugs) → review-
(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.
Flags: needinfo?(ttaubert) → needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
Attached patch patch v2Splinter Review
Attachment #8593405 - Attachment is obsolete: true
Attachment #8594052 - Flags: review?(bugs)
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;
Attachment #8594052 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/b76c5d271fea
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.