Last Comment Bug 776934 - Camera - Hook up API (navigator.mozCameras) to WebAPI permissions manager
: Camera - Hook up API (navigator.mozCameras) to WebAPI permissions manager
Status: RESOLVED FIXED
[WebAPI:P0] [LOE:S]
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla18
Assigned To: Mike Habicher [:mikeh] (high bugzilla latency)
:
: Andrew Overholt [:overholt]
Mentors:
: 779142 (view as bug list)
Depends on:
Blocks: 774716
  Show dependency treegraph
 
Reported: 2012-07-24 08:12 PDT by Mike Habicher [:mikeh] (high bugzilla latency)
Modified: 2012-12-21 08:52 PST (History)
8 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
WIP - check for permission to access the camera API (3.71 KB, patch)
2012-09-17 11:17 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
justin.lebar+bug: feedback+
Details | Diff | Splinter Review
WIP - check for permission to access the camera API (3.71 KB, patch)
2012-09-17 11:18 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
no flags Details | Diff | Splinter Review
check for permission to access the camera API, refactor similar APIs (29.71 KB, patch)
2012-09-18 08:36 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
no flags Details | Diff | Splinter Review
check for permission to access the camera API, refactor similar APIs (11.88 KB, patch)
2012-09-18 09:15 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
no flags Details | Diff | Splinter Review
check for permission to access the camera API, refactor similar APIs (16.05 KB, patch)
2012-09-18 09:28 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
justin.lebar+bug: review-
Details | Diff | Splinter Review
check for permission to access the camera API, refactor similar APIs (11.87 KB, patch)
2012-09-18 12:30 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
no flags Details | Diff | Splinter Review
check for permission to access the camera API, refactor similar APIs (15.44 KB, patch)
2012-09-18 12:34 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
justin.lebar+bug: review+
Details | Diff | Splinter Review
check for permission to access the camera API, refactor similar APIs (16.71 KB, patch)
2012-09-18 16:37 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
no flags Details | Diff | Splinter Review
check for permission to access the camera API, refactor similar APIs (16.81 KB, patch)
2012-09-18 17:34 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
no flags Details | Diff | Splinter Review
check for permission to access the camera API, refactor similar APIs (20120920-1433) (12.22 KB, patch)
2012-09-20 11:34 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
no flags Details | Diff | Splinter Review
check for permission to access the camera API, refactor similar APIs (20120920-1647) (217.46 KB, patch)
2012-09-20 13:48 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
no flags Details | Diff | Splinter Review
check for permission to access the camera API, refactor similar APIs (20120920-1649) (12.31 KB, patch)
2012-09-20 13:49 PDT, Mike Habicher [:mikeh] (high bugzilla latency)
no flags Details | Diff | Splinter Review

Description Mike Habicher [:mikeh] (high bugzilla latency) 2012-07-24 08:12:21 PDT

    
Comment 1 Mike Habicher [:mikeh] (high bugzilla latency) 2012-07-24 08:17:21 PDT
CCed people as suggested by :ladamski.

Single point of permissions check is in dom/camera/DOMCameraManager.cpp::nsDOMCameraManager::Create().
Comment 2 Mike Habicher [:mikeh] (high bugzilla latency) 2012-07-31 08:32:08 PDT
*** Bug 779142 has been marked as a duplicate of this bug. ***
Comment 3 Lucas Adamski [:ladamski] 2012-09-14 04:00:52 PDT
How are we coming along here?
Comment 4 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-14 08:19:12 PDT
(In reply to Lucas Adamski from comment #3)
> How are we coming along here?

Planning on finishing up other code this week and getting this done next week.
Comment 5 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-17 11:17:40 PDT
Created attachment 661847 [details] [diff] [review]
WIP - check for permission to access the camera API

Add permission check to the camera API.  This takes place in nsDOMCameraManager::CheckPermissionAndCreateInstance().  If the check fails, no DOM-facing Camera Manager is created, and no subsequent camera APIs can be called.
Comment 6 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-17 11:18:07 PDT
Created attachment 661848 [details] [diff] [review]
WIP - check for permission to access the camera API

Add permission check to the camera API.  This takes place in nsDOMCameraManager::CheckPermissionAndCreateInstance().  If the check fails, no DOM-facing Camera Manager is created, and no subsequent camera APIs can be called.
Comment 7 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-17 11:18:50 PDT
Comment on attachment 661848 [details] [diff] [review]
WIP - check for permission to access the camera API

Remove duplicate attachment.
Comment 8 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-17 11:20:43 PDT
:jlebar, would you mind giving this a quick look-over to see if I'm on the right path?  I based this patch on the changes in the sub-bugs to 774716, but even when I remove "camera" and "camera-content" from 'permissions' in gaia/apps/camera/manifest.webapp and |./build.sh gaia && ./flash.sh gaia|, the camera always starts.
Comment 9 Justin Lebar (not reading bugmail) 2012-09-17 12:05:57 PDT
Comment on attachment 661847 [details] [diff] [review]
WIP - check for permission to access the camera API

This looks right to me, so if you're having problems, the bug may not be in here.

I would also be thrilled if this code, which is now copy-pasted n+1 times, were moved into nsIPermissionManager.
Comment 10 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-18 08:36:50 PDT
Created attachment 662178 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs
Comment 11 Justin Lebar (not reading bugmail) 2012-09-18 08:50:31 PDT
Comment on attachment 662178 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs

Also, what was going wrong with your earlier approach?

It looks like this diff has two copies of every file (run it through diffstat to see).
Comment 12 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-18 09:15:58 PDT
Created attachment 662190 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs

A patch without the duplicate files (didn't notice that I was >>'ing instead of >'ing--sorry).

The problem is https://bugzilla.mozilla.org/show_bug.cgi?id=785632 , which gives apps the permissions of NO_APP_ID while the plumbing is being finalized.  This seems to include permission to access the camera.  Right now, removing the hack (in extensions/cookie/nsPermissionManager.cpp) breaks B2G's start-up.
Comment 13 Justin Lebar (not reading bugmail) 2012-09-18 09:17:39 PDT
Okay...we also need -U8 in this diff.  I have in my ~/.hgrc:

[diff]
git = 1
showfunc = 1
unified = 8
Comment 14 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-18 09:28:30 PDT
Created attachment 662193 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs

with -U8
Comment 15 Justin Lebar (not reading bugmail) 2012-09-18 10:26:06 PDT
Comment on attachment 662193 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs

Thanks a lot for doing this.

>diff --git a/netwerk/base/public/nsIPermissionManager.idl b/netwerk/base/public/nsIPermissionManager.idl
> [scriptable, uuid(da33450a-f3cb-4fdb-93ee-219644e450c2)]

Please change the UUID. (You can use uuidgen, or ask firebot on irc for a uuid
with "firebot: uuid").

>@@ -141,16 +142,24 @@ interface nsIPermissionManager : nsISupports
>   /**
>    * Test whether the principal has the permission to perform a given action.
>    * System principals will always have permissions granted.
>    */
>   uint32_t testPermissionFromPrincipal(in nsIPrincipal principal,
>                                        in string type);
> 
>   /**
>+   * Test whether the document associated with this window has permission to
>+   * perform a given action.  System principals will always have permissions
>+   * granted.
>+   */
>+  uint32_t testPermissionFromWindow(in nsPIDOMWindow window,
>+                                    in string type);

nsPIDOMWindow isn't a scriptable interface; it's defined in a header file, not
an IDL file.  Therefore you shouldn't use nsPIDOMWindow here, because then we
couldn't call this method from JS.

nsIDOMWindow should work fine.  nsPIDOMWindow inherits from nsIDOMWindow, so
you shouldn't even need to modify callers.

Also, there's a lack of parallelism in the comment.  The first sentence says
we're testing whether the /document/ has permission.  But the second sentence
says that a certain /principal/ always has permission.

Either say "Test whether the principal associated with this window's document
has permission" or "Documents with the system principal will always have
permissions granted."

>diff --git a/dom/bluetooth/BluetoothManager.cpp b/dom/bluetooth/BluetoothManager.cpp
> nsresult
> NS_NewBluetoothManager(nsPIDOMWindow* aWindow,
>                        nsIDOMBluetoothManager** aBluetoothManager)
> {
>   NS_ASSERTION(aWindow, "Null pointer!");
> 
>-  nsPIDOMWindow* innerWindow = aWindow->IsInnerWindow() ?
>-    aWindow :
>-    aWindow->GetCurrentInnerWindow();
>-
>-  // Need the document for security check.
>-  nsCOMPtr<nsIDocument> document = innerWindow->GetExtantDoc();
>-  NS_ENSURE_TRUE(document, NS_NOINTERFACE);
>-
>-  nsCOMPtr<nsIPrincipal> principal = document->NodePrincipal();
>-  NS_ENSURE_TRUE(principal, NS_ERROR_UNEXPECTED);
>-
>   nsCOMPtr<nsIPermissionManager> permMgr =
>     do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
>   NS_ENSURE_TRUE(permMgr, NS_ERROR_UNEXPECTED);
> 
>   uint32_t permission;
>   nsresult rv =
>-    permMgr->TestPermissionFromPrincipal(principal, "mozBluetooth",
>+    permMgr->TestPermissionFromWindow(aWindow, "mozBluetooth",
>                                          &permission);

Please fix the indentation of &permission.

>diff --git a/dom/camera/DOMCameraControl.h b/dom/camera/DOMCameraControl.h
>--- a/dom/camera/DOMCameraManager.h
>+++ b/dom/camera/DOMCameraManager.h
>@@ -9,23 +9,25 @@
> 
> #include "nsCOMPtr.h"
> #include "nsAutoPtr.h"
> #include "nsIThread.h"
> #include "nsThreadUtils.h"
> #include "nsIDOMCameraManager.h"
> #include "mozilla/Attributes.h"
> 
>+class nsPIDOMWindow;
>+
> class nsDOMCameraManager MOZ_FINAL : public nsIDOMCameraManager
> {
> public:
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSIDOMCAMERAMANAGER
> 
>-  static already_AddRefed<nsDOMCameraManager> Create(uint64_t aWindowId);
>+  static already_AddRefed<nsDOMCameraManager> CheckPermissionAndCreateInstance(nsPIDOMWindow* aWindow);

This should be nsIDOMWindow* again.  Also, please break the line after the
close template.  (I see that some other similar factory functions take
nsPIDOMWindow, but it's only because they're un-enlightened.)

>diff --git a/dom/camera/DOMCameraManager.cpp b/dom/camera/DOMCameraManager.cpp
>index 49576a4..60bcd91 100644
>--- a/dom/camera/DOMCameraManager.cpp
>+++ b/dom/camera/DOMCameraManager.cpp
>@@ -1,12 +1,14 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>+#include "nsIDocument.h"
>+#include "nsIPermissionManager.h"
> #include "DOMCameraControl.h"
> #include "DOMCameraManager.h"
> #include "nsDOMClassInfo.h"
> #include "DictionaryHelpers.h"
> #include "CameraCommon.h"
> 
> using namespace mozilla;
> 
>@@ -53,26 +55,43 @@ nsDOMCameraManager::~nsDOMCameraManager()
> void
> nsDOMCameraManager::OnNavigation(uint64_t aWindowId)
> {
>   // TODO: see bug 779145.
> }
> 
> // static creator
> already_AddRefed<nsDOMCameraManager>
>-nsDOMCameraManager::Create(uint64_t aWindowId)
>+nsDOMCameraManager::CheckPermissionAndCreateInstance(nsPIDOMWindow* aWindow)
> {
>-  // TODO: see bug 776934.
>-
> #ifdef PR_LOGGING
>+  /**
>+   * VERY IMPORTANT:
>+   * Setting 'gCameraLog' *MUST* happen before calling any DOM_CAMERA_LOGx()
>+   * macros (except _LOGR()--see CameraCommon.h), else you WILL cause
>+   * yourself lots of SIGSEGV headachery.
>+   */
>   if (!gCameraLog) {
>     gCameraLog = PR_LOG_DEFINE("Camera");
>   }

This is why everyone else who uses PR_LOG_DEFINE (which, to be fair, is only
two other people) uses a static initializer.  :)  For example, in hal/Hal.cpp:

  PRLogModuleInfo *sHalLog = PR_LOG_DEFINE("hal");

You don't even have to guard PR_LOG_DEFINE with an ifdef PR_LOGGING; that ifdef
is already part of PR_LOG_DEFINE.  (See nsprpub/pr/include/prlog.h.)

>-  nsRefPtr<nsDOMCameraManager> cameraManager = new nsDOMCameraManager(aWindowId);
>+
>+  NS_ASSERTION(aWindow, "Null nsPIDOMWindow pointer!");

This assertion is not adding anything.  We're going to crash with a
null-pointer exception on aWindow at the second-to-last line here, if not
sooner.

>+  nsCOMPtr<nsIPermissionManager> permMgr = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);

Please wrap this line after the =.

>+  NS_ENSURE_TRUE(permMgr, nullptr);
>+
>+  uint32_t permission = nsIPermissionManager::DENY_ACTION;
>+  permMgr->TestPermissionFromWindow(aWindow, "camera", &permission);
>+  if (permission != nsIPermissionManager::ALLOW_ACTION) {
>+    printf_stderr("%s:%d : no permission to access camera\n", __func__, __LINE__);

This should either go through your gCameraLog or be an NS_WARNING.  We do not
use printf_stderr for warnings like this, because there's no way to turn it
off.

>+    return nullptr;
>+  }
>+
>+  nsRefPtr<nsDOMCameraManager> cameraManager = new nsDOMCameraManager(aWindow->WindowID());

Wrap the line after =, please.

>diff --git a/dom/camera/ICameraControl.h b/dom/camera/ICameraControl.h
>index 12e6d3d..6fb565e 100644
>--- a/dom/camera/ICameraControl.h
>+++ b/dom/camera/ICameraControl.h
>@@ -1,17 +1,15 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #ifndef DOM_CAMERA_ICAMERACONTROL_H
> #define DOM_CAMERA_ICAMERACONTROL_H
> 
>-#include "base/basictypes.h"
>-#include "prtypes.h"
> #include "jsapi.h"
> #include "nsIDOMCameraManager.h"
> #include "DictionaryHelpers.h"
> #include "CameraCommon.h"
> 
> namespace mozilla {
> 
> using namespace dom;

>diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp
>index 3258361..b66af07 100644
>--- a/extensions/cookie/nsPermissionManager.cpp
>+++ b/extensions/cookie/nsPermissionManager.cpp
>@@ -903,16 +903,33 @@ nsPermissionManager::TestPermission(nsIURI     *aURI,
>   nsCOMPtr<nsIPrincipal> principal;
>   nsresult rv = GetPrincipal(aURI, getter_AddRefs(principal));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return TestPermissionFromPrincipal(principal, aType, aPermission);
> }
> 
> NS_IMETHODIMP
>+nsPermissionManager::TestPermissionFromWindow(nsPIDOMWindow* aWindow,
>+                                              const char* aType,
>+                                              uint32_t* aPermission)
>+{
>+  nsPIDOMWindow* innerWindow = aWindow->IsInnerWindow() ?
>+    aWindow :
>+    aWindow->GetCurrentInnerWindow();
>+
>+  // Get the document for security check
>+  nsCOMPtr<nsIDocument> document = innerWindow->GetExtantDoc();
>+  NS_ENSURE_TRUE(document, NS_NOINTERFACE);
>+
>+  nsCOMPtr<nsIPrincipal> principal = document->NodePrincipal();
>+  return TestPermissionFromPrincipal(principal, "camera", aPermission);
>+}

>+NS_IMETHODIMP
> nsPermissionManager::TestPermissionFromPrincipal(nsIPrincipal* aPrincipal,
>                                                  const char* aType,
>                                                  uint32_t* aPermission)

We really should make these two methods always return NS_OK and set aPermission
to PERMISSION_DENIED by default.  That eliminates an entire class of bugs,
where someone does

  uint32_t permission;
  TestPermissionFromPrincipal(&permission);
  if (permission != PERMISSION_DENIED) {
    // Whoa nelly; |permission| may be uninitialized!
  }

You don't have to do that here if you don't want to, though.  :)

I'd like to have another look after these fixups.
Comment 16 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-18 11:33:54 PDT
(In reply to Justin Lebar [:jlebar] from comment #15)
> Comment on attachment 662193 [details] [diff] [review]
> check for permission to access the camera API, refactor similar APIs
> 
> Thanks a lot for doing this.
> 
> >   /**
> >+   * Test whether the document associated with this window has permission to
> >+   * perform a given action.  System principals will always have permissions
> >+   * granted.
> >+   */
> >+  uint32_t testPermissionFromWindow(in nsPIDOMWindow window,
> >+                                    in string type);
>
> nsPIDOMWindow isn't a scriptable interface; it's defined in a header file, not
> an IDL file.  Therefore you shouldn't use nsPIDOMWindow here, because then we
> couldn't call this method from JS.
> 
> nsIDOMWindow should work fine.  nsPIDOMWindow inherits from nsIDOMWindow, so
> you shouldn't even need to modify callers.

It looks like nsPIDOMWindow doesn't support IsInnerWindow() or GetCurrentInnerWindow(), which are required in TestPermissionFromWindow() to get 'innerWindow'.  I suppose I could remove testPermissionFromWindow() from nsIPermissionManager.idl and add it to nsPIDOMWindow instead.

> > #ifdef PR_LOGGING
> >+  /**
> >+   * VERY IMPORTANT:
> >+   * Setting 'gCameraLog' *MUST* happen before calling any DOM_CAMERA_LOGx()
> >+   * macros (except _LOGR()--see CameraCommon.h), else you WILL cause
> >+   * yourself lots of SIGSEGV headachery.
> >+   */
> >   if (!gCameraLog) {
> >     gCameraLog = PR_LOG_DEFINE("Camera");
> >   }
> 
> >-  nsRefPtr<nsDOMCameraManager> cameraManager = new nsDOMCameraManager(aWindowId);
> >+
> >+  NS_ASSERTION(aWindow, "Null nsPIDOMWindow pointer!");
> 
> This assertion is not adding anything.  We're going to crash with a
> null-pointer exception on aWindow at the second-to-last line here, if not
> sooner.

This trope appears in lots of other places--should I get rid of them as well?

> >diff --git a/dom/camera/ICameraControl.h b/dom/camera/ICameraControl.h
> >index 12e6d3d..6fb565e 100644
> >--- a/dom/camera/ICameraControl.h
> >+++ b/dom/camera/ICameraControl.h
> >@@ -1,17 +1,15 @@
> > /* This Source Code Form is subject to the terms of the Mozilla Public
> >  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > 
> > #ifndef DOM_CAMERA_ICAMERACONTROL_H
> > #define DOM_CAMERA_ICAMERACONTROL_H
> > 
> >-#include "base/basictypes.h"
> >-#include "prtypes.h"
> > #include "jsapi.h"
> > #include "nsIDOMCameraManager.h"
> > #include "DictionaryHelpers.h"
> > #include "CameraCommon.h"
> > 
> > namespace mozilla {
> > 
> > using namespace dom;

Just checking: was there supposed to be a comment here?

> >diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp
> >index 3258361..b66af07 100644
> >--- a/extensions/cookie/nsPermissionManager.cpp
> >+++ b/extensions/cookie/nsPermissionManager.cpp
> >@@ -903,16 +903,33 @@ nsPermissionManager::TestPermission(nsIURI     *aURI,
> >   nsCOMPtr<nsIPrincipal> principal;
> >   nsresult rv = GetPrincipal(aURI, getter_AddRefs(principal));
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >   return TestPermissionFromPrincipal(principal, aType, aPermission);
> > }
> > 
> > NS_IMETHODIMP
> >+nsPermissionManager::TestPermissionFromWindow(nsPIDOMWindow* aWindow,
> >+                                              const char* aType,
> >+                                              uint32_t* aPermission)
> >+{
> >+  nsPIDOMWindow* innerWindow = aWindow->IsInnerWindow() ?
> >+    aWindow :
> >+    aWindow->GetCurrentInnerWindow();
> >+
> >+  // Get the document for security check
> >+  nsCOMPtr<nsIDocument> document = innerWindow->GetExtantDoc();
> >+  NS_ENSURE_TRUE(document, NS_NOINTERFACE);
> >+
> >+  nsCOMPtr<nsIPrincipal> principal = document->NodePrincipal();
> >+  return TestPermissionFromPrincipal(principal, "camera", aPermission);
> >+}
> 
> >+NS_IMETHODIMP
> > nsPermissionManager::TestPermissionFromPrincipal(nsIPrincipal* aPrincipal,
> >                                                  const char* aType,
> >                                                  uint32_t* aPermission)
> 
> We really should make these two methods always return NS_OK and set
> aPermission to PERMISSION_DENIED by default.  That eliminates an entire
> class of bugs, where someone does
> 
>   uint32_t permission;
>   TestPermissionFromPrincipal(&permission);
>   if (permission != PERMISSION_DENIED) {
>     // Whoa nelly; |permission| may be uninitialized!
>   }
> 
> You don't have to do that here if you don't want to, though.  :)

Just grepped through the tree, and there seem to be two usage patterns:

(1)
    uint32_t permission;
    nsresult rv = TestPermissionFromPrincipal(..., &permission);
    NS_ENSURE_SUCCESS(rv, rv);
    if (permission != ALLOW_ACTION) {
        return;
    }

(2)
    uint32_t permission = DENY_ACTION;
    TestPermissionFromPrincipal(..., &permission);
    if (permission != ALLOW_ACTION) {
        return;
    }

Places where the permission isn't preloaded AND the return value isn't checked:
- dom/src/storage/nsDOMStorage.cpp:1423
- extensions/cookie/nsCookiePermission.cpp:209

File a new bug for these?  :)

It looks like dom/media/MediaManager.cpp:386 and content/html/content/src/nsHTMLInputElement.cpp:347 both call nsPopupWindowManager::TestPermission(), which preloads the *aPermission from mPolicy.  Do you think we would need/want something like that?
Comment 17 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-18 11:37:00 PDT
(In reply to Mike Habicher [:mikeh] from comment #16)
> 
> It looks like nsPIDOMWindow doesn't support IsInnerWindow() or
> GetCurrentInnerWindow(), which are required in TestPermissionFromWindow() to
> get 'innerWindow'.  I suppose I could remove testPermissionFromWindow() from
> nsIPermissionManager.idl and add it to nsPIDOMWindow instead.

I just re-read this reply and it makes no sense.  :)  I meant, I could add it to nsPermissionManager directly, instead of defining it as part of nsIPermissionManager.
Comment 18 Justin Lebar (not reading bugmail) 2012-09-18 11:41:20 PDT
> It looks like nsPIDOMWindow doesn't support IsInnerWindow() or GetCurrentInnerWindow(), which are 
> required in TestPermissionFromWindow() to get 'innerWindow'.

Correct.  You can QI to nsPIDOMWindow first thing in your implementation.

>> This assertion is not adding anything.  We're going to crash with a
>> null-pointer exception on aWindow at the second-to-last line here, if not
>> sooner.
>
> This trope appears in lots of other places--should I get rid of them as well?

Sure if you like, but the more fixes we add to this one patch, the harder it becomes to review.  So maybe you can do that in a separate patch.

> Just checking: was there supposed to be a comment here?

No, you're OK.  :)

> File a new bug for these?  :)

Sure, or you can fix them here but in a separate patch (this bug has already morphed into "clean up calls to TestPermissionFrom*").  Whatever you prefer.

> Do you think we would need/want something like that?

I can't imagine having a policy of anything other than failing closed, so I don't think so.
Comment 19 Justin Lebar (not reading bugmail) 2012-09-18 11:42:56 PDT
> I meant, I could add it to nsPermissionManager directly, instead of defining it as part of 
> nsIPermissionManager.

You could, but then every caller would have to static_cast their nsIPermissionManager* to nsPermissionManager*, which is an uncommon idiom in Gecko.  Better would be to make the method [noscript] in XPCOM, if we really wanted to use nsPIDOMWindow for some reason.
Comment 20 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-18 12:30:13 PDT
Created attachment 662267 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs

Incorporate feedback from reviews:
- refactor TestPermissionFromPrincipal() calls into TestPermissionFromWindow()
- do_QueryInterface() from nsIDOMWindow to nsPIDOMWindow
- new UUID on nsIPermissionManager
- tidy up the PRLogging
- clean ups
Comment 21 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-18 12:34:04 PDT
Created attachment 662269 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs

Now with extra -U8.
Comment 22 Justin Lebar (not reading bugmail) 2012-09-18 13:10:40 PDT
> class nsDOMCameraManager MOZ_FINAL : public nsIDOMCameraManager
> {
> public:
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSIDOMCAMERAMANAGER
> 
>-  static already_AddRefed<nsDOMCameraManager> CheckPermissionAndCreateInstance(nsPIDOMWindow* aWindow);
>+  static already_AddRefed<nsDOMCameraManager>
>+    CheckPermissionAndCreateInstance(nsPIDOMWindow* aWindow);

Per the earlier review, this should be nsIDOMWindow*.

>diff --git a/dom/camera/DOMCameraManager.cpp b/dom/camera/DOMCameraManager.cpp
>+#include "nsDebug.h"

I'm surprised this isn't included by one of the other files, but ok!

> NS_IMPL_RELEASE(nsDOMCameraManager)
> 
> /**
>  * Global camera logging object
>  *
>  * Set the NSPR_LOG_MODULES environment variable to enable logging
>  * in a debug build, e.g. NSPR_LOG_MODULES=Camera:5
>  */
>-#ifdef PR_LOGGING
>-PRLogModuleInfo* gCameraLog;
>-#endif
>+PRLogModuleInfo* gCameraLog = PR_LOG_DEFINE("Camera");;

;;

>@@ -57,60 +56,29 @@ nsDOMCameraManager::OnNavigation(uint64_t aWindowId)
> {
>   // TODO: see bug 779145.
> }
> 
> // static creator
> already_AddRefed<nsDOMCameraManager>
> nsDOMCameraManager::CheckPermissionAndCreateInstance(nsPIDOMWindow* aWindow)
>+  permMgr->TestPermissionFromWindow(aWindow, "camera", &permission);
>   if (permission != nsIPermissionManager::ALLOW_ACTION) {
>-    printf_stderr("%s:%d : no permission to access camera\n", __func__, __LINE__);
>+    NS_WARNING("No permission to access camera\n");

No \n.
Comment 23 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-18 16:35:26 PDT
(In reply to Justin Lebar [:jlebar] from comment #22)
> > class nsDOMCameraManager MOZ_FINAL : public nsIDOMCameraManager
> > {
> > public:
> >   NS_DECL_ISUPPORTS
> >   NS_DECL_NSIDOMCAMERAMANAGER
> > 
> >-  static already_AddRefed<nsDOMCameraManager> CheckPermissionAndCreateInstance(nsPIDOMWindow* aWindow);
> >+  static already_AddRefed<nsDOMCameraManager>
> >+    CheckPermissionAndCreateInstance(nsPIDOMWindow* aWindow);
> 
> Per the earlier review, this should be nsIDOMWindow*.

All of the other CheckPermissionAndCreateInstance() functions take an nsPIDOMWindow* as well, not an nsIDOMWindow*.  In my particular case, I also need the WindowID() method which only exists on nsPIDOMWindow.  Since (unlike TestPermissionFromWindow()) this isn't a scriptable function either, so I'd like to leave this as-is.

> > NS_IMPL_RELEASE(nsDOMCameraManager)
> > 
> > /**
> >  * Global camera logging object
> >  *
> >  * Set the NSPR_LOG_MODULES environment variable to enable logging
> >  * in a debug build, e.g. NSPR_LOG_MODULES=Camera:5
> >  */
> >-#ifdef PR_LOGGING
> >-PRLogModuleInfo* gCameraLog;
> >-#endif
> >+PRLogModuleInfo* gCameraLog = PR_LOG_DEFINE("Camera");;
> 
> ;;

*sigh* thanks. :)
Comment 24 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-18 16:37:35 PDT
Created attachment 662355 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs

Incorporate last feedback.
Comment 25 Justin Lebar (not reading bugmail) 2012-09-18 17:03:31 PDT
> All of the other CheckPermissionAndCreateInstance() functions take an nsPIDOMWindow* as 
> well, not an nsIDOMWindow*.

Indeed, but that doesn't mean they know what they're doing.  :)

I'm happy if you want to leave this as-is, but let's not make a habit of adding public APIs (scriptable or otherwise) which take PIDOMWindow; much of the time, it just means that the caller has to do a QI, which is unnecessarily annoying.

(If you know your caller will have a PIDOMWindow and you want to /save/ a QI because perf matters, then it might make sense to accept a PIDOMWindow.  But that's not the case here.)
Comment 26 Justin Lebar (not reading bugmail) 2012-09-18 17:05:32 PDT
If you're ready for this to be checked in, please hg export a patch with a proper commit message and mark the bug as checkin-needed.

It looks like you may be familiar with this, but in case you're not, more info is at http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Comment 27 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-18 17:34:55 PDT
Created attachment 662368 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs

Now with 100% more headers.
Comment 28 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-18 17:37:43 PDT
(In reply to Justin Lebar [:jlebar] from comment #25)
> > All of the other CheckPermissionAndCreateInstance() functions take an nsPIDOMWindow* as 
> > well, not an nsIDOMWindow*.
> 
> Indeed, but that doesn't mean they know what they're doing.  :)

Heh.  :)

If this is something that you think should be cleaned up across the board, I'll keep it in the back of my mind.  Thanks again for the review!
Comment 29 Justin Lebar (not reading bugmail) 2012-09-18 17:41:20 PDT
(In reply to Mike Habicher [:mikeh] from comment #27)
> Now with 100% more headers.

Needs an r=jlebar
Comment 30 Ryan VanderMeulen [:RyanVM] 2012-09-18 18:46:30 PDT
Pushed to Try since I don't see any results here. Will checkin if it goes green.
https://tbpl.mozilla.org/?tree=Try&rev=f9cf6f65e19f

(In reply to Justin Lebar [:jlebar] from comment #29)
> Needs an r=jlebar

Pfft, the checkin monkeys have that part covered :P
Comment 31 Justin Lebar (not reading bugmail) 2012-09-18 18:51:27 PDT
> Pfft, the checkin monkeys have that part covered :P

In all seriousness, do we not ask people to put r=foo in their commit messages for checkin-needed bugs anymore?  I don't want to be the only one asking for it!
Comment 32 Ryan VanderMeulen [:RyanVM] 2012-09-18 18:57:08 PDT
It's preferred, but I also see little point in posting a new patch if all that's being added is the r=. I check every commit message before pushing, so it'll end up on there one way or another.
Comment 33 Ryan VanderMeulen [:RyanVM] 2012-09-18 19:44:22 PDT
This is failing tests.

https://tbpl.mozilla.org/php/getParsedLog.php?id=15326726&tree=Try

951 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/power/test/test_power_basics.html | Shouldn be able to access power manager with permission.
Comment 34 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-20 11:34:58 PDT
Created attachment 663084 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs (20120920-1433)

Fixes a nasty bug that was causing everything to test again camera permissions.

New try results: https://tbpl.mozilla.org/?tree=Try&rev=f4e72c50967e
Comment 35 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-20 11:36:29 PDT
The fix diff: https://bugzilla.mozilla.org/attachment.cgi?oldid=662368&action=interdiff&newid=663084&headers=1
Comment 36 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-20 13:48:07 PDT
Created attachment 663147 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs (20120920-1647)

(Now fortified with extra headers.)
Comment 37 Mike Habicher [:mikeh] (high bugzilla latency) 2012-09-20 13:49:58 PDT
Created attachment 663148 [details] [diff] [review]
check for permission to access the camera API, refactor similar APIs (20120920-1649)

...and actually the right attachment this time.  *sigh*
Comment 38 Ryan VanderMeulen [:RyanVM] 2012-09-20 17:11:21 PDT
The last patch attached appears to have some differences from what was pushed to Try. Pushed again to be safe. I'll land it if it goes green.
https://tbpl.mozilla.org/?tree=Try&rev=b6377046bd34
Comment 39 Ryan VanderMeulen [:RyanVM] 2012-09-21 03:35:12 PDT
(In reply to Ryan VanderMeulen from comment #38)
> https://tbpl.mozilla.org/?tree=Try&rev=b6377046bd34

Green on Try (the Win7 m-oth failure was not from this).
https://hg.mozilla.org/integration/mozilla-inbound/rev/da5b2bdfd115
Comment 40 Ryan VanderMeulen [:RyanVM] 2012-09-21 19:56:48 PDT
https://hg.mozilla.org/mozilla-central/rev/da5b2bdfd115

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