Last Comment Bug 781355 - Hook up mozBrowser to Permission Manager
: Hook up mozBrowser to Permission Manager
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
Depends on:
Blocks: 774716 764618
  Show dependency treegraph
 
Reported: 2012-08-08 14:55 PDT by Gregor Wagner [:gwagner]
Modified: 2012-08-15 18:45 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
patch (41.00 KB, patch)
2012-08-10 16:56 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (40.00 KB, patch)
2012-08-13 14:34 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (39.49 KB, patch)
2012-08-13 14:37 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (39.46 KB, patch)
2012-08-13 18:20 PDT, Gregor Wagner [:gwagner]
justin.lebar+bug: review-
Details | Diff | Review
patch (44.37 KB, patch)
2012-08-14 14:59 PDT, Gregor Wagner [:gwagner]
justin.lebar+bug: review+
Details | Diff | Review

Description Gregor Wagner [:gwagner] 2012-08-08 14:55:05 PDT
We should remove dom.mozBrowserFramesWhitelist.
Comment 1 Justin Lebar (not reading bugmail) 2012-08-08 18:43:02 PDT
I don't see why we would block shipping the phone if this bug weren't fixed.
Comment 2 Jonas Sicking (:sicking) 2012-08-10 09:47:10 PDT
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) 2012-08-10 09:58:36 PDT
> 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] 2012-08-10 16:56:43 PDT
Created attachment 651051 [details] [diff] [review]
patch

WiP
Comment 5 Gregor Wagner [:gwagner] 2012-08-13 14:34:54 PDT
Created attachment 651534 [details] [diff] [review]
patch
Comment 6 Gregor Wagner [:gwagner] 2012-08-13 14:37:33 PDT
Created attachment 651536 [details] [diff] [review]
patch
Comment 7 Gregor Wagner [:gwagner] 2012-08-13 14:38:25 PDT
Try run: https://tbpl.mozilla.org/?tree=Try&rev=3713094db832
Comment 8 Gregor Wagner [:gwagner] 2012-08-13 18:20:23 PDT
Created attachment 651602 [details] [diff] [review]
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) 2012-08-14 09:48:16 PDT
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) 2012-08-14 09:48:49 PDT
Comment on attachment 651602 [details] [diff] [review]
patch

r- just for not unsetting permissions when the tests complete.
Comment 11 Gregor Wagner [:gwagner] 2012-08-14 14:59:43 PDT
Created attachment 651898 [details] [diff] [review]
patch
Comment 12 Gregor Wagner [:gwagner] 2012-08-14 15:01:13 PDT
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) 2012-08-15 07:44:58 PDT
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 15 Ryan VanderMeulen [:RyanVM] 2012-08-15 18:45:15 PDT
https://hg.mozilla.org/mozilla-central/rev/c1fbceec9a05

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