524674 - nsIEventListenerService: tracking of dynamically added and removed event listeners

Status: RESOLVED FIXED

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

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This bug is closely related to the work made on:
https://bugzilla.mozilla.org/show_bug.cgi?id=506961
https://bugzilla.mozilla.org/show_bug.cgi?id=448602

It would be great if nsIEventListenerService supports also registering
callbacks that are called when a new event listener is added or an existing
removed. This would allow to keep the event panel (displaying list of event listeners) up to date without necessity to entirely refresh the view (which can be time consuming).

Honza

Comment 1

[timeless]

i'm pretty sure i wanted this while i was @cenzic.

Comment 2

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: v1

bgrins, would this be good enough?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a03d7bfc38d8

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8597034 [details] [diff] [review]
v1

Review of attachment 8597034 [details] [diff] [review]:
-----------------------------------------------------------------

This works very well!  I have put together a quick patch in Bug 1157469 using this API and have confirmed it works when adding and removing event listeners on elements in the inspector

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8597034 [details] [diff] [review]
v1

Hmm, if you think EventListenerService should work as singleton, you should:

* hide its constructor (i.e., define it in private or protected) like the destructor
* make nsLayoutModule.cpp use NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR
* drop NS_NewEventListenerService() in EventListenerService.cpp

Additionally, don't we need to check script blocker? And how about if the instance is destroyed by shutting down XPCOM during the loop in NotifyPendingChanges()?

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Why would we need to check script blockers? The API calls the callback async.

>make nsLayoutModule.cpp use NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR
Well, then ctor needs to be public.

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8597034 [details] [diff] [review]
v1

I could use the macro, and not NS_NewEventListenerService(), but
that is really not about this bug.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #5)
> Why would we need to check script blockers? The API calls the callback async.

Ah, I got it.

> >make nsLayoutModule.cpp use NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR
> Well, then ctor needs to be public.

I don't think so, only EventUtils::GetInstance() should be public.

(In reply to Olli Pettay [:smaug] from comment #6)
> I could use the macro, and not NS_NewEventListenerService(), but
> that is really not about this bug.

Okay, then, you shouldn't use MOZ_ASSERT() at checking the instance in the destructor.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8597034 [details] [diff] [review]
v1

>+EventListenerService*
>+EventListenerService::sInstance = nullptr;
>+
>+EventListenerService::EventListenerService()
>+{
>+  MOZ_ASSERT(!sInstance);
>+  sInstance = this;
>+}
>+
>+EventListenerService::~EventListenerService()
>+{
>+  MOZ_ASSERT(sInstance == this);

So, please use NS_ASSERTION() until we make this class completely singleton.

>+NS_IMETHODIMP
>+EventListenerService::AddListenerChangeListener(nsIListenerChangeListener* aListener)
>+{
>+  if (!mChangeListeners.Contains(aListener)) {
>+    mChangeListeners.AppendElement(aListener);
>+  }
>+  return NS_OK;
>+};
>+
>+

nit: why don't you insert just one empty line?

>+void
>+EventListenerService::NotifyAboutMainThreadListenerChangeInternal(dom::EventTarget* aTarget)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  if (!mChangeListeners.IsEmpty()) {

if(mChangeListeners.IsEmpty()) {
  return;
}

can reduce the following indent.

>+    if (!mPendingListenerChanges) {
>+      mPendingListenerChanges = nsArrayBase::Create();
>+      nsCOMPtr<nsIRunnable> runnable = NS_NewRunnableMethod(this,
>+        &EventListenerService::NotifyPendingChanges);
>+      NS_DispatchToCurrentThread(runnable);
>+    }
>+
>+    if (!mPendingListenerChangesSet.Get(aTarget)) {
>+      mPendingListenerChanges->AppendElement(aTarget, false);

This looks a bit tricky... but it's okay because I have no idea to mPendingListenerChanges becomes nullptr again even if others will insert something between |mPendingListenerChanges = nsArrayBase::Create()| and this line.

>+      mPendingListenerChangesSet.Put(aTarget, true);
>+    }
>+  }
>+}
>+
>+void
>+EventListenerService::NotifyPendingChanges()
>+{
>+  nsCOMPtr<nsIMutableArray> changes;
>+  mPendingListenerChanges.swap(changes);
>+  mPendingListenerChangesSet.Clear();
>+
>+  nsTObserverArray<nsCOMPtr<nsIListenerChangeListener>>::EndLimitedIterator
>+    iter(mChangeListeners);
>+  while (iter.HasMore()) {
>+    nsCOMPtr<nsIListenerChangeListener> listener = iter.GetNext();
>+    listener->ListenersChanged(changes);

Doesn't a call of this cause XPCOM shutdown? If so, you may need to quite from this loop, isn't it?

>+  void addListenerChangeListener(in nsIListenerChangeListener aListener);
>+  void removeListenerChangeListener(in nsIListenerChangeListener aListener);

ListenerChangeListener sounds odd to me (I know it's not wrong name), but I have no better idea.



If my some concerns are wrong or fixed before landing, r=masayuki

And please file a follow up bug to make the class guaranteed as singleton class.

Comment 9

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

> >+EventListenerService::~EventListenerService()
> >+{
> >+  MOZ_ASSERT(sInstance == this);
> 
> So, please use NS_ASSERTION() until we make this class completely singleton.
It is really MOZ_ASSERT style issue if someone else creates another instance


> >+  while (iter.HasMore()) {
> >+    nsCOMPtr<nsIListenerChangeListener> listener = iter.GetNext();
> >+    listener->ListenersChanged(changes);
> 
> Doesn't a call of this cause XPCOM shutdown? If so, you may need to quite
> from this loop, isn't it?
Why? We don't have such checks for other callbacks.

> ListenerChangeListener sounds odd to me
Yeah, it is a bit odd, but couldn't figure out anything better

Comment 10

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: v2

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c49454dd2f

Comment 12

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c49454dd2f

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/a4c49454dd2f

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #9)
> > >+  while (iter.HasMore()) {
> > >+    nsCOMPtr<nsIListenerChangeListener> listener = iter.GetNext();
> > >+    listener->ListenersChanged(changes);
> > 
> > Doesn't a call of this cause XPCOM shutdown? If so, you may need to quite
> > from this loop, isn't it?
> Why? We don't have such checks for other callbacks.

E.g., isn't it possible some listeners try to kill the process, like "Exit" menu? Then, the loop will keep working without crash? (Of course, it's not usual scenario, but I worry that if it causes a security hole.)

Comment 15

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Why wouldn't it work like any other callback? I'm not worried about that case.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I worry about the others too... This sounds similar to checking if widget is destroyed before dispatching an event. But if you're sure it's safe different from widget case, my worry is just wrong.