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

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

unspecified
mozilla14
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 612198 [details] [diff] [review]
patch v.1
Assignee: nobody → doug.turner
Attachment #612198 - Flags: review?(bugs)

Comment 2

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

># HG changeset patch
># Parent 087971d348c7b8e002f9695ccbc4871dc2044537
># User Doug Turner <dougt@dougt.org>
>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
Attachment #612198 - Flags: review?(bugs) → review+
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2ba2d3ae51f
(Assignee)

Updated

5 years ago
Duplicate of this bug: 742605
https://hg.mozilla.org/mozilla-central/rev/b2ba2d3ae51f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
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?
(Assignee)

Comment 7

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

Comment 8

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

Comment 9

5 years ago
yeah, i don't recall what the early return was about.
The early return (if it was !deviceType) would be for performance.
(Assignee)

Comment 11

5 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Then we'd iterate all the listeners always.
(Assignee)

Comment 13

5 years ago
don't we do that for every other event type?
Eh, what? No. At least we didn't used to.
  if (deviceType) {
    return;
  }
should be
  if (!deviceType) {
    return;
  }

right?
(Assignee)

Comment 15

5 years ago
Created attachment 628511 [details] [diff] [review]
follow to fix logic
(Assignee)

Updated

5 years ago
Attachment #628511 - Flags: review?(bugs)
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?
Attachment #628511 - Flags: review?(bugs) → review+
(Assignee)

Comment 17

5 years ago
yes, great suggestion.
(Assignee)

Comment 18

5 years ago
see bug 759989.
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a1829c4f00a
https://hg.mozilla.org/mozilla-central/rev/8a1829c4f00a
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Doug, do we want this to branches?
(Assignee)

Comment 22

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

we need this or devices may not be shutdown after use.
Attachment #628511 - Flags: approval-mozilla-aurora?
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.
Attachment #628511 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 782549
You need to log in before you can comment on or make changes to this bug.