Closed Bug 1354599 Opened 3 years ago Closed 3 years ago

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

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

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.
Assignee: nobody → amarchesini
Attached patch part 2 - BroadcastChannel (obsolete) — Splinter Review
Attached patch part 3 - MediaQueryList (obsolete) — Splinter Review
Depends on: 1354441
Attachment #8855862 - Attachment is obsolete: true
Attachment #8858280 - Flags: review?(bugs)
Attachment #8855863 - Attachment is obsolete: true
Attachment #8858281 - Flags: review?(bugs)
Attached patch part 3 - MediaQueryList (obsolete) — Splinter Review
Attachment #8855864 - Attachment is obsolete: true
Attachment #8858282 - Flags: review?(bugs)
Attachment #8858282 - Attachment is obsolete: true
Attachment #8858282 - Flags: review?(bugs)
Attachment #8858296 - Flags: review?(bugs)
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.
Attachment #8858280 - Flags: review?(bugs) → review-
Attachment #8858281 - Flags: review?(bugs) → review+
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?
Attachment #8858296 - Flags: review?(bugs) → review-
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?
Looks like BroadcastChannel should override DisconnectFromOwner too.
And even then need to go through all these keepalive objects to ensure we actually release them eventually.
Ah, BroadcastChannel uses inner-window-destroyed, so it should be fine.
ah, Document traverses MediaQueryList
Attachment #8858296 - Flags: review- → review+
Attachment #8858280 - Attachment is obsolete: true
Attachment #8858741 - Flags: review?(bugs)
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
Attachment #8858741 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
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
Andrea, could we use this infrastructure for nsDOMDataChannel too?
Flags: needinfo?(amarchesini)
Yes, I think it would work well. I'll file a bug.
Flags: needinfo?(amarchesini)
Blocks: 1450192
Depends on: 1480179
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.