902714 - Add a simple implementation of nsIMessageManager::{Add,Remove}WeakMessageListener

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

In bug 901759, I implemented nsIMessageManager::{Add,Remove}WeakMessageListener using mozilla::WeakSet.

I like this a lot, but there are enough questions around mozilla::WeakSet and the surrounding code that I think it would be prudent to do something simpler first, and target that for b2g18.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Tests, v1

This is the test from bug 901759, plus a check that the weak message listener is actually weak.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

This is the same IDL changes from the earlier bug, plus the obvious changes to nsFrameMessageManager.cpp.

Sorry for giving you the other code earlier, smaug.  At the very least I should have done this in the reverse order.

Comment 3

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

Comment on attachment 787250 [details] [diff] [review]
Patch, v1


>+NS_IMETHODIMP
>+nsFrameMessageManager::AddWeakMessageListener(const nsAString& aMessage,
>+                                              nsIMessageListener* aListener)
>+{
>+  nsWeakPtr weak = do_GetWeakReference(aListener);
>+  NS_ENSURE_TRUE(weak, NS_ERROR_NO_INTERFACE);
>+
>+  nsCOMPtr<nsIAtom> message = do_GetAtom(aMessage);
>+  uint32_t len = mListeners.Length();
>+  for (uint32_t i = 0; i < len; ++i) {
>+    if (mListeners[i].mMessage == message &&
>+        mListeners[i].mWeakListener == weak) {
Hmm, don't you want to compare the non-weak pointers.
Weak reference can be implemented as a tearoff.


>+nsFrameMessageManager::RemoveWeakMessageListener(const nsAString& aMessage,
>+                                                 nsIMessageListener* aListener)
>+{
>+  nsWeakPtr weak = do_GetWeakReference(aListener);
>+  NS_ENSURE_TRUE(weak, NS_OK);
>+
>+  nsCOMPtr<nsIAtom> message = do_GetAtom(aMessage);
>+  uint32_t len = mListeners.Length();
>+  for (uint32_t i = 0; i < len; ++i) {
>+    if (mListeners[i].mMessage == message &&
>+        mListeners[i].mWeakListener == weak) {
ditto

Comment 4

[Justin Lebar (not reading bugmail)]

> Hmm, don't you want to compare the non-weak pointers.

Do you mean, I don't want to compare the weak pointers, I need to do_QueryReferent first and compare non-weak pointers?

The issue with tearoffs is that one object can give me two different weakref tearoffs?

Comment 5

[Justin Lebar (not reading bugmail)]

> The issue with tearoffs is that one object can give me two different weakref tearoffs?

If this is the case, my WeakSet implementation is totally wrong.  :)

Comment 6

[Justin Lebar (not reading bugmail)]

I think someone who implemented nsISupportsWeakReference as a tearoff would need to implement nsIWeakReference themselves, since the nsIWeakReference implementation used by nsSupportsWeakReference is hidden in a cpp file.

If that's the case, then I can vouch from my previous patches that the only other nsIWeakReference implementation is the one used by the FragmentOrElement tearoff.  Looking at the implementation of nsNodeSupportsWeakRefTearoff, even if FragmentOrElement gives us two different tearoffs when we QI to nsISupportsWeakReference, the tearoffs will all give us the same nsIWeakReference object when we call GetWeakReference.

Is your worry about binary extensions, conformance to XPCOM rules, or something else?

Comment 7

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

The worry is about XPCOM rules yes.

Could you perhaps add a debug only check/assert that we don't end up keeping two WeakRefs to the same
object in the list. Would be just a tiny loop.
Then I could live with this :)

Comment 8

[Justin Lebar (not reading bugmail)]

Thanks, Olli.

https://hg.mozilla.org/integration/b2g-inbound/rev/1ad239c019a2
https://hg.mozilla.org/integration/b2g-inbound/rev/6697d4740263

Comment 9

[Justin Lebar (not reading bugmail)]

Blocks a blocker.

Comment 10

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/16ca97f8e128
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6279a8dc7c96

Comment 11

[Carsten Book [:Tomcat]]

Hi Justin, had to back this changes out in https://tbpl.mozilla.org/?tree=Mozilla-B2g18&rev=218640747bcd since this checkin caused failed mochitests like 
https://tbpl.mozilla.org/php/getParsedLog.php?id=26327224&tree=Mozilla-B2g18

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ad239c019a2
https://hg.mozilla.org/mozilla-central/rev/6697d4740263

Comment 13

[Justin Lebar (not reading bugmail)]

Re-landed on b2g18.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4c3f59e9c7a0
https://hg.mozilla.org/releases/mozilla-b2g18/rev/494d88a27129

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest timeouts.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/4e35bfc690f4

Comment 15

[Justin Lebar (not reading bugmail)]

Relanded again

https://hg.mozilla.org/releases/mozilla-b2g18/rev/bcd70a871ade
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e85db8ff0a7c

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Looks like it's going to stick this time.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/367cb34c8518
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/e51b8e012b43

Comment 18

[Gene Lian (I already quit Mozilla)]

I heard someone saying the cpmm/ppmm (using DOMRequestIpcHelper) doesn't work *randomly*. Sometimes, the receiveMessage() in child doesn't run when the parent calls target.sendAsyncMessage(...). The symptom looks like the mochitest timeout causing the previous back-out. I'm wondering it's due to these patches regarding the weak reference things (I can be wrong). Sadly, I'm not an expert on all of these changes...

Comment 19

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

Some testcase please, and file a bug. This one is closed.