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

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
Created attachment 808456 [details] [diff] [review]
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.
(Assignee)

Comment 1

4 years ago
Created attachment 808457 [details]
Gaia test
(Assignee)

Updated

4 years ago
Attachment #808456 - Attachment description: weakreftest.patch → Gecko patch
(Assignee)

Updated

4 years ago
Assignee: nobody → ferjmoreno
Summary: [Message manager] Weak listener removed with the first received message → [Message manager] Objects adding weak listeners must implement Ci.nsIMessageListener
(Assignee)

Updated

4 years ago
Blocks: 915598
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.
(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.
(Assignee)

Updated

4 years ago
Summary: [Message manager] Objects adding weak listeners must implement Ci.nsIMessageListener → [Message manager] We must not force weak listeners to implement Ci.nsIMessageListener
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.
If strong listeners don't have to QI to nsIMessageListener, weak listeners shouldn't either.
(Assignee)

Comment 6

4 years ago
Created attachment 808530 [details] [diff] [review]
v1

Olli's argument sounds good enough to me
Attachment #808530 - Flags: review?(bugs)
This is blocking a potential koi+ issue. Please see bug 915598, comment #16. Nominating for koi+.
blocking-b2g: --- → koi?
https://hg.mozilla.org/mozilla-central/rev/75607a3b40e5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Updated

4 years ago
blocking-b2g: koi? → koi+
https://hg.mozilla.org/releases/mozilla-aurora/rev/772abe4f65ec
status-b2g-v1.2: --- → fixed
status-firefox25: --- → wontfix
status-firefox26: --- → fixed
status-firefox27: --- → fixed

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.