nsIEventListenerService: tracking of dynamically added and removed event listeners

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
10 years ago
a month ago

People

(Reporter: Honza, Assigned: smaug)

Tracking

({dev-doc-needed})

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [firebug-p2])

Attachments

(2 attachments)

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
Whiteboard: [firebug-p1]
(Assignee)

Updated

10 years ago
Version: 1.9.1 Branch → Trunk

Comment 1

10 years ago
i'm pretty sure i wanted this while i was @cenzic.
Component: JavaScript Debugging APIs → DOM
QA Contact: jsd → general
OS: Windows XP → All
Hardware: x86 → All

Updated

8 years ago
Whiteboard: [firebug-p1] → [firebug-p2]
(Assignee)

Comment 2

4 years ago
Posted patch v1Splinter Review
bgrins, would this be good enough?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a03d7bfc38d8
Attachment #8597034 - Flags: feedback?(bgrinstead)
(Assignee)

Updated

4 years ago
Attachment #8597034 - Flags: review?(masayuki)
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
Attachment #8597034 - Flags: feedback?(bgrinstead) → feedback+
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()?
Attachment #8597034 - Flags: review?(masayuki) → review-
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
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.
Attachment #8597034 - Flags: review?(masayuki)
(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 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.
Attachment #8597034 - Flags: review?(masayuki)
Attachment #8597034 - Flags: review-
Attachment #8597034 - Flags: review+
(Assignee)

Comment 9

4 years ago
> >+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
(Assignee)

Comment 10

4 years ago
Posted patch v2Splinter Review
Assignee: nobody → bugs
https://hg.mozilla.org/mozilla-central/rev/a4c49454dd2f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(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.)
(Assignee)

Comment 15

4 years ago
Why wouldn't it work like any other callback? I'm not worried about that case.
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.
(Assignee)

Updated

4 years ago
Depends on: 1160181
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.