902859 - Prevent to call EnableDevice(X) more times than it calls DisableDevice(X) in nsEventListenerManager

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Jinho Hwang [:jeffhwang]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

If there are several listeners for a same event-type on a window, we couldn't remove them from observer server. 

We remove listener's info from observer array too early at http://dxr.mozilla.org/mozilla-central/source/content/events/src/nsEventListenerManager.cpp#l485 

But, we don't exactly remove the listener at here http://dxr.mozilla.org/mozilla-central/source/content/events/src/nsEventListenerManager.cpp#l513 
because we have other listeners for the type on a window.

After this happened,  we can freely add redundant observers even though they have same listener, type and flags because it is not in observer array of EventListenerManager. 
http://dxr.mozilla.org/mozilla-central/source/content/events/src/nsEventListenerManager.cpp#l243 


Ex) 
1. turn on/off screen again repeatedly
2. using data(wifi / cellular) continuously 

The listeners for Network-Upload/Download-Events are piled up in a observer server and observer array.
This is solved from Bug 901416 but that's a workaround for a specific event, NetworkEvent. There might be some other cases that uses multiple listeners for a type of event. 

So, I think we need to do some thing for the EventListener manager.

Comment 1

[Jinho Hwang [:jeffhwang]]

Attachment: nsEventListenerManager.patch

Comment 2

[Jinho Hwang [:jeffhwang]]

Comment on attachment 787430 [details] [diff] [review]
nsEventListenerManager.patch

The attachment is a tiny sample patch for this. 
I'd like to hear your opinion about this before proceeding further.

Comment 3

[Justin Lebar (not reading bugmail)]

The previous behavior was to call DisableTimeChangeNotifications when there were 0 event listeners with aType == NS_MOZ_TIME_CHANGE_EVENT.

With this patch, we call DisableTimeChangeNotifications whenever we remove one event listener with aType == NS_MOZ_TIME_CHANGE_EVENT.  Is that right?

The issue you're trying to fix is that we currently call DisableTimeChangeNotifications once every time we add a new listener with aType == NS_MOZ_TIME_CHANGE_EVENT, but we call DisableTimeChangeNotifications only once we've gotten rid of all the NS_MOZ_TIME_CHANGE_EVENT listeners?

If I'm understanding this correctly, then I think the right thing to do would be to change the logic in AddEventListenerManagerInternal so that it matches the existing logic in RemoveEventListenerManagerInternal.  That is, we should call Enable and Disable only once, instead of calling Enable and Disable N times.

Comment 4

[Jinho Hwang [:jeffhwang]]

(In reply to Justin Lebar [:jlebar] (limited availability 8/9 – 8/12) from comment #3)
Thank you for the feedback.

> The previous behavior was to call DisableTimeChangeNotifications when there
> were 0 event listeners with aType == NS_MOZ_TIME_CHANGE_EVENT.
  Yes, actually, the previous behaviour was to call DisableTimeChangeNotifications when there is no other listeners except the one requested removing the event listener.
 
> With this patch, we call DisableTimeChangeNotifications whenever we remove
> one event listener with aType == NS_MOZ_TIME_CHANGE_EVENT.  Is that right?
  this patch will remove a event listener only when there is a event listener created by requester.
 
> The issue you're trying to fix is that we currently call
> DisableTimeChangeNotifications once every time we add a new listener with
> aType == NS_MOZ_TIME_CHANGE_EVENT, but we call
> DisableTimeChangeNotifications only once we've gotten rid of all the
> NS_MOZ_TIME_CHANGE_EVENT listeners?
  Yes, the problem is that we keep adding observer to ObserverServer in AddEventListener but we only call remove observer from it once. 
Because we have different polity to control listeners in AddEventListerner and RemoveEventListener. So, I thought we need to have a Consistency policy between them. 

> If I'm understanding this correctly, then I think the right thing to do
> would be to change the logic in AddEventListenerManagerInternal so that it
> matches the existing logic in RemoveEventListenerManagerInternal.  That is,
> we should call Enable and Disable only once, instead of calling Enable and
> Disable N times.
Yes, I agree with you. We don't need to call N times Enable but currently we are doing that. I'll upload a small patch for this.

Comment 5

[Jinho Hwang [:jeffhwang]]

Attachment: nsEventListenerManager.patch

something like this we can have a same policy for add / remove event listeners.

Comment 6

[Jinho Hwang [:jeffhwang]]

Attachment: nsEventListenerManager.patch

patch for hg

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 788034 [details] [diff] [review]
nsEventListenerManager.patch

I think this is the right idea, but I think it has a bug.

>diff --git a/content/events/src/nsEventListenerManager.cpp b/content/events/src/nsEventListenerManager.cpp
>--- a/content/events/src/nsEventListenerManager.cpp
>+++ b/content/events/src/nsEventListenerManager.cpp

>@@ -213,25 +213,25 @@ nsEventListenerManager::AddEventListener
>   if (!aListener || mClearingListeners) {
>     return;
>   }
> 
>   // Since there is no public API to call us with an EventListenerHolder, we
>   // know that there's an EventListenerHolder on the stack holding a strong ref
>   // to the listener.
> 
>+  bool isAlreadyInList = false;
>   nsListenerStruct* ls;
>   uint32_t count = mListeners.Length();
>   for (uint32_t i = 0; i < count; i++) {
>     ls = &mListeners.ElementAt(i);
>-    if (ls->mListener == aListener &&
>-        ls->mListenerIsHandler == aHandler &&
>+    if (ls->mListenerIsHandler == aHandler &&
>         ls->mFlags == aFlags &&
>         EVENT_TYPE_EQUALS(ls, aType, aTypeAtom, aAllEvents)) {
>-      return;
>+      isAlreadyInList = true;
>     }
>   }

This breaks the behavior of this method in a significant way.

Before, if you had an event handler H of for event E registered, and then you
re-registered the handler, we'd hit the early return above.  But with this
patch, we add the duplicate event handler to the list:

>   ls = aAllEvents ? mListeners.InsertElementAt(0) : mListeners.AppendElement();
>   ls->mListener = aListener;

That's not right.

Comment 8

[Jinho Hwang [:jeffhwang]]

(In reply to Justin Lebar [:jlebar] from comment #7)
Thanks for the feedback. 

> This breaks the behavior of this method in a significant way.
> Before, if you had an event handler H of for event E registered, and then you
> re-registered the handler, we'd hit the early return above.  But with this
> patch, we add the duplicate event handler to the list:
> That's not right.

Okay, you are right. I'll attach new one for this.

Comment 9

[Jinho Hwang [:jeffhwang]]

Attachment: nsEventListenerManager.patch

Comment 10

[Justin Lebar (not reading bugmail)]

>diff --git a/content/events/src/nsEventListenerManager.cpp b/content/events/src/nsEventListenerManager.cpp
>--- a/content/events/src/nsEventListenerManager.cpp
>+++ b/content/events/src/nsEventListenerManager.cpp

>@@ -205,25 +205,29 @@ nsEventListenerManager::AddEventListener
>   NS_ABORT_IF_FALSE(aType && aTypeAtom, "Missing type");
> 
>   if (!aListener || mClearingListeners) {
>     return;
>   }
> 
>   nsRefPtr<nsIDOMEventListener> kungFuDeathGrip = aListener;
> 
>+  bool isAlreadyInList = false;

This really means something like "is the type already in the list?", so can we
change the name?

>   nsListenerStruct* ls;
>   uint32_t count = mListeners.Length();
>   for (uint32_t i = 0; i < count; i++) {
>     ls = &mListeners.ElementAt(i);
>-    if (ls->mListener == aListener &&
>-        ls->mListenerIsHandler == aHandler &&
>-        ls->mFlags == aFlags &&
>+    if (ls->mFlags == aFlags &&
>         EVENT_TYPE_EQUALS(ls, aType, aTypeAtom)) {

Why do we check ls->mFlags == aFlags in this branch, as opposed to the one
below?

>-      return;
>+      // Detect duplicated EventListener 
>+      if (ls->mListener == aListener &&
>+          ls->mListenerIsHandler == aHandler ) {

No space before ")".

>+        return;
>+      }
>+      isAlreadyInList = true;      
>     }
>   }
> 
>   mNoListenerForEvent = NS_EVENT_TYPE_NULL;
>   mNoListenerForEventAtom = nullptr;
> 
>   ls = mListeners.AppendElement();
>   ls->mListener = aListener;

>@@ -238,16 +242,21 @@ nsEventListenerManager::AddEventListener
>   if (aFlags & NS_PRIV_EVENT_FLAG_SCRIPT) {
>     ls->mListenerType = eJSEventListener;
>   } else if ((wjs = do_QueryInterface(aListener))) {
>     ls->mListenerType = eWrappedJSListener;
>   } else {
>     ls->mListenerType = eNativeListener;
>   }
> 
>+  // The type we are trying to add is already in the list
>+  // So, we just add it to listener array only.
>+  if(isAlreadyInList) {
>+    return;
>+  }

Nit: Space before "(".

>@@ -428,25 +437,24 @@ nsEventListenerManager::RemoveEventListe
>   uint32_t typeCount = 0;
>   bool deviceType = IsDeviceType(aType);
> #ifdef MOZ_B2G
>   bool timeChangeEvent = (aType == NS_MOZ_TIME_CHANGE_EVENT);
>   bool networkEvent = (aType == NS_NETWORK_UPLOAD_EVENT ||
>                        aType == NS_NETWORK_DOWNLOAD_EVENT);
> #endif // MOZ_B2G
> 
>-  for (uint32_t i = 0; i < count; ++i) {
>-    ls = &mListeners.ElementAt(i);
>+  for (uint32_t i = count; i > 0; --i) {
>+  ls = &mListeners.ElementAt(i-1); 
>     if (EVENT_TYPE_EQUALS(ls, aType, aUserType)) {
>       ++typeCount;
>       if (ls->mListener == aListener &&
>           (ls->mFlags & ~NS_PRIV_EVENT_UNTRUSTED_PERMITTED) == aFlags) {
>         nsRefPtr<nsEventListenerManager> kungFuDeathGrip = this;
>-        mListeners.RemoveElementAt(i);
>-        --count;
>+        mListeners.RemoveElementAt(i-1);
>         mNoListenerForEvent = NS_EVENT_TYPE_NULL;
>         mNoListenerForEventAtom = nullptr;
> 
>         if (!deviceType
> #ifdef MOZ_B2G
>             && !timeChangeEvent && !networkEvent
> #endif // MOZ_B2G
>             ) {

Why did you need to change this hunk?  If it's just a change for efficiency (I
agree it should be more efficient), let's do that in another bug, if you don't
mind.

Comment 11

[Jinho Hwang [:jeffhwang]]

(In reply to Justin Lebar [:jlebar] from comment #10)

Thanks Justin for the review.
> This really means something like "is the type already in the list?", so can
> we
> change the name?
Sure, any suggestion for the name? how about just simply 'foundInList'?

> Why do we check ls->mFlags == aFlags in this branch, as opposed to the one
> below?
You're right no need to check that.

> Why did you need to change this hunk?  If it's just a change for efficiency
> (I
> agree it should be more efficient), let's do that in another bug, if you
> don't
> mind.
Okay, no problem with me. Let's do this in another bug. 
I'll explain the reason I changed it there.

And I opened new bug for the event listener leaking in Gaia System application. 
Bug 907505.

Comment 12

[Justin Lebar (not reading bugmail)]

> how about just simply 'foundInList'?

I think we should include "type" in there; otherwise it's ambiguous whether we found the type or the listener in the list (we're searching for both).

Maybe typeFoundInList, or typeIsInList, or typeInList?

Comment 13

[Jinho Hwang [:jeffhwang]]

(In reply to Justin Lebar [:jlebar] from comment #12)
> Maybe typeFoundInList, or typeIsInList, or typeInList?
Okay, typeFoundInList is good I think.

And I just remembered that why I put the "ls->mFlags == aFlags" in the if statement.
If we have different flags on event we need to keep the responding observers to catch the right event. Especially, for the touch and mouse event type, Capturing & Bubbling to handle focus or other behaviour.

So, We need to keep the "ls->mFlags == aFlags" condition in the if statement.

I'll upload revised patch. 
Thank you.

Comment 14

[Justin Lebar (not reading bugmail)]

The purpose of typeFoundInList is to prevent us from calling EnableDevice(NS_DEVICE_XXX) multiple times, right?

So I don't understand why we need the flags condition.  Can you elaborate a bit more?

Note that if we do need the flags condition, I think the patch is still not right, because with this patch it's possible that we call EnableDevice(NS_DEVICE_XXX) twice, but then we'll only call DisableDevice(NS_DEVICE_XXX) once, as I read the code.

Comment 15

[Jinho Hwang [:jeffhwang]]

Attachment: nsEventListenerManager.patch

Comment 16

[Jinho Hwang [:jeffhwang]]

(In reply to Justin Lebar [:jlebar] from comment #14)

> The purpose of typeFoundInList is to prevent us from calling
> EnableDevice(NS_DEVICE_XXX) multiple times, right?
Yes, right.

> So I don't understand why we need the flags condition.  Can you elaborate a
> bit more?
Sorry for that. We don't need the flags condition but we need to change the return statement line after set the mMayHaveCapturingListeners. So, the listener can handle the bubble event. Look at http://dxr.mozilla.org/mozilla-central/source/content/events/src/nsEventListenerManager.h#l316

The attachment #793330 [details] [diff] [review] I changed it.

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 793330 [details] [diff] [review]
nsEventListenerManager.patch

This looks pretty good.

It looks like this patch does not apply to trunk.  (My trunk doesn't have a
kungFuDeathGrip, for instance.)  Do you intend for us to take this patch on
b2g18?  I don't think triage would go for that, because this is a just-in-case
sort of fix.

I'd like to see a patch which applies against trunk before I r+, but aside from
that, I only have nits.

Maybe when we land this we should also back out the change we made to
nsGlobalWindow (bug 901416)?

># HG changeset patch
># Parent 0c8931d85f0ac9d1663d13e3882f377e7e1f496e
># User Jinho Hwang <[e-mail]>
>Bug 902859 - Event listener manager is leaking event listeners if there are several listeners for a type.

As I understand this bug, the EventListenerManager doesn't leak anything.  The
issue is that the EventListenerManager calls EnableDevice(X) more times than it
calls DisableDevice(X).  Maybe you can say that instead?

>diff --git a/content/events/src/nsEventListenerManager.cpp b/content/events/src/nsEventListenerManager.cpp
>--- a/content/events/src/nsEventListenerManager.cpp
>+++ b/content/events/src/nsEventListenerManager.cpp

>@@ -205,25 +205,29 @@ nsEventListenerManager::AddEventListener
>   NS_ABORT_IF_FALSE(aType && aTypeAtom, "Missing type");
> 
>   if (!aListener || mClearingListeners) {
>     return;
>   }
> 
>   nsRefPtr<nsIDOMEventListener> kungFuDeathGrip = aListener;
> 
>+  bool typeFoundInList = false;
>   nsListenerStruct* ls;
>   uint32_t count = mListeners.Length();
>   for (uint32_t i = 0; i < count; i++) {
>     ls = &mListeners.ElementAt(i);
>-    if (ls->mListener == aListener &&
>-        ls->mListenerIsHandler == aHandler &&
>-        ls->mFlags == aFlags &&
>-        EVENT_TYPE_EQUALS(ls, aType, aTypeAtom)) {
>-      return;
>+    if (EVENT_TYPE_EQUALS(ls, aType, aTypeAtom)) {
>+      // Detect duplicated EventListener 

s/duplicated/duplicate

>+      if (ls->mListener == aListener &&
>+          ls->mListenerIsHandler == aHandler
>+          ls->mFlags == aFlags) {
>+        return;
>+      }
>+      typeFoundInList = true;      
>     }
>   }
> 
>   mNoListenerForEvent = NS_EVENT_TYPE_NULL;
>   mNoListenerForEventAtom = nullptr;
> 
>   ls = mListeners.AppendElement();
>   ls->mListener = aListener;

>@@ -246,16 +250,22 @@ nsEventListenerManager::AddEventListener
> 
>   if (aFlags & NS_EVENT_FLAG_SYSTEM_EVENT) {
>     mMayHaveSystemGroupListeners = true;
>   }
>   if (aFlags & NS_EVENT_FLAG_CAPTURE) {
>     mMayHaveCapturingListeners = true;
>   }
> 
>+  // The type we are trying to add is already in the list
>+  // So, we just add it to listener array only.
>+  if (isAlreadyInList) {
>+    return;
>+  }

I don't think this will compile as-is.  :)

Comment 18

[Jinho Hwang [:jeffhwang]]

Attachment: nsEventListenerManager.patch

Comment 19

[Jinho Hwang [:jeffhwang]]

Attachment: nsEventListenerManager_m-c.patch

patch for the M-C

Comment 20

[Jinho Hwang [:jeffhwang]]

(In reply to Justin Lebar [:jlebar] from comment #17)
Thank you for the feedback.

> Do you intend for us to take this patch on b2g18?  
> I don't think triage would go for that, because this is a
> just-in-case sort of fix.
Yeh, For b2g18.. I think it's too late and not much important.
But for m-c, we need this because without this we needlessly keep asking to add new observer to window. And if there is no duplication check routine, it will have many duplicate observers for a type of event.
  
> Maybe when we land this we should also back out the change we made to
> nsGlobalWindow (bug 901416)?
After this, that patch will be unnecessary but I think it's better to keep the duplication check too just in case..

Comment 21

[Justin Lebar (not reading bugmail)]

> After this, that patch will be unnecessary but I think it's better to keep the 
> duplication check too just in case..

If you want something just-in-case, how about we turn the duplication-check in nsGlobalWindow into an assertion?  You can make the bools there DebugOnly<bool> and then use MOZ_ASSERT to check them.

That's even better than leaving things how they are, because if something breaks with the code here, we'll find out.

Comment 22

[Justin Lebar (not reading bugmail)]

Comment on attachment 794441 [details] [diff] [review]
nsEventListenerManager.patch

Moving the r? over to the m-c patch, per comment 20.

Comment 23

[Justin Lebar (not reading bugmail)]

Sorry, I missed something in my last review (or otherwise, this is different between b2g18 and m-c).

I know it's safe to change how frequently we call EnableDevice(), but this patch changes how frequently we call a bunch of other things (everything under the |return|).  I think most of these are OK to change, but I'm not sure about the very last thing in the function, the call to mTarget->EventListenerAdded().

Looking at the code that this runs, I think we probably want to call that even if typeInList.

Sorry to send this back for another review; we're almost there.

While we're at it, here's one more nit:

> Bug 902859 - Prevent to call EnableDevice(X) more times than it calls DisableDevice(X) in nsEventListenerManager

"Prevent" takes a direct object + "from" + a gerund.  I don't think it ever takes an infinitive.  So a better wording would be "Prevent nsEventListenerManager from calling EnableDevice(X) more times than it calls DisableDevice(X)."  Alternatively, you could say "Don't call EnableDevice(X) more times than we call DisableDevice(X) in nsEventListenerManager."

Comment 24

[Jinho Hwang [:jeffhwang]]

Attachment: nsEventListenerManager_m-c.patch

Comment 25

[Jinho Hwang [:jeffhwang]]

(In reply to Justin Lebar [:jlebar] from comment #23)
Thanks Justin for the feedback.

> I'm not sure about the very last thing in the function, the call to
> mTarget->EventListenerAdded().
> Looking at the code that this runs, I think we probably want to call that
> even if typeInList.
  Yes, you are right. I missed that.
 
> So a better wording would be "Prevent
> nsEventListenerManager from calling EnableDevice(X) more times than it calls
> DisableDevice(X)." 
  Changed. :)

There is one additional change. I put needToCheckFlags for touch events. Because if the first request of touch event has a type with system event flag, we don't actually call SetHasTouchEventListeners but the type of touch event is added on the list. So, we'll lose the chance to add touch event observer for the late requsets which have a type without system event flag.

So, I just set the feedback flag to you. 
Let me know if I miss something again.

Comment 26

[Justin Lebar (not reading bugmail)]

Comment on attachment 795811 [details] [diff] [review]
nsEventListenerManager_m-c.patch

>diff --git a/content/events/src/nsEventListenerManager.cpp b/content/events/src/nsEventListenerManager.cpp
>--- a/content/events/src/nsEventListenerManager.cpp
>+++ b/content/events/src/nsEventListenerManager.cpp

>@@ -237,25 +237,47 @@ nsEventListenerManager::AddEventListener
>   if (!aListener || mClearingListeners) {
>     return;
>   }
> 
>   // Since there is no public API to call us with an EventListenerHolder, we
>   // know that there's an EventListenerHolder on the stack holding a strong ref
>   // to the listener.
> 
>+  bool typeFoundInList = false;
>+  bool needToCheckFlags = false;
>+  if ((aTypeAtom == nsGkAtoms::ontouchstart ||
>+          aTypeAtom == nsGkAtoms::ontouchend ||
>+          aTypeAtom == nsGkAtoms::ontouchmove ||
>+          aTypeAtom == nsGkAtoms::ontouchenter ||
>+          aTypeAtom == nsGkAtoms::ontouchleave ||
>+          aTypeAtom == nsGkAtoms::ontouchcancel) &&
>+       !aFlags.mInSystemGroup) {
>+    needToCheckFlags = true;
>+  }

Be careful with indentation; if we left it as is, the |aTypeAtom|s should all
lign up, and the "a" in "!aFlags" should line up with the "a" in aTypeAtom.

But we should just change this to

bool needToCheckFlags = (aTypeAtom == nsGkAtoms::ontouchstart ||
                         aTypeAtom == nsGKAtoms::ontouchend || 
			 ...) &&
                        !aFlags.mInSystemGroup;

Also please add a comment explaining why needToCheckFlags is necessary.

>   nsListenerStruct* ls;
>   uint32_t count = mListeners.Length();
>   for (uint32_t i = 0; i < count; i++) {
>     ls = &mListeners.ElementAt(i);
>-    if (ls->mListener == aListener &&
>-        ls->mListenerIsHandler == aHandler &&
>-        ls->mFlags == aFlags &&
>-        EVENT_TYPE_EQUALS(ls, aType, aTypeAtom, aTypeString, aAllEvents)) {
>-      return;
>+    if (EVENT_TYPE_EQUALS(ls, aType, aTypeAtom, aTypeString, aAllEvents)) {
>+      // Detect duplicate EventListener 
>+      if (ls->mListener == aListener &&
>+          ls->mListenerIsHandler == aHandler &&
>+          ls->mFlags == aFlags) {
>+        return;
>+      }
>+      if (needToCheckFlags) {
>+        if (!aFlags.mInSystemGroup) {
>+          typeFoundInList = true;
>+          break;
>+        }
>+      } else {
>+        typeFoundInList = true;
>+        break;
>+      }
>     }
>   }

This is pretty sub-optimal, because it duplicates a lot of complicated logic
from down below.  With this change, plus the big typeFoundInList change above,
I feel like we're inviting regressions here.

>+void
>+nsEventListenerManager::AddEventListenerToWindow(
>+                          uint32_t aType,
>+                          nsIAtom* aTypeAtom,
>+                          const EventListenerFlags& aFlags)

This should either be indented as

  nsEventListenerManager::AddEventListenerToWindow(uint32_t aType,
                                                   nsIAtom* aTypeAtom,
						   const EventListenerFlags& aFlags)

or, if that would cause the line to be longer than 80 chars, we indent the args
two spaces, as:

  nsEventListenerManager::AddEventListenerToWindow(
    uint32_t aType,
    nsIAtom* aTypeAtom,
    const EventListenerFlags& aFlags);

Sorry to continue kicking this can, but I think smaug should take over this
bug.  My last day at Mozilla is on Friday, so he's the one who's going to have
to deal with any ugliness here.  If he's OK with it, then this can go in.

Thanks for your continued patience here.

Comment 27

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

Comment on attachment 795811 [details] [diff] [review]
nsEventListenerManager_m-c.patch

Please add some helper method for 
+ aTypeAtom == nsGkAtoms::ontouchstart ||
+          aTypeAtom == nsGkAtoms::ontouchend ||
+          aTypeAtom == nsGkAtoms::ontouchmove ||
+          aTypeAtom == nsGkAtoms::ontouchenter ||
+          aTypeAtom == nsGkAtoms::ontouchleave ||
+          aTypeAtom == nsGkAtoms::ontouchcancel
We have that now twice in the file.
Something like

bool
IsTouchEventType(nsIAtom* aTypeAtom)
{
   return aTypeAtom == nsGkAtoms::ontouchstart ||
          aTypeAtom == nsGkAtoms::ontouchend ||
          aTypeAtom == nsGkAtoms::ontouchmove ||
          aTypeAtom == nsGkAtoms::ontouchenter ||
          aTypeAtom == nsGkAtoms::ontouchleave ||
          aTypeAtom == nsGkAtoms::ontouchcancel;  
}


But the patch doesn't work for ondeviceproximity and onuserproximity.
They both enable NS_DEVICE_PROXIMITY.

Comment 28

[Jinho Hwang [:jeffhwang]]

Attachment: nsEventListenerManager_m-c.patch

Comment 29

[Jinho Hwang [:jeffhwang]]

(In reply to Olli Pettay [:smaug] from comment #27)
Thank you for the review.

> But the patch doesn't work for ondeviceproximity and onuserproximity.
> They both enable NS_DEVICE_PROXIMITY.
Could you give me the detail? 
I wonder how the patch doesn't work only for ondeviceproximity and onuserproximity.

Comment 30

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

If the idea is to enable something only once, adding ondeviceproximity and onuserproximity
ends up activating NS_DEVICE_PROXIMITY twice.

Comment 31

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

I think the best option would be to not use NS_DEVICE_PROXIMITY for two thing in ELM, 
and in fact we do have  NS_USER_PROXIMITY and nsEventListenerManager::EnableDevice can handle that.
So just do something like 
  } else if (aTypeAtom == nsGkAtoms::ondeviceproximity) {
    EnableDevice(NS_DEVICE_PROXIMITY);
  else if (aTypeAtom == nsGkAtoms::onuserproximity) {
    EnableDevice(NS_USER_PROXIMITY);
  }


I know nsEventListenerManager::EnableDevice maps those currently to the same thing, but still, better
be consistent.

Comment 32

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

Comment on attachment 798690 [details] [diff] [review]
nsEventListenerManager_m-c.patch

And sorry about the delay in review. Ping me on IRC if you need faster reviews ;)

Comment 33

[Jinho Hwang [:jeffhwang]]

Attachment: nsEventListenerManager_m-c.patch

Comment 34

[Jinho Hwang [:jeffhwang]]

(In reply to Olli Pettay [:smaug] from comment #31)
Thank you for the review.

> I know nsEventListenerManager::EnableDevice maps those currently to the same
> thing, but still, better
> be consistent.
 I agree with u. I change that as you said.

Comment 35

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

Comment on attachment 805156 [details] [diff] [review]
nsEventListenerManager_m-c.patch


>+  // We only add touchevent listeners to window when the flag is not in
>+  // the system group. Otherwise, we ignore them.
>+  // So, we need to check the flag type for touchevents.
>+  bool needToCheckFlags = IsTouchEventType(aTypeAtom) &&
>+                          !aFlags.mInSystemGroup;
Perhaps rename needToCheckFlags to checkOnlyDefaultGroupListeners


>+      if (needToCheckFlags) {
>+        if (!ls->mFlags.mInSystemGroup) {
>+          typeFoundInList = true;
>+          break;
>+        }
>+      } else {
>+        typeFoundInList = true;
>+        break;
>+      }
And then write this
        if (!checkOnlyDefaultGroupListeners || !ls->mFlags.mInSystemGroup) {
          typeFoundInList = true;
          break;
        }

Comment 36

[[:fabrice] Fabrice Desré]

Jinho, does anything prevent this bug from landing?

Comment 37

[Jinho Hwang [:jeffhwang]]

Attachment: nsEventListenerManager_m-c.patch

Sorry for the tremendous delay in updates. I've been sick.

I changed as pointed out on comment #35.

Comment 38

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

Comment on attachment 8355137 [details] [diff] [review]
nsEventListenerManager_m-c.patch

> 
>+  bool typeFoundInList = false;
>+
>+  // We only add touchevent listeners to window when the flag is not in
We only notify window about touch event listeners when the listener is not in


>+  // the system group. Otherwise, we ignore them.
>+  // So, we need to check the flag type for touchevents.
>+  bool checkOnlyDefaultGroupListeners = IsTouchEventType(aTypeAtom) &&
>+                          !aFlags.mInSystemGroup;
Odd indentation. Align !aFlags.mInSystemGroup; under IsTouchEventType(aTypeAtom) &&


>   nsListenerStruct* ls;
>   uint32_t count = mListeners.Length();
>   for (uint32_t i = 0; i < count; i++) {
>     ls = &mListeners.ElementAt(i);
>-    // mListener == aListener is the last one, since it can be a bit slow.
>-    if (ls->mListenerIsHandler == aHandler &&
>-        ls->mFlags == aFlags &&
>-        EVENT_TYPE_EQUALS(ls, aType, aTypeAtom, aTypeString, aAllEvents) &&
>-        ls->mListener == aListener) {
>-      return;
>+    if (EVENT_TYPE_EQUALS(ls, aType, aTypeAtom, aTypeString, aAllEvents)) {
>+      // Detect duplicate EventListener 
>+      if (ls->mListener == aListener &&
>+          ls->mListenerIsHandler == aHandler &&
>+          ls->mFlags == aFlags) {
Move ls->mListener == aListener to be the last one in the 'if', and don't remove the comment why it is the last one.

>+        return;
>+      }
>+      if (!checkOnlyDefaultGroupListeners || !ls->mFlags.mInSystemGroup) {
>+          typeFoundInList = true;
>+          break;
How can you have the break; here?
If one does
target.addEventListener("touchdown", listener1);
target.addEventListener("touchdown", listener2);
target.addEventListener("touchdown", listener2);

you end up adding listener2 twice.

>+nsEventListenerManager::AddEventListenerToWindow(
Call this method NotifyEventListenerAdded

Comment 39

[Eric Rahm [:erahm]]

Closing out old Firefox OS specific memshrink bugs.