Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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.