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

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:leo+, firefox24 wontfix, firefox25 wontfix, firefox26 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

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.
Posted patch Tests, v1Splinter Review
This is the test from bug 901759, plus a check that the weak message listener is actually weak.
Attachment #787249 - Flags: review?
Posted patch Patch, v1Splinter Review
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.
Attachment #787250 - Flags: review?(bugs)
Attachment #787249 - Flags: review? → review+
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
Attachment #787250 - Flags: review?(bugs) → review+
> 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?
> 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.  :)
Flags: needinfo?(bugs)
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?
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 :)
Flags: needinfo?(bugs)
Blocks: 900221
Blocks a blocker.
blocking-b2g: --- → leo+
https://hg.mozilla.org/mozilla-central/rev/1ad239c019a2
https://hg.mozilla.org/mozilla-central/rev/6697d4740263
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Looks like it's going to stick this time.
Assignee: nobody → justin.lebar+bug
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...
Some testcase please, and file a bug. This one is closed.
You need to log in before you can comment on or make changes to this bug.