781355 - Hook up mozBrowser to Permission Manager

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

We should remove dom.mozBrowserFramesWhitelist.

Comment 1

[Justin Lebar (not reading bugmail)]

I don't see why we would block shipping the phone if this bug weren't fixed.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Mozbrowser won't work for 3rd party apps at all without this.

Am I understanding it correct that if it wasn't for this bug mozbrowser would work for 3rd party apps, although we wouldn't have the process separation?

Comment 3

[Justin Lebar (not reading bugmail)]

> Am I understanding it correct that if it wasn't for this bug mozbrowser would work for 3rd party 
> apps, although we wouldn't have the process separation?

I think so.

And to be clear, "we wouldn't have the process separation" means that the browser content would run in the same process as the app.

Comment 4

[Gregor Wagner [:gwagner]]

Attachment: patch

WiP

Comment 5

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 6

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 7

[Gregor Wagner [:gwagner]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=3713094db832

Comment 8

[Gregor Wagner [:gwagner]]

Attachment: patch

I changed the permission name from "mozBrowser" to "browser".
We need to merge https://github.com/gregorwagner/gaia/compare/mozbrowser before we can land this.

Comment 9

[Justin Lebar (not reading bugmail)]

Thanks for doing this.

>diff -r f89feda9d997 content/html/content/src/nsGenericHTMLFrameElement.cpp
>--- a/content/html/content/src/nsGenericHTMLFrameElement.cpp	Sun Aug 12 21:28:26 2012 +0300
>+++ b/content/html/content/src/nsGenericHTMLFrameElement.cpp	Mon Aug 13 14:36:27 2012 -0700
>@@ -285,25 +286,30 @@ nsGenericHTMLFrameElement::GetReallyIsBr
>   // Fail if this frame doesn't have the mozbrowser attribute.
>   bool isBrowser = false;
>   GetMozbrowser(&isBrowser);
>   if (!isBrowser) {
>     return NS_OK;
>   }
> 
>   // Fail if the node principal isn't trusted.
>-  // TODO: check properly for mozApps rights when mozApps will be less hacky.
>   nsIPrincipal *principal = NodePrincipal();
>-  nsCOMPtr<nsIURI> principalURI;
>-  principal->GetURI(getter_AddRefs(principalURI));
>-  if (!nsContentUtils::IsSystemPrincipal(principal) &&
>-      !nsContentUtils::URIIsChromeOrInPref(principalURI,
>-                                           "dom.mozBrowserFramesWhitelist")) {
>+  nsCOMPtr<nsIPermissionManager> permMgr =
>+    do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
>+  if (!permMgr)
>     return NS_OK;
>-  }

Nit: We brace all our if statements in this code.  At least, in theory.  :)

But this can be an NS_ENSURE_STATE(); it's pretty bad if we don't have the
permission manager.

>+  PRUint32 permission;
>+  nsresult rv =
>+    permMgr->TestPermissionFromPrincipal(principal, "browser", &permission);
>+  NS_ENSURE_SUCCESS(rv, NS_OK);
>+
>+  if (permission != nsIPermissionManager::ALLOW_ACTION) {
>+    return NS_OK;
>+  } 
>   // Otherwise, succeed.
>   *aOut = true;
>   return NS_OK;
> }

Nit: It would be more concise if we did

  PRUint32 permission = nsIPermissionManager::DISALLOW_ACTION;
  permMgr->GetPermission(...);
  *aOut = permission == nsIPermissionManager::ALLOW_ACTION;

>diff -r f89feda9d997 dom/browser-element/mochitest/browserElementTestHelpers.js
>@@ -76,32 +68,40 @@ const browserElementTestHelpers = {
>   getPageThumbsEnabledPref: function() {
>     return this._getBoolPref('browser.pageThumbs.enabled');
>   },
> 
>   setPageThumbsEnabledPref: function(value) {
>     this._setBoolPref('browser.pageThumbs.enabled', value);
>   },
> 
>-  addToWhitelist: function() {
>-    var whitelist = this.getWhitelistPref();
>-    whitelist += ',  http://' + window.location.host + ',  ';
>-    this.setWhitelistPref(whitelist);
>+  addPermission: function() {
>+    SpecialPowers.addPermission("browser", true, document);
>+  },
>+
>+  removePermission: function() {
>+    SpecialPowers.addPermission("browser", document);
>+  },
>+
>+  addPermissionForUrl: function(url) {
>+    SpecialPowers.addPermission("browser", true, url);
>+  },
>+
>+  removePermissionForUrl: function(url) {
>+    SpecialPowers.removePermission("browser", url);
>   },

It looks like you never call removePermission{,ForURL}.  I think we should
restore the permission to its original state when the test finishes, because
otherwise we can get weird inter-test interactions.

Either doing the restoration in this helper or by adding something to
SpecialPowers akin to pushPrefEnv is fine by me.

I'm sorry to send this back for another review over a silly point like this,
but I imagine the browser tests aren't the only ones which need to figure out
how to safely set and unset permissions...

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 651602 [details] [diff] [review]
patch

r- just for not unsetting permissions when the tests complete.

Comment 11

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 12

[Gregor Wagner [:gwagner]]

I was thinking of removing the permissions but I was lazy :) It should be fixed now.
Another try run:
https://tbpl.mozilla.org/?tree=Try&rev=f471473e7ced

Comment 13

[Justin Lebar (not reading bugmail)]

This review is on the interdiff.

>diff -u b/content/html/content/src/nsGenericHTMLFrameElement.cpp b/content/html/content/src/nsGenericHTMLFrameElement.cpp
>@@ -294,20 +294,12 @@
>   nsIPrincipal *principal = NodePrincipal();
>   nsCOMPtr<nsIPermissionManager> permMgr =
>     do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
>-  if (!permMgr)
>-    return NS_OK;
>+  NS_ENSURE_STATE(permMgr);
> 
>-  PRUint32 permission;
>-  nsresult rv =
>-    permMgr->TestPermissionFromPrincipal(principal, "browser", &permission);
>+  PRUint32 permission = nsIPermissionManager::DENY_ACTION;
>+  nsresult rv = permMgr->TestPermissionFromPrincipal(principal, "browser", &permission);
>   NS_ENSURE_SUCCESS(rv, NS_OK);

I don't think you even need to check for success, since if
TestPermissionFromPrincipal fails, it would be a bug if it set the permission
to anything other than DENY.

But either way, no problem...

>diff -u b/dom/browser-element/mochitest/browserElementTestHelpers.js b/dom/browser-element/mochitest/browserElementTestHelpers.js
>--- b/dom/browser-element/mochitest/browserElementTestHelpers.js	Mon Aug 13 14:36:27 2012 -0700
>+++ b/dom/browser-element/mochitest/browserElementTestHelpers.js	Tue Aug 14 14:59:18 2012 -0700
>@@ -75,18 +75,18 @@
> 
>   addPermission: function() {
>     SpecialPowers.addPermission("browser", true, document);
>+    this.tempPermissions.push(SpecialPowers.getURISpec(document));
>   },

Why can't you do location.href instead of SpecialPowers.getURISpec(document)?

>-  removePermission: function() {
>-    SpecialPowers.addPermission("browser", document);
>+  removeAllTempPermissions: function() {
>+    for(var i = 0; i < this.tempPermissions.length; i++) {
>+      SpecialPowers.removePermission("browser", this.tempPermissions[i]);
>+    }
>   },

This will have the unfortunate side-effect of nuking already-existing
permissions.  Bug I guess since this is browser-element-only code, and I don't
expect anyone to add persistent permissions for our pages, this is OK.

>@@ -121,6 +123,8 @@
> browserElementTestHelpers.origOOPByDefaultPref = browserElementTestHelpers.getOOPByDefaultPref();
> browserElementTestHelpers.origPageThumbsEnabledPref = browserElementTestHelpers.getPageThumbsEnabledPref();
> 
>+//browserElementTestHelpers.origPermissions = SpecialPowers.getPermissions();

Did you mean to remove this?

>--- a/testing/mochitest/tests/SimpleTest/specialpowersAPI.js	Sun Aug 12 13:43:47 2012 +0300
>+  getURISpec: function(urlOrDocument) {
>+    return this._getURI(urlOrDocument).spec;
>+  },

I don't think I can review this (assuming you actually need it).

Comment 14

[Gregor Wagner [:gwagner]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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