Last Comment Bug 759989 - Add test to ensure device sensors are shutdown when listeners are removed
: Add test to ensure device sensors are shutdown when listeners are removed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Doug Turner (:dougt)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 782549
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-30 20:51 PDT by Doug Turner (:dougt)
Modified: 2012-08-14 10:31 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v.1 (5.97 KB, patch)
2012-05-30 20:51 PDT, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
patch v.2 (5.98 KB, patch)
2012-06-04 09:29 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
patch v.3 (5.81 KB, patch)
2012-06-04 09:50 PDT, Doug Turner (:dougt)
bugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2012-05-30 20:51:39 PDT
Created attachment 628592 [details] [diff] [review]
patch v.1

follow up from 742376 
 
* Adds test to verify that device sensors are being shutdown.
* Adds a new method to nsIDeviceSensors that exposes if a window has a listener.
* Fixes bug in nsGlobalWindow::DisableDeviceSensor where we call through to RemoveWindowListener while there are still valid listeners
Comment 1 Olli Pettay [:smaug] 2012-05-31 01:48:57 PDT
Comment on attachment 628592 [details] [diff] [review]
patch v.1


>+++ b/content/events/test/test_bug742376.html
>@@ -0,0 +1,59 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=402089
>+-->
>+<head>
>+  <title>Test for Bug 742376</title>
>+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>+</head>
>+
>+<body>
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=742376">Mozilla Bug 742376</a>
>+<script class="testbody" type="text/javascript">
>+
>+/** Test for Bug 742376 **/
>+
>+function getListenerCount() {
>+
>+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+  var dss = Components.classes["@mozilla.org/devicesensors;1"]
>+                      .getService(Components.interfaces.nsIDeviceSensors);
>+
>+  return dss.getWindowListenerCount(Components.interfaces.nsIDeviceSensorData.TYPE_ORIENTATION,
>+                                    window);

Either make this chrome test or expose deviceSensors in SpecialPowers. We really don't want
new enablePrivilege() calls.
> 
>+NS_IMETHODIMP nsDeviceSensors::GetWindowListenerCount(PRUint32 aType, nsIDOMWindow *aWindow, PRInt32 *_retval NS_OUTPARAM)

drop NS_OUTPARAM and _retval -> aRetVal



>+{
>+   *_retval = 0;
>+
>+  if (!mEnabled)
>+    return NS_OK;
>+
>+  if (mWindowListeners[aType]->IndexOf(aWindow) == NoIndex)
>+    return NS_OK;
Looks like the file doesn't use Mozilla coding style, so perhaps no need
to fix these to use
if (expr) {
  stmt;
}

>+[scriptable, uuid(83306c9f-1c8f-43c4-900a-245d7f219511)]
> interface nsIDeviceSensors : nsISupports
> {
>+  long getWindowListenerCount(in unsigned long aType, in nsIDOMWindow aWindow);
This is strange method. It takes aWindow just to check that window has registered aType.
Wouldn't it be better to have two separate methods.
bool hasRegisteredType(in nsIDOMWindow aWindow);
and
long listenerCount(in unsigned long aType);
Comment 2 Doug Turner (:dougt) 2012-05-31 16:28:36 PDT
> We really don't want new enablePrivilege() calls.

There are 55 other calls in that directory.  I do not see the harm in adding more.  When enablePrivilege is actually removed, someone is going to have to fix up all of these in the manner that you prescribed.

if you insist, where exactly should this test live?

> Wouldn't it be better to have two separate methods.

every window can have n listeners for every type. 

> Looks like the file doesn't use Mozilla coding style, so perhaps no need
to fix these to use

I'll fix it, but I should really just send you a patch which is just ws changes.
Comment 3 Doug Turner (:dougt) 2012-06-04 09:29:33 PDT
Created attachment 629811 [details] [diff] [review]
patch v.2

converting test to use SpecialPowers.
Comment 4 Doug Turner (:dougt) 2012-06-04 09:50:23 PDT
Created attachment 629826 [details] [diff] [review]
patch v.3

dropping GetWindowListenerCount() in favor of listenerCount.  we do not need hasRegisteredType at this point.
Comment 6 Doug Turner (:dougt) 2012-06-04 12:23:46 PDT
backed out. :(  https://hg.mozilla.org/integration/mozilla-inbound/rev/e27433b51442

some other listeners is in mochitest-1 that is hurting this.
Comment 7 Doug Turner (:dougt) 2012-06-05 14:08:21 PDT
Comment on attachment 629826 [details] [diff] [review]
patch v.3

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

Does change a uuid of an interface this is used internally.
Comment 8 Doug Turner (:dougt) 2012-06-06 11:53:06 PDT
change the test so that we record the number of listeners at the beginning of the test run, and ensure at the end of the test run we equal that number.
Comment 10 Graeme McCutcheon [:graememcc] 2012-06-08 04:20:49 PDT
https://hg.mozilla.org/mozilla-central/rev/a9024396eeb7

(Merged by Ed Morley)
Comment 11 Doug Turner (:dougt) 2012-06-12 09:56:31 PDT
Comment on attachment 629826 [details] [diff] [review]
patch v.3

similar to bug 742376 and 759354.  Without this, we will not shutdown listeners.
Comment 12 Alex Keybl [:akeybl] 2012-06-15 16:03:53 PDT
Comment on attachment 629826 [details] [diff] [review]
patch v.3

[Triage Comment]
The IDL change is only in nsIDeviceSensors.idl, which as I understand it is unused externally. Approved for Aurora 15.

Note You need to log in before you can comment on or make changes to this bug.