1174307 - Add some internal content policy types for the purpose of reflecting them on RequestContext

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

These new content policy types will be internal ones that we will map
to external nsContentPolicyTypes before passing them to content policy
implementations.

Comment 1

[(no longer active)]

Attachment: Add some internal content policy types for the purpose of reflecting them on RequestContext

These new content policy types will be internal ones that we will map
to external nsContentPolicyTypes before passing them to content policy
implementations.

Comment 2

[(no longer active)]

Comment on attachment 8621820 [details] [diff] [review]
Add some internal content policy types for the purpose of reflecting them on RequestContext

This is what we discussed a couple of months ago for exposing the correct RequestContext without creating new content policy types that will be exposed to extensions, etc.

I will use these TYPE_INTERNAL_* values in future patches...

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8621820 [details] [diff] [review]
Add some internal content policy types for the purpose of reflecting them on RequestContext

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

Hi Ehsan, I see what you are doing, but I am not a dom-peer, hence I can not r+ the patch, but since Jonas is one, you are good to go :-)

::: dom/base/nsContentPolicyUtils.h
@@ +122,5 @@
> +    CASE_RETURN( TYPE_INTERNAL_FRAME    );
> +    CASE_RETURN( TYPE_INTERNAL_IFRAME   );
> +    CASE_RETURN( TYPE_INTERNAL_AUDIO    );
> +    CASE_RETURN( TYPE_INTERNAL_VIDEO    );
> +    CASE_RETURN( TYPE_INTERNAL_TRACK    );

nit: you could align the closing parens with *_SHARED_WORKER all the way down.

::: dom/base/nsIContentPolicy.idl
@@ +19,5 @@
>   * WARNING: do not block the caller from shouldLoad or shouldProcess (e.g.,
>   * by launching a dialog to prompt the user for something).
>   */
>  
> +[scriptable,uuid(b545899e-42bd-434c-8fec-a0af3448ea15)]

I don't think you need a new uuid here, do you?

::: dom/base/nsIContentPolicyBase.idl
@@ +23,5 @@
>   * WARNING: do not block the caller from shouldLoad or shouldProcess (e.g.,
>   * by launching a dialog to prompt the user for something).
>   */
>  
> +[scriptable,uuid(11b8d725-7c2b-429e-b51f-8b5b542d5009)]

Also here, you are just adding constants, right? no need for a new uuid.

@@ +191,5 @@
> +  const nsContentPolicyType TYPE_INTERNAL_SCRIPT = 23;
> +
> +  /**
> +   * Indicates an internal constant for scripts loaded through creating a
> +   * dedidated worker.

something seems off with "loaded through creating a ..." - can you clean that sentence up. Also, fix typo for 'dedidated'.

@@ +200,5 @@
> +  const nsContentPolicyType TYPE_INTERNAL_WORKER = 24;
> +
> +  /**
> +   * Indicates an internal constant for scripts loaded through creating a
> +   * shared worker.

same here, please check that sentence.

::: dom/base/nsISimpleContentPolicy.idl
@@ +27,5 @@
>   * WARNING: do not block the caller from shouldLoad or shouldProcess (e.g.,
>   * by launching a dialog to prompt the user for something).
>   */
>  
> +[scriptable,uuid(b181c97c-9d67-4da1-95a0-e0a202e1807c)]

also here, do you need a new uuid?

::: dom/security/nsMixedContentBlocker.cpp
@@ +339,5 @@
>    // to them.
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  MOZ_ASSERT(aContentType == nsContentUtils::InternalContentPolicyTypeToExternal(aContentType),
> +             "We should only see external content policy types here.");

Can you also add that assertion to
> CSPService::ShouldLoad(uint32_t aContentType

::: extensions/permissions/nsContentBlocker.cpp
@@ +52,5 @@
> +                                    "",
> +                                    "",
> +                                    "",
> +                                    "",
> +                                    ""};

do we really want empty strings here? I would rather see the internal one than getting an empty string if I would have to debug that.

Comment 4

[(no longer active)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Comment on attachment 8621820 [details] [diff] [review]
> Add some internal content policy types for the purpose of reflecting them on
> RequestContext
> 
> Review of attachment 8621820 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Ehsan, I see what you are doing, but I am not a dom-peer, hence I can not
> r+ the patch, but since Jonas is one, you are good to go :-)

Cool, thanks for the feedback!  :-)

> ::: dom/base/nsContentPolicyUtils.h
> @@ +122,5 @@
> > +    CASE_RETURN( TYPE_INTERNAL_FRAME    );
> > +    CASE_RETURN( TYPE_INTERNAL_IFRAME   );
> > +    CASE_RETURN( TYPE_INTERNAL_AUDIO    );
> > +    CASE_RETURN( TYPE_INTERNAL_VIDEO    );
> > +    CASE_RETURN( TYPE_INTERNAL_TRACK    );
> 
> nit: you could align the closing parens with *_SHARED_WORKER all the way
> down.

Will do.

> ::: dom/base/nsIContentPolicy.idl
> @@ +19,5 @@
> >   * WARNING: do not block the caller from shouldLoad or shouldProcess (e.g.,
> >   * by launching a dialog to prompt the user for something).
> >   */
> >  
> > +[scriptable,uuid(b545899e-42bd-434c-8fec-a0af3448ea15)]
> 
> I don't think you need a new uuid here, do you?

This is done for the base class uuid revision (we typically rev all interface IDs in the inheritance chain.)

> ::: dom/base/nsIContentPolicyBase.idl
> @@ +23,5 @@
> >   * WARNING: do not block the caller from shouldLoad or shouldProcess (e.g.,
> >   * by launching a dialog to prompt the user for something).
> >   */
> >  
> > +[scriptable,uuid(11b8d725-7c2b-429e-b51f-8b5b542d5009)]
> 
> Also here, you are just adding constants, right? no need for a new uuid.

I'm pretty sure that in the past I've seen new constants added without a uuid update not find their ways in the generated xpt files for incremental builds, which can lead to sadness when landing the patch.  I don't remember whether the underlying issue was ever fixed, but since this is mostly harmless, I prefer to keep it.  :-)

> ::: dom/security/nsMixedContentBlocker.cpp
> @@ +339,5 @@
> >    // to them.
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> > +  MOZ_ASSERT(aContentType == nsContentUtils::InternalContentPolicyTypeToExternal(aContentType),
> > +             "We should only see external content policy types here.");
> 
> Can you also add that assertion to
> > CSPService::ShouldLoad(uint32_t aContentType

Of course.

> ::: extensions/permissions/nsContentBlocker.cpp
> @@ +52,5 @@
> > +                                    "",
> > +                                    "",
> > +                                    "",
> > +                                    "",
> > +                                    ""};
> 
> do we really want empty strings here? I would rather see the internal one
> than getting an empty string if I would have to debug that.

The reason why I'm adding the empty strings is that further down we convert the policy type numbers to indices into this array, and while none of these internal names should not have any effect, as ensured by the other two hunks in this file, not including the strings here is going to screw up the next person who adds a content policy type, since once they add their item at the end of this array, the index for that item would be wrong.

The reason why I have chosen to use empty strings here is to make it simpler to ignore them later on in this file.  For example, if I use "internal-foo" for one of these items, and someone adds a permissions.default.internal-foo pref to their profile, then the code in PrefChanged needs to know to ignore that pref change.  I could of course compare the beginnings of the strings I receive with "internal-" to ensure that they are not internal names, but this seems simpler and more efficient.

But if you feel strongly about this, I can obviously change it, not a big deal.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #4)
> > > +[scriptable,uuid(11b8d725-7c2b-429e-b51f-8b5b542d5009)]
> > 
> > Also here, you are just adding constants, right? no need for a new uuid.
> 
> I'm pretty sure that in the past I've seen new constants added without a
> uuid update not find their ways in the generated xpt files for incremental
> builds, which can lead to sadness when landing the patch.  I don't remember
> whether the underlying issue was ever fixed, but since this is mostly
> harmless, I prefer to keep it.  :-)

I am fine either way, people have pointed it out in lots of my patches that there is no need to change the uuid if you are just adding constants.

> > > +                                    "",
> > > +                                    "",
> > > +                                    "",
> > > +                                    ""};
> > 
> > do we really want empty strings here? I would rather see the internal one
> > than getting an empty string if I would have to debug that.
> 
> The reason why I'm adding the empty strings is that further down we convert
> the policy type numbers to indices into this array, and while none of these
> internal names should not have any effect, as ensured by the other two hunks
> in this file, not including the strings here is going to screw up the next
> person who adds a content policy type, since once they add their item at the
> end of this array, the index for that item would be wrong.
> 
> The reason why I have chosen to use empty strings here is to make it simpler
> to ignore them later on in this file.  For example, if I use "internal-foo"
> for one of these items, and someone adds a permissions.default.internal-foo
> pref to their profile, then the code in PrefChanged needs to know to ignore
> that pref change.  I could of course compare the beginnings of the strings I
> receive with "internal-" to ensure that they are not internal names, but
> this seems simpler and more efficient.

Makes sense, it just feels slighlty weired to have a few empty strings in an array.
How about adding a comment on the side for each empty string:

>                    "", // TYPE_INTERNAL_SCRIPT
>                    "", // ...
>                    "", // ...

> 
> But if you feel strongly about this, I can obviously change it, not a big
> deal.

I don't feel strongly, either way is fine with me.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eefebfb86eb

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1eefebfb86eb

Comment 8

[Eric Shepherd [:sheppy]]

Doc updated: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIContentPolicy

(Yes, this is actually in nsIContentPolicyBase but realistically, it's surprising we're updating the doc at all at this point). :D

Comment 9

[Eric Shepherd [:sheppy]]

Not actually done yet but in process; hit save too soon here.

Comment 10

[Eric Shepherd [:sheppy]]

Now it's done!