742376 - DisableDevice is being called with types that are not known device sensors

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Doug Turner (:dougt)]

We are seeing these warnings all of the time:

   NS_WARNING("Disabling an unknown device sensor.");

Caused here:
   http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventListenerManager.cpp#360


The problem is that the test:
  EVENT_TYPE_EQUALS(ls, aType, aUserType)

Is false, and we never increment typeCount.

We drop out of that loop, and call DisableDevice on whatever type is passed in.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

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

Comment on attachment 612198 [details] [diff] [review]
patch v.1

># HG changeset patch
># Parent 087971d348c7b8e002f9695ccbc4871dc2044537
># User Doug Turner <[e-mail]>
>Bug 742376 - DisableDevice is being called with types that are not known device sensors. r=smaug
>
>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
>@@ -372,38 +372,39 @@ nsEventListenerManager::RemoveEventListe
>     return;
>   }
> 
>   nsListenerStruct* ls;
>   aFlags &= ~NS_PRIV_EVENT_UNTRUSTED_PERMITTED;
> 
>   PRUint32 count = mListeners.Length();
>   PRUint32 typeCount = 0;
>+  bool deviceType = IsDeviceType(aType);
> 
>   for (PRUint32 i = 0; i < count; ++i) {
>     ls = &mListeners.ElementAt(i);
>     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;
>         mNoListenerForEvent = NS_EVENT_TYPE_NULL;
>         mNoListenerForEventAtom = nsnull;
> 
>-        if (!IsDeviceType(aType)) {
>+        if (deviceType) {
>           return;
>         }
>         --typeCount;
>       }
>     }
>   }
> 
>-  if (typeCount == 0) {
>+  if (deviceType && typeCount == 0) {
>     DisableDevice(aType);
>   }
> }
> 
> static inline bool
> ListenerCanHandle(nsListenerStruct* aLs, nsEvent* aEvent)
> {
>   // This is slightly different from EVENT_TYPE_EQUALS in that it returns

Comment 3

[Doug Turner (:dougt)]

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

Comment 5

[(no longer active)]

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

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 612198 [details] [diff] [review]
patch v.1

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

::: content/events/src/nsEventListenerManager.cpp
@@ +395,2 @@
>            return;
>          }

You flipped this conditional around.  How exactly do we ever get into DisableDevice now?

Comment 7

[Doug Turner (:dougt)]

when count == 0.  Maybe I am not understanding what you are asking.

Comment 8

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

Bug there is this even before
>+        if (deviceType) {
>           return;
>         }

Comment 9

[Doug Turner (:dougt)]

yeah, i don't recall what the early return was about.

Comment 10

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

The early return (if it was !deviceType) would be for performance.

Comment 11

[Doug Turner (:dougt)]

I think it was something like if we had two event listeners for a sensor and one event event listener was removed, we'd end up calling DisableDevice.  So, we have the early return to prevent that.  Reading the code, i think we can just remove that early return all together.

Comment 12

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

Then we'd iterate all the listeners always.

Comment 13

[Doug Turner (:dougt)]

don't we do that for every other event type?

Comment 14

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

Eh, what? No. At least we didn't used to.
  if (deviceType) {
    return;
  }
should be
  if (!deviceType) {
    return;
  }

right?

Comment 15

[Doug Turner (:dougt)]

Attachment: follow to fix logic

Comment 16

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

Comment on attachment 628511 [details] [diff] [review]
follow to fix logic

Is it possible the add some tests to check that we end up actually
disabling some devices?

Comment 17

[Doug Turner (:dougt)]

yes, great suggestion.

Comment 18

[Doug Turner (:dougt)]

see bug 759989.

Comment 19

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a1829c4f00a

Comment 20

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/mozilla-central/rev/8a1829c4f00a

Comment 21

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

Doug, do we want this to branches?

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 628511 [details] [diff] [review]
follow to fix logic

we need this or devices may not be shutdown after use.

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 628511 [details] [diff] [review]
follow to fix logic

Looks low risk and we're early in the cycle - let's get this on Aurora 15 as soon as possible.