901759 - Add nsIMessageManager::{Add,Remove}WeakMessageListener based on mozilla:MaybeWeakSet (bug 901746)

Status: NEW

(Core :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

I have patches which allow nsIMessageManager to add weak message listeners.

These patches also convert nsIMessageManager's strong message listeners to use a hashtable of lists, instead of a plain list.  This makes the strong message listeners perform more similarly to weak message listeners (which are implemented as a hashtable of hashsets).  If you like, I'd be happy to make regular message listeners be a hashtable of hashsets; I didn't do this because I was a bit worried about memory waste when we have many hashtables with just one or two elements.

smaug, I know you didn't really like this the last time we talked about it.  We /can/ do without this patch; I'm adding finalizers to weakrefs, so an object could implement a weak message listener itself.  But this is /much/ simpler for users of the API.

We've seen a lot of leaks stemming from message listeners, and based on the experience of finding and fixing those leaks, I think this is a good idea.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 2

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

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

Please document that weak listeners are called in random order!

>+  nsTArray<nsCOMPtr<nsIMessageListener> >* listeners = nullptr;
>+  if (!mStrongListeners.Get(aMessage, &listeners)) {
>+    mStrongListeners.Put(aMessage, new nsTArray<nsCOMPtr<nsIMessageListener> >());
>+    mStrongListeners.Get(aMessage, &listeners);
>+  }
Could you perhaps assign to listeners before Put, and put that value?
Then you wouldn't need to use Get


>+nsFrameMessageManager::AddWeakMessageListener(const nsAString& aMessage,
>+                                              nsIMessageListener* aListener)
>+{
>+  WeakSet<nsIMessageListener>* listeners = nullptr;
>+  mWeakListeners.Get(aMessage, &listeners);
>+  if (!listeners) {
>+    listeners = new WeakSet<nsIMessageListener>();
>+    mWeakListeners.Put(aMessage, listeners);
>+  }
>+
>+  // WeakSet::Add() returns false iff its arg can't be QI'ed to
>+  // nsISupportsWeakReference.
>+  if (!listeners->Add(aListener)) {
>+    return NS_ERROR_NO_INTERFACE;
>+  }
Could you even add a warning here, so that we see in the terminal that the call failed.


>+NS_IMETHODIMP
>+nsFrameMessageManager::RemoveWeakMessageListener(const nsAString& aMessage,
>+                                                 nsIMessageListener* aListener)
>+{
>+  WeakSet<nsIMessageListener>* listeners = nullptr;
>+  mWeakListeners.Get(aMessage, &listeners);
>+  if (!listeners) {
>+    return NS_OK;
>+  }
>+
>+  // WeakSet::Remove() returns false iff its arg can't be QI'ed to
>+  // nsISupportsWeakReference.
>+  if (!listeners->Remove(aListener)) {
>+    return NS_ERROR_NO_INTERFACE;
>+  }
And here too
>+  if (strongListeners) {
>+    for (uint32_t i = 0; i < strongListeners->Length(); i++) {
++i, please

>+      nsresult rv = DispatchMessage((*strongListeners)[i], aTarget, aMessage,
>+                                    aSync, aCloneData, aCpows, aJSONRetVal);
>+      NS_ENSURE_SUCCESS(rv, rv);
Hmm, should we actually just continue here. I think so.

>+    for (uint32_t i = 0; i < weakListeners.Length(); i++) {
>+      nsresult rv = DispatchMessage(weakListeners[i], aTarget, aMessage, aSync,
>+                                    aCloneData, aCpows, aJSONRetVal);
>+      NS_ENSURE_SUCCESS(rv, rv);
ditto


This should have at least some basic tests.
With that, r=me

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Tests, v1

I tried writing a new test file and had a lot of difficulty getting it to work.  I hope you don't mind that I added it here; this looks like it's the test for the mm interface, anyway.

Comment 4

[Justin Lebar (not reading bugmail)]

> Hmm, should we actually just continue here. I think so.

Do you want me to make DispatchMessage return void, or should it continue to return nsresult?  If it continues to return nsresult, do you want to print a warning if the result indicates failure?

Comment 5

[Justin Lebar (not reading bugmail)]

I think the right thing to do may be to have DispatchMessage return void but report its early returns with NS_ENSURE_TRUE_VOID() and so on.  This will let us see exactly which early return we took.

Comment 6

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

NS_ENSURE_TRUE_VOID() sounds good

Comment 7

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

Comment on attachment 786568 [details] [diff] [review]
Tests, v1

This is ok, but it would be nice to have a test which checks that the
weak listener, not owned by anyone, actually disappears at some point
(after gc+cc+gc?).
But if you add some similar kind of tests for the WeakSet, or WeakSet+ObserverService, this is enough.


(I want tests mainly to prevent regressions in the future.)

Comment 8

[Justin Lebar (not reading bugmail)]

> But if you add some similar kind of tests for the WeakSet, or WeakSet+ObserverService, this is 
> enough.

This might be easier; I'll see.

Comment 9

[Justin Lebar (not reading bugmail)]

Talking to bsmedberg (and hearing his issues with the patches in bug 901746) convinced me that we should try a simpler approach for b2g18.

I don't like this, because it's asymptotically extremely inefficient and doesn't prevent us from building a big array of nsWeakPtr's under some circumstances, so I really want to do what we have here.  But baby steps.

Comment 10

[Justin Lebar (not reading bugmail)]

I'll file a new bug for the simpler, slower thing.

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v2

This is ready for review, but I want to get at least preliminary f+ on the patches this depends on before asking someone to look at it.

Tests have already landed.

Comment 12

[Justin Lebar (not reading bugmail)]

This patch is nice because it's simpler than the old patch, and it also allows us to interleave strong and weak observers in our dispatch list.