Last Comment Bug 774819 - Handle system principals in nsPermissionManager when taking principals
: Handle system principals in nsPermissionManager when taking principals
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 769586 769594
  Show dependency treegraph
 
Reported: 2012-07-17 12:30 PDT by Mounir Lamouri (:mounir)
Modified: 2012-07-19 07:34 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (6.11 KB, patch)
2012-07-17 12:30 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch (6.11 KB, patch)
2012-07-17 12:32 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch (6.56 KB, patch)
2012-07-17 12:33 PDT, Mounir Lamouri (:mounir)
jonas: review+
mounir: checkin+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-07-17 12:30:34 PDT
Created attachment 643089 [details] [diff] [review]
Patch
Comment 1 Mounir Lamouri (:mounir) 2012-07-17 12:32:34 PDT
Created attachment 643090 [details] [diff] [review]
Patch

Without compilation errors ;)
Comment 2 Mounir Lamouri (:mounir) 2012-07-17 12:33:20 PDT
Created attachment 643091 [details] [diff] [review]
Patch

... with `hg qref` :)
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-17 21:33:44 PDT
Comment on attachment 643091 [details] [diff] [review]
Patch

Review of attachment 643091 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm.. I feel like I commented on this one already but it might have gotten lost. I don't have connectivity right now so I can't check.

::: extensions/cookie/nsPermissionManager.cpp
@@ +514,5 @@
>  
> +  // We don't add the system principal because it actually has no URI and we
> +  // always allow action for them.
> +  if (nsContentUtils::IsSystemPrincipal(aPrincipal)) {
> +    return NS_OK;

Make this MOZ_ASSERT and return an error.

@@ +709,5 @@
>    NS_ENSURE_ARG_POINTER(aPrincipal);
>  
> +  // System principals are never added to the database, no need to remove them.
> +  if (nsContentUtils::IsSystemPrincipal(aPrincipal)) {
> +    return NS_OK;

Make this MOZ_ASSERT and return an error.

::: netwerk/base/public/nsIPermissionManager.idl
@@ +93,5 @@
>     * Add permission information for a given principal.
>     * It is internally calling the other add() method using the nsIURI from the
>     * principal.
> +   * Passing a system principal will be a no-op because they will always be
> +   * granted permissions.

Update this comment.

@@ +118,5 @@
>    /**
>     * Remove permission information for a given principal.
>     * This is internally calling remove() with the host from the principal's URI.
> +   * Passing system principal will be a no-op because we never add them to the
> +   * database.

Update this comment.
Comment 4 Ed Morley [:emorley] 2012-07-19 07:34:52 PDT
https://hg.mozilla.org/mozilla-central/rev/d26ee2e87945

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