919429 - [Message manager] We must not force weak listeners to implement Ci.nsIMessageListener

Status: RESOLVED FIXED

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

Description

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko patch

I've being doing some tests with |addWeakMessageManager| and I find myself in a situation where a weak listener is being removed as soon as the first message is received.

The attached patches are:

1- Gecko mod: a few debug statements (B2G only) and a quick hack on the Webapps API which adds a weak listener for the "Webapps:GetSelf:Return:OK" message with every |.getSelf| call and removes this listener when the message is received in the child side.

2- Gaia test: a mozApps.getSelf test call.

The logs that I get with these patches applied are the following:

I/Gecko   ( 1670): === getSelf
I/Gecko   ( 1670): AddWeakMessageListener Webapps:GetSelf:Return:OK
I/Gecko   ( 1534): ReceiveMessage Webapps:GetSelf
I/Gecko   ( 1534): Got Webapps:GetSelf
I/Gecko   ( 1534): sendAsyncMessage Webapps:GetSelf:Return:OK
I/Gecko   ( 1670): ReceiveMessage Webapps:GetSelf:Return:OK
I/Gecko   ( 1670): ===== Removing expired weak listener Webapps:GetSelf:Return:OK ======!
E/GeckoConsole( 1670): Content JS LOG at app://getselftest.gaiamobile.org/test.js:4 in test: Calling getSelf

As you can see, the listener is being removed with the first message received, even if this message is exactly the one being listener, and the "Webapps:GetSelf:Return:OK" message is never received in the child (1670).

Adding a non-weak listener gives the following output, where the listener is not being removed and the messages are received in the child side as expected:

I/Gecko   ( 1845): === getSelf
I/Gecko   ( 1845): AddMessageListener Webapps:GetSelf:Return:OK
I/Gecko   ( 1719): ReceiveMessage Webapps:GetSelf
I/Gecko   ( 1719): Got Webapps:GetSelf
I/Gecko   ( 1719): sendAsyncMessage Webapps:GetSelf:Return:OK
I/Gecko   ( 1845): ReceiveMessage Webapps:GetSelf:Return:OK
I/Gecko   ( 1845): Got Webapps:GetSelf:Return:OK
I/Gecko   ( 1719): ReceiveMessage Webapps:RegisterForMessages
E/GeckoConsole( 1845): Content JS LOG at app://getselftest.gaiamobile.org/test.js:4 in test: Calling getSelf
E/GeckoConsole( 1845): Content JS LOG at app://getselftest.gaiamobile.org/test.js:6 in anonymous: We never get here :(

I wonder if this is actually the expected behavior or if the message manager is being too aggressive removing the weak listeners. Unfortunately, I couldn't find out yet if the listener is being GC'ed.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gaia test

Comment 2

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

Just for posterity here, the issue is that if you pass a JS object to a method that takes an nsIMessageListener, XPConnect will do the implicit conversion for you.  But if you pass it to a method that takes an nsISupports and explicitly call QI on it, that will fail.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo, please) from comment #0)
> I/Gecko   ( 1670): === getSelf
> I/Gecko   ( 1670): AddWeakMessageListener Webapps:GetSelf:Return:OK
> I/Gecko   ( 1534): ReceiveMessage Webapps:GetSelf
> I/Gecko   ( 1534): Got Webapps:GetSelf
> I/Gecko   ( 1534): sendAsyncMessage Webapps:GetSelf:Return:OK
> I/Gecko   ( 1670): ReceiveMessage Webapps:GetSelf:Return:OK
> I/Gecko   ( 1670): ===== Removing expired weak listener
> Webapps:GetSelf:Return:OK ======!
> E/GeckoConsole( 1670): Content JS LOG at
> app://getselftest.gaiamobile.org/test.js:4 in test: Calling getSelf
> 
> As you can see, the listener is being removed with the first message
> received, even if this message is exactly the one being listener, and the
> "Webapps:GetSelf:Return:OK" message is never received in the child (1670).

This looks ok to me. When the weak listener is "removed", it means the object it points to has already gone away.


> I wonder if this is actually the expected behavior or if the message manager
> is being too aggressive removing the weak listeners. Unfortunately, I
> couldn't find out yet if the listener is being GC'ed.
Well, if weakListener = do_QueryReferent(mListeners[i].mWeakListener); causes weakListener to be null, 
the listener is either dead or the listener doesn't have the right QI... ahaa, that is why the summary was changed.

Comment 4

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

FWIW I disagree with the latest title of this bug.  Forcing them to implement it explicitly is fine as long as the failure behavior is easy to understand.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

If strong listeners don't have to QI to nsIMessageListener, weak listeners shouldn't either.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Olli's argument sounds good enough to me

Comment 7

[Gene Lian (I already quit Mozilla)]

This is blocking a potential koi+ issue. Please see bug 915598, comment #16. Nominating for koi+.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75607a3b40e5

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/75607a3b40e5

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/772abe4f65ec