927363 - Add/Remove SystemMessageManager:GetPendingMessages:Return on demand

Status: RESOLVED WONTFIX

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

Description

[Fernando Jiménez Moreno [:ferjm]]

We don't need to add the SystemMessageManager:GetPendingMessages:Return listener during the initialization of the SystemMessageManager. We can simply add it on demand when sending the SystemMessageManager:GetPendingMessages request.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 2

[Gene Lian (I already quit Mozilla)]

Comment on attachment 817786 [details] [diff] [review]
v1

Review of attachment 817786 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/messages/SystemMessageManager.js
@@ +250,5 @@
>  
>    // nsIDOMGlobalPropertyInitializer implementation.
>    init: function sysMessMgr_init(aWindow) {
>      debug("init");
> +    this.initDOMRequestHelper(aWindow, ["SystemMessageManager:Message"]);

Looks good to me but have one question not related to this bug. r=gene

In bug 915598, the default initDOMRequestHelper goes in a way of adding weak reference, which means the "SystemMessageManager:Message" listener would be removed at any points even if the window is still alive. Right? How can this happen? Does it mean the child window would probably miss some system messages sent from the parent?

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Thanks Gene!

https://hg.mozilla.org/integration/b2g-inbound/rev/3c0d710561bf

Regarding your question, it's actually possible that the listener is removed if the object is GC'd, I'm creating a bug for this. Now that we fixed bug 918479, I believe we are safe adding strong listeners in these specific cases. We still need to review all APIs using DOMRequestHelper.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c0d710561bf

Comment 5

[Gene Lian (I already quit Mozilla)]

Back this out due causing regression at bug 930296.

https://hg.mozilla.org/mozilla-central/rev/770de5942471
https://hg.mozilla.org/integration/mozilla-inbound/rev/13469e0250a3
https://hg.mozilla.org/integration/b2g-inbound/rev/19315df9110d

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Please reopen bugs if you back out patches.

Comment 7

[Gene Lian (I already quit Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> Please reopen bugs if you back out patches.

Yes, I know. :) We're leaving comments at the same time.

Let's keep the original codes as they are since it hasn't fixed fix any bugs. Instead, just some enhancement (cause regression though...). Resolved this one as WONTFIX.

Fernanodo, please feel free to reopen it if you still think we need to fix that. Otherwise, I'd prefer keeping that.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Also, you don't need to backout from both b2g-inbound and m-c. They merge to each other. One branch is fine and reduces the odds of hitting merge conflicts later.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #8)
> Also, you don't need to backout from both b2g-inbound and m-c. They merge to
> each other. One branch is fine and reduces the odds of hitting merge
> conflicts later.

*and inbound. That was 3x more work than you needed to do.