1354599 - Implement DOMEventTargetHelper::KeepAliveIfHasListenersFor(nsIAtom*||const nsAString&)

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Currently we have custom implementations for keeping a DOMEventTargetHelper alive if there is at least 1 listener. This happens in BroadcastChannel, WebSocket and MediaQueryList.

I would like to move such logic in DOMEventTargetHelper.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - KeepAliveIfHasListenerFor()

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - BroadcastChannel

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - MediaQueryList

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 1 - KeepAliveIfHasListenerFor()

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 2 - BroadcastChannel

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 3 - MediaQueryList

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 3 - MediaQueryList

Comment 8

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

Comment on attachment 8858280 [details] [diff] [review]
part 1 - KeepAliveIfHasListenerFor()

This looks a bit complicated approach.
Why not just reuse
virtual void EventListenerAdded(nsIAtom* aType) {}
virtual void EventListenerRemoved(nsIAtom* aType) {} ?
Possibly add similar methods for string case.

+  struct {
+    nsTArray<nsString> mStrings;
+    nsTArray<nsIAtom*> mAtoms;
+  } mKeepingAliveTypes;
Looks scary to have raw pointer. I could easily see some new API adding usage for this stuff for some esoteric event type, which might not use
static atoms, so we'd just have security critical bug with atoms use.
nsCOMPtr please.

Comment 9

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

Comment on attachment 8858296 [details] [diff] [review]
part 3 - MediaQueryList

I wonder why MediaQueryList has all sorts of NS_IsMainThread() checks when it is main thread only object. But ok, not about this bug.

But I don't understand its lifetime management.
Is DisconnectFromOwner not overridden for example? what guarantees the extra refcnt is released?

Comment 10

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

Yeah, I definitely don't understand MediaQueryList's memory management.
What guarantees it gets kill. 
What if it has event listener for 'change' and the listener has a pointer to document object?

Comment 11

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

Looks like BroadcastChannel should override DisconnectFromOwner too.

Comment 12

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

And even then need to go through all these keepalive objects to ensure we actually release them eventually.

Comment 13

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

Ah, BroadcastChannel uses inner-window-destroyed, so it should be fine.

Comment 14

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

ah, Document traverses MediaQueryList

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 1 - KeepAliveIfHasListenerFor()

Comment 16

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

Comment on attachment 8858741 [details] [diff] [review]
part 1 - KeepAliveIfHasListenerFor()

>+++ b/dom/base/WebSocket.h
>@@ -46,17 +46,20 @@ public:
>   };
> 
> public:
>   NS_DECL_ISUPPORTS_INHERITED
>   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(WebSocket, DOMEventTargetHelper)
>   virtual bool IsCertainlyAliveForCC() const override;
> 
>   // EventTarget
>+  using EventTarget::EventListenerAdded;
>   virtual void EventListenerAdded(nsIAtom* aType) override;
>+
>+  using EventTarget::EventListenerRemoved;
>   virtual void EventListenerRemoved(nsIAtom* aType) override;

ok, the 'using' is for nsAString form right? I hope you verified this works correctly.
>@@ -190,16 +193,17 @@ DOMEventTargetHelper::RemoveEventListene
>                                           nsIDOMEventListener* aListener,
>                                           bool aUseCapture)
> {
>   EventListenerManager* elm = GetExistingListenerManager();
>   if (elm) {
>     elm->RemoveEventListener(aType, aListener, aUseCapture);
>   }
> 
>+  MaybeUpdateKeepAlive();
There shouldn't be need for this

> DOMEventTargetHelper::AddEventListener(const nsAString& aType,
>                                        nsIDOMEventListener* aListener,
>@@ -215,16 +219,18 @@ DOMEventTargetHelper::AddEventListener(c
>   if (aOptionalArgc < 2) {
>     nsresult rv = WantsUntrusted(&aWantsUntrusted);
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>   EventListenerManager* elm = GetOrCreateListenerManager();
>   NS_ENSURE_STATE(elm);
>   elm->AddEventListener(aType, aListener, aUseCapture, aWantsUntrusted);
>+
>+  MaybeUpdateKeepAlive();
or this

>   ret

> DOMEventTargetHelper::AddEventListener(const nsAString& aType,
>                                        EventListener* aListener,
>                                        const AddEventListenerOptionsOrBoolean& aOptions,
>                                        const Nullable<bool>& aWantsUntrusted,
>@@ -243,16 +249,18 @@ DOMEventTargetHelper::AddEventListener(c
> 
>   EventListenerManager* elm = GetOrCreateListenerManager();
>   if (!elm) {
>     aRv.Throw(NS_ERROR_UNEXPECTED);
>     return;
>   }
> 
>   elm->AddEventListener(aType, aListener, aOptions, wantsUntrusted);
>+
>+  MaybeUpdateKeepAlive();
or this

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4ad5693a0b9
Implement DOMEventTargetHelper::KeepAliveIfHasListenersFor, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb167221f1cf
Use of DOMEventTargetHelper::KeepAliveIfHasListenersFor in BroadcastChannel, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/00142b44a4e4
Use of DOMEventTargetHelper::KeepAliveIfHasListenersFor in MediaQueryList, r=smaug

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/c4ad5693a0b9
https://hg.mozilla.org/mozilla-central/rev/fb167221f1cf
https://hg.mozilla.org/mozilla-central/rev/00142b44a4e4

Comment 19

[Boris Zbarsky [:bzbarsky]]

Andrea, could we use this infrastructure for nsDOMDataChannel too?

Comment 20

[Andrea Marchesini [:baku]]

Yes, I think it would work well. I'll file a bug.