Closed Bug 1362791 Opened 7 years ago Closed 7 years ago

Enable testing permissions using URIs without having to mint principals

Categories

(Core :: Permission Manager, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file, 1 obsolete file)

The permissions manager store uses principal origins with suffix in the
key entry, but for the API entry points where we accept a raw nsIURI, we
currently mint a new codebase principal with a blank OriginAttributes
only to read out the origin string effectively, since the suffix is
guaranteed to always be an empty string in this case.

This can be slow, so this patch adds a fast path to bypass minting a new
principal and uses ContentPrincipal::GenerateOriginNoSuffixFromURI() to
generate the origin string from the input nsIURI directly.
Assignee: nobody → ehsan
Blocks: 1353179
Blocks: 1347376
No longer blocks: 1353179
Comment on attachment 8865145 [details] [diff] [review]
Enable testing permissions using URIs without having to mint principals

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +238,5 @@
>  
>  // This function produces a nsIPrincipal which is identical to the current
>  // nsIPrincipal, except that it has one less subdomain segment. It returns
>  // `nullptr` if there are no more segments to remove.
> +static already_AddRefed<nsIPrincipal>

I don't think that the `static` here is necessary, as this function is within an anonymous namespace.

@@ +243,1 @@
>  GetNextSubDomainPrincipal(nsIPrincipal* aPrincipal)

Please change this method to use GetNextSubdomainURI internally to avoid code duplication.

@@ +286,5 @@
>  
> +// This function produces a nsIURI which is identical to the current
> +// nsIURI, except that it has one less subdomain segment. It returns
> +// `nullptr` if there are no more segments to remove.
> +static already_AddRefed<nsIURI>

Same with this static.

@@ +2219,5 @@
>  }
>  
>  nsresult
>  nsPermissionManager::CommonTestPermission(nsIPrincipal* aPrincipal,
> +                                          nsIURI     *aURI,

I don't like that we need to pass in both aURI and aPrincipal here. Can we split this into 3 methods, one which performs the prelude up to the call to GetPermissionHashKey for aPrincipal, one for aURI, and then a common internal function for the suffix?

@@ +2262,5 @@
>  
>      return NS_OK;
>    }
>  
> +  MOZ_ASSERT_IF(aPrincipal, PermissionAvaliable(aPrincipal, aType));

In debug builds, please fabricate a principal here and pass it to PermissionAvaliable. I don't want to lose this assertion if you call through the nsIURI*-based bindings.
Attachment #8865145 - Flags: review?(michael)
(In reply to Michael Layzell [:mystor] from comment #2)
> @@ +2219,5 @@
> >  }
> >  
> >  nsresult
> >  nsPermissionManager::CommonTestPermission(nsIPrincipal* aPrincipal,
> > +                                          nsIURI     *aURI,
> 
> I don't like that we need to pass in both aURI and aPrincipal here. Can we
> split this into 3 methods, one which performs the prelude up to the call to
> GetPermissionHashKey for aPrincipal, one for aURI, and then a common
> internal function for the suffix?

I don't think that really improves the code, and it would make things less efficient by adding a potential function call and more importantly duplicate the logic across the URL and principal code paths, which makes it likely for a bug fix in the future in one code to not be made in the other, etc.  If your objection is to the interface of this function, I can fix that at the interface level by providing two overloads.  What do you think?
Flags: needinfo?(michael)
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #3)
> (In reply to Michael Layzell [:mystor] from comment #2)
> > @@ +2219,5 @@
> > >  }
> > >  
> > >  nsresult
> > >  nsPermissionManager::CommonTestPermission(nsIPrincipal* aPrincipal,
> > > +                                          nsIURI     *aURI,
> > 
> > I don't like that we need to pass in both aURI and aPrincipal here. Can we
> > split this into 3 methods, one which performs the prelude up to the call to
> > GetPermissionHashKey for aPrincipal, one for aURI, and then a common
> > internal function for the suffix?
> 
> I don't think that really improves the code, and it would make things less
> efficient by adding a potential function call and more importantly duplicate
> the logic across the URL and principal code paths, which makes it likely for
> a bug fix in the future in one code to not be made in the other, etc.  If
> your objection is to the interface of this function, I can fix that at the
> interface level by providing two overloads.  What do you think?

Yeah, that's fine.
Flags: needinfo?(michael)
The permissions manager store uses principal origins with suffix in the
key entry, but for the API entry points where we accept a raw nsIURI, we
currently mint a new codebase principal with a blank OriginAttributes
only to read out the origin string effectively, since the suffix is
guaranteed to always be an empty string in this case.

This can be slow, so this patch adds a fast path to bypass minting a new
principal and uses ContentPrincipal::GenerateOriginNoSuffixFromURI() to
generate the origin string from the input nsIURI directly.
Attachment #8869591 - Flags: review?(michael)
Attachment #8865145 - Attachment is obsolete: true
Comment on attachment 8869591 [details] [diff] [review]
Enable testing permissions using URIs without having to mint principals

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +248,3 @@
>  {
>    nsCOMPtr<nsIURI> uri;
> +  nsresult rv = aURI->Clone(getter_AddRefs(uri));

Can we move the clone until after GetNextSubDomainForHost so that we don't bother copying it unless we actually need it to return?

@@ +2227,3 @@
>  {
> +  MOZ_ASSERT(aPrincipal || aURI);
> +  NS_ENSURE_ARG_POINTER(aPrincipal || aURI);

Can we have something like MOZ_ASSERT_IF(aPrincipal, !aURI) as well here, just to make sure that we don't have both?
Attachment #8869591 - Flags: review?(michael) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b115265af36
Enable testing permissions using URIs without having to mint principals; r=mystor
https://hg.mozilla.org/mozilla-central/rev/8b115265af36
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1379345
No longer blocks: 1379345
Depends on: 1377109
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: