Add internal contentPolicyTypes to allow identification of preloads within CSP

RESOLVED FIXED in Firefox 44

Status

()

defect
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: stearns, Assigned: ckerschb)

Tracking

(Blocks 2 bugs)

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(7 attachments, 8 obsolete attachments)

9.06 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
19.17 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
28.86 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
25.07 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
9.74 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
6.69 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
762 bytes, patch
dveditz
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140728123914

Steps to reproduce:

1. Set style-src to 'self' using content security policy headers
2. Try to load a stylesheet from some other source using @import

I just submitted a test case to web-platform-tests for this (fails in Firefox, passes in Chrome)

https://github.com/w3c/web-platform-tests/pull/1146


Actual results:

The stylesheet fails to load, but no CSP violation report is sent


Expected results:

A report noting the style-src violation should be sent
Status: UNCONFIRMED → NEW
Component: Untriaged → Security
Ever confirmed: true
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All
Version: 32 Branch → Trunk
Blocks: CSP
Blocks: csp-w3c-1.0
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Priority: -- → P2
The root cause for not sending reports when using @import is that we do not send reports for preloads [1] and that we incorrectly classify the loading of the imported CSS as a preload. In case of a CSP violation for a preload we do block the load but do not send a report so that we do not send two reports for the same violation. I provided a stacktrace underneath, particularly relevant is [2]. In the testcase we do use 'mDocument' as the requestingContext for CheckLoadAllowed() and later for ShouldLoad)_. Later in [1], we query:
> nsCOMPtr<nsIDOMHTMLDocument> doc = do_QueryInterface(aRequestContext);
which succeeds and hence we incorrectly classify the load as a preload.

I don't think there is an easy fix to that because there is no other context available, but maybe David has an idea how we could resolve that problem!

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsCSPContext.cpp#138
[2] http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#2122


#01: nsCSPContext::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, short*) (nsCSPContext.cpp:188, in XUL)
#02: CSPService::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) (nsCSPService.cpp:194, in XUL)
#03: nsContentPolicy::CheckPolicy(tag_nsresult (nsIContentPolicy::*)(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*), unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) (nsContentPolicy.cpp:125, in XUL)
#04: nsContentPolicy::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) (nsContentPolicy.cpp:188, in XUL)
#05: NS_CheckContentLoadPolicy(unsigned int, nsIURI*, nsIPrincipal*, nsISupports*, nsACString_internal const&, nsISupports*, short*, nsIContentPolicy*, nsIScriptSecurityManager*) (nsContentPolicyUtils.h:217, in XUL)
#06: mozilla::css::Loader::CheckLoadAllowed(nsIPrincipal*, nsIURI*, nsISupports*) (Loader.cpp:1059, in XUL)
#07: mozilla::css::Loader::LoadChildSheet(mozilla::CSSStyleSheet*, nsIURI*, nsMediaList*, mozilla::css::ImportRule*) (Loader.cpp:2126, in XUL)
#08: (anonymous namespace)::CSSParserImpl::ProcessImport(nsString const&, nsMediaList*, void (*)(mozilla::css::Rule*, void*), void*, unsigned int, unsigned int) (nsCSSParser.cpp:3272, in XUL)
Flags: needinfo?(dbaron)
bz knows the CSS loader better than I do, though I could have a look if he's busy.
Flags: needinfo?(dbaron) → needinfo?(bzbarsky)
Posted patch bug_1048048_tests.patch (obsolete) — Splinter Review
Providing a half-baked testcase. I copied test_csp_report.html and adapted some parts so that it loads a CSS which uses @import for loading another CSS. Probably good enough for a first debug session.
Loader::LoadChildSheet walks up the parent sheet chain looking for the linking element.  Why doesn't it find it?  Is aParentSheet->GetOwningDocument() returning false because aOwningSheet was preloaded, perhaps (and a clone is what has the owning node)?

But a bigger question here is why there's this weird assumption about "context is document means preload"?  For CSS stuff there is no sane context because of the coalescing we do, so document really makes the most sense in some ways anyway.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #4)
> Loader::LoadChildSheet walks up the parent sheet chain looking for the
> linking element.  Why doesn't it find it?  Is
> aParentSheet->GetOwningDocument() returning false because aOwningSheet was
> preloaded, perhaps (and a clone is what has the owning node)?

Not sure about the reason, but aParentSheet->GetOwningDocument() returns false, probably for the reason you mentioned and hence we use mDocument as the context.

> But a bigger question here is why there's this weird assumption about
> "context is document means preload"?

I don't like that part either, but I think at the moment this is the best we can come up with. If there is a better way to identify whether we are calling shouldLoad() for a preload or not, please let us know and I am more than happy to fix this immediately.

> For CSS stuff there is no sane context
> because of the coalescing we do, so document really makes the most sense in
> some ways anyway.

That makes sense, maybe we can find a better solution to identify preloads in ShouldLoad().
> If there is a better way to identify whether we are calling shouldLoad() for a preload or
> not

Most simply, we could add new content policy types, right?
Component: Security → DOM: Security
Since we are about to add some new internal content policy types, I am marking the work from ehsan [Bug 1174307] blocking this bug.
Depends on: 1174307
Hey Ehsan, as discussed yesterday, I added new internal content policy types for
* image preloads
* script preloads
* style preloads
so that CSP is able to distinguish between actual loads and preloads. Ulitmately that separation will be used for speculative loads when implementing Meta CSP [Bug 663570], but it makes sense to do that within this patch to also clean up the way we currently identify preloads within CSP.

We have to propagate those flags for (a) content policy checks as well as (b) for creation of new channels since we need to store the contentPolicyType within the loadInfo. Further, we want to send the external content policyType for all contentPolicy checks but use the internal one for CSP checks.

Are you fine with that approach or am I missing something? If you are ok with that approach, I would split the patches up and ask individual peers for the preload scenarios.

William provided the following list for preloads, I suppose we only need the first three, but I am not 100% sure - here is the list.

[yes] Script: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1603
[yes] Style: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#2024
[yes] Image: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#2009
[???] Picture: source: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#1986
[???] Manifest: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentSink.cpp#1036
[???] Preconnect: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#2051
Flags: needinfo?(ehsan)
Attachment #8641337 - Flags: feedback?(ehsan)
Hi Christoph,

Separate internal content policy types for preloads sounds good!  How are you going to expose them only to CSP?  Since CSP is called from NS_CheckContentLoadPolicy and that API takes the external content policy type, do you plan to separate CSP out of NS_CheckContentLoadPolicy and call it individually?

Thanks!
(In reply to Tanvi Vyas [:tanvi] from comment #9)
> Hi Christoph,
> 
> Separate internal content policy types for preloads sounds good!  How are
> you going to expose them only to CSP?  Since CSP is called from
> NS_CheckContentLoadPolicy and that API takes the external content policy
> type, do you plan to separate CSP out of NS_CheckContentLoadPolicy and call
> it individually?

Nope, CSP remains an nsIContentPolicy. Just passing the internalPolicyType to CSP instead of the external one and CSP does it's own translation - see changes within nsContentPolicy::CheckPolicy in the attached patch.
So, here is more robust test that also works in e10s.
Attachment #8517618 - Attachment is obsolete: true
Comment on attachment 8641337 [details] [diff] [review]
bug_1048048_preload_types.patch

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

::: dom/base/nsIContentPolicyBase.idl
@@ +270,5 @@
> +  /**
> +   * Indicates a stylesheet preload (e.g., STYLE elements).
> +   */
> +  const nsContentPolicyType TYPE_INTERNAL_STYLESHEET_PRELOAD = 35;
> +

I just realized that I forgot to update the DBSchema:
http://hg.mozilla.org/mozilla-central/rev/593ac5424b6e

We have to make sure that happens here as well.
Comment on attachment 8641337 [details] [diff] [review]
bug_1048048_preload_types.patch

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

::: dom/base/nsContentPolicy.cpp
@@ +136,5 @@
> +        // * TYPE_INTERNAL_SCRIPT_PRELOAD
> +        // * TYPE_INTERNAL_IMAGE_PRELOAD
> +        // * TYPE_INTERNAL_STYLESHEET_PRELOAD
> +        bool isCSP = cspService == entries[i];
> +        rv = (entries[i]->*policyMethod)(isCSP ? contentType : externalType,

We should not pass the external type to the CSP policy!  I think what we want to do is to add another helper such as nsContentUtils::InternalContentPolicyTypeToExternalOrPreload() or some such (that name is awful!) that woudl handle the above _PRELOAD values by passing them through and calling into InternalContentPolicyTypeToExternal for other cases.

::: dom/base/nsIContentPolicyBase.idl
@@ +264,5 @@
> +
> +  /**
> +   * Indicates an image preload (e.g., IMG elements).
> +   */
> +  const nsContentPolicyType TYPE_INTERNAL_IMAGE_PRELOAD = 34;

We need to be able to differentiate between TYPE_IMAGE as opposed to this type, and TYPE_IMAGE as the mapped external type, so you need to add a TYPE_INTERNAL_IMAGE too.  TYPE_INTERNAL_SCRIPT also exists for this very reason.

@@ +269,5 @@
> +
> +  /**
> +   * Indicates a stylesheet preload (e.g., STYLE elements).
> +   */
> +  const nsContentPolicyType TYPE_INTERNAL_STYLESHEET_PRELOAD = 35;

Ditto.

::: dom/fetch/InternalRequest.cpp
@@ +122,5 @@
>      break;
>    case nsIContentPolicy::TYPE_INTERNAL_SHARED_WORKER:
>      context = RequestContext::Sharedworker;
>      break;
>    case nsIContentPolicy::TYPE_IMAGE:

This would be TYPE_INTERNAL_IMAGE.

@@ +128,3 @@
>      context = RequestContext::Image;
>      break;
>    case nsIContentPolicy::TYPE_STYLESHEET:

Ditto.

::: dom/fetch/InternalRequest.h
@@ +42,5 @@
>   * form              |
>   * frame             | TYPE_INTERNAL_FRAME
>   * hyperlink         |
>   * iframe            | TYPE_INTERNAL_IFRAME
> + * image             | TYPE_IMAGE, TYPE_INTERNAL_IMAGE_PRELOAD

This would be TYPE_INTERNAL_IMAGE.

@@ +57,3 @@
>   * sharedworker      | TYPE_INTERNAL_SHARED_WORKER
>   * subresource       | Not supported by Gecko
> + * style             | TYPE_STYLESHEET, TYPE_INTERNAL_STYLESHEET_PRELOAD

Ditto.

::: dom/security/nsCSPContext.cpp
@@ +118,5 @@
> +     aContentType == nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD);
> +
> +  // Since we know whether we are dealing with a preload, we have to convert
> +  // the internal policytype ot the external policy type before moving on.
> +  aContentType = nsContentUtils::InternalContentPolicyTypeToExternal(aContentType);

This should become unnecessary.

::: dom/security/nsCSPService.cpp
@@ -105,5 @@
>                         nsIPrincipal *aRequestPrincipal,
>                         int16_t *aDecision)
>  {
> -  MOZ_ASSERT(aContentType == nsContentUtils::InternalContentPolicyTypeToExternal(aContentType),
> -             "We should only see external content policy types here.");

You would need to change this assertion according to my comment on nsContentPolicy.cpp.

@@ +249,5 @@
>                            int16_t          *aDecision)
>  {
> +  // If this body is ever going to return something else than ACCEPT,
> +  // then we whave to convert aContentType to user the external rather
> +  // then the internal policy type!!!!

Same here.
Attachment #8641337 - Flags: feedback?(ehsan) → feedback-
Flags: needinfo?(ehsan)
Thanks for the feedback Ehsan!

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #13)
> ::: dom/base/nsContentPolicy.cpp
> @@ +136,5 @@
> > +        // * TYPE_INTERNAL_SCRIPT_PRELOAD
> > +        // * TYPE_INTERNAL_IMAGE_PRELOAD
> > +        // * TYPE_INTERNAL_STYLESHEET_PRELOAD
> > +        bool isCSP = cspService == entries[i];
> > +        rv = (entries[i]->*policyMethod)(isCSP ? contentType : externalType,
> 
> We should not pass the external type to the CSP policy!

I suppose you mean the internal one, right? We *do* want to pass the external one - the same way we pass the external on to all other content polcies.

> I think what we
> want to do is to add another helper such as
> nsContentUtils::InternalContentPolicyTypeToExternalOrPreload() or some such
> (that name is awful!) that woudl handle the above _PRELOAD values by passing
> them through and calling into InternalContentPolicyTypeToExternal for other
> cases.

I think that is not possible because if we pass the external contentPolicyType to CSP then there is a one to many relation that we can not resolve. Let's assume we pass TYPE_IMAGE to CSP then we don't know whether we are dealing with TYPE_INTERNAL_IMAGE or TYPE_INTERNAL_IMAGE_PRELOAD, or am I missing something?
Flags: needinfo?(ehsan)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> Thanks for the feedback Ehsan!
> 
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #13)
> > ::: dom/base/nsContentPolicy.cpp
> > @@ +136,5 @@
> > > +        // * TYPE_INTERNAL_SCRIPT_PRELOAD
> > > +        // * TYPE_INTERNAL_IMAGE_PRELOAD
> > > +        // * TYPE_INTERNAL_STYLESHEET_PRELOAD
> > > +        bool isCSP = cspService == entries[i];
> > > +        rv = (entries[i]->*policyMethod)(isCSP ? contentType : externalType,
> > 
> > We should not pass the external type to the CSP policy!
> 
> I suppose you mean the internal one, right? We *do* want to pass the
> external one - the same way we pass the external on to all other content
> polcies.

Oops, yeah I mean the internal one.  My bad!

> > I think what we
> > want to do is to add another helper such as
> > nsContentUtils::InternalContentPolicyTypeToExternalOrPreload() or some such
> > (that name is awful!) that woudl handle the above _PRELOAD values by passing
> > them through and calling into InternalContentPolicyTypeToExternal for other
> > cases.
> 
> I think that is not possible because if we pass the external
> contentPolicyType to CSP then there is a one to many relation that we can
> not resolve. Let's assume we pass TYPE_IMAGE to CSP then we don't know
> whether we are dealing with TYPE_INTERNAL_IMAGE or
> TYPE_INTERNAL_IMAGE_PRELOAD, or am I missing something?

The point is that for the CSP service, we should never pass TYPE_IMAGE.  We should either pass TYPE_INTERNAL_IMAGE, or TYPE_INTERNAL_IMAGE_PRELOAD.  The assertions I suggested you should add back will ensure that.  (Similarly for stylesheets.)

For all other content policy implementations, we should never pass TYPE_INTERNAL_IMAGE* and only pass TYPE_IMAGE to them.  (Again, similarly for stylesheets.)

In other words, the CSP service will get to see some of the internal types that we hide from other content policy implementations.

Does that make sense?
Flags: needinfo?(ehsan)
Summary: CSP violation report not sent for @import → Add internal contentPolicyTypes to allow identification of preloads within CSP
Hi Ehsan, I split the patches up to allow easier reviewing for images, stylesheets and scripts. I am only going to flag you for this first one, but in fact it would be great if you could review the following four patches within this bug:

bug_1048048_preload_types
bug_1048048_preload_types_images
bug_1048048_preload_types_stylesheets
bug_1048048_preload_types_scripts
Attachment #8641337 - Attachment is obsolete: true
Attachment #8644645 - Flags: review?(ehsan)
Comment on attachment 8644645 [details] [diff] [review]
bug_1048048_preload_types.patch

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

Looks great!

::: dom/base/nsContentUtils.h
@@ +939,5 @@
>    /**
> +   * Map internal content policy types to external ones or preload types:
> +   *   * TYPE_INTERNAL_SCRIPT_PRELOAD
> +   *   * TYPE_INTERNAL_IMAGE_PRELOAD
> +   *   * TYPE_INTERNAL_STYLESHEET_PRELOAD

Can you please add something along the lines of "this is probably not what you want to use, use InternalContentPolicyTypeToExternal instead"?

::: dom/base/nsIContentPolicyBase.idl
@@ +284,5 @@
> +   * Indicates an image (e.g., IMG elements).
> +   */
> +
> +  /**
> +   * Indicates an internal constant for images.

Nit: normal images.

@@ +300,5 @@
> +   */
> +  const nsContentPolicyType TYPE_INTERNAL_IMAGE_PRELOAD = 37;
> +
> +  /**
> +   * Indicates an internal constant for stylesheets.

Nit: normal stylesheets.
Attachment #8644645 - Flags: review?(ehsan) → review+
Could you get someone else to review the patches assigned to me, unless there's a specific reason you want my review?
Comment on attachment 8644647 [details] [diff] [review]
bug_1048048_preload_types_images.patch

Seth, any chance you can review those bits? The idea is to split the contentPolicyType for preloads and actual loads so that the LoadInfo on the channel can tell us whether we are dealing with a speculative load or an actual load.

So IMO the most important thing you would have to make sure is that nsDocument::MaybePreLoadImage passes the right argument. All the other arguments are merely just updates from TYPE_IMAGE to TYPE_INTERNAL_IMAGE to be consistent throughout the codebase.
Attachment #8644647 - Flags: review?(jonas) → review?(seth)
Comment on attachment 8644648 [details] [diff] [review]
bug_1048048_preload_types_stylesheets.patch

Cameron, a bit of background: We are going to use a different content policy type for preloads and actual loads so that CSP can distinguish between preloads and actual loads. We need to pass that information to content policies, but also store the new content policy type within the loadInfo of the channel, because within Bug 1182535 we are going to remove all security checks from the callsite and move them into asyncOpen(2). Hence we need to know about the type in the loadInfo as well.
Attachment #8644648 - Flags: review?(jonas) → review?(cam)
Comment on attachment 8644649 [details] [diff] [review]
bug_1048048_preload_types_scripts.patch

Baku, any chance you can review this? Already chattet with Ehsan, he also suggested you as a reviewer. Please see Comment 23 and Comment 24 for details. Thanks!
Attachment #8644649 - Flags: review?(jonas) → review?(amarchesini)
Attachment #8644649 - Flags: review?(ehsan)
Attachment #8644649 - Flags: review?(amarchesini)
Attachment #8644649 - Flags: review+
Comment on attachment 8644647 [details] [diff] [review]
bug_1048048_preload_types_images.patch

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

LGTM. Probably should update the commit message to say "r=seth" (or "r=ehsan,seth" if Ehsan did review this patch; I may have missed it).
Attachment #8644647 - Flags: review?(seth) → review+
Comment on attachment 8644649 [details] [diff] [review]
bug_1048048_preload_types_scripts.patch

Please let me know if you also want me to review this.
Attachment #8644649 - Flags: review?(ehsan)
Comment on attachment 8644648 [details] [diff] [review]
bug_1048048_preload_types_stylesheets.patch

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

I'm not sure what bits of this ehsan has already reviewed.  The s/TYPE_STYLESHEET/TYPE_INTERNAL_STYLESHEET/ bits seem to accord with previous discussion on the bug, so rs=me on those bits and r=me on the rest.

::: layout/style/Loader.cpp
@@ +1002,5 @@
>  nsresult
>  Loader::CheckLoadAllowed(nsIPrincipal* aSourcePrincipal,
>                           nsIURI* aTargetURI,
> +                         nsISupports* aContext,
> +                         bool aIsPreload)

Please add a doc comment for the new arg.

@@ +1023,5 @@
>  
>      // Check with content policy
> +    nsContentPolicyType contentPolicyType = aIsPreload
> +                                            ? nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD
> +                                            : nsIContentPolicy::TYPE_INTERNAL_STYLESHEET;

Please rewrap to fit 80 columns, maybe like:

  nsContentPolicyType contentPolicyType =
    aIsPreload ? nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD
               : nsIContentPolicy::TYPE_INTERNAL_STYLESHEET;

@@ +1554,5 @@
>    }
>  
> +  nsContentPolicyType contentPolicyType = aIsPreload
> +                                          ? nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD
> +                                          : nsIContentPolicy::TYPE_INTERNAL_STYLESHEET;

As above.

::: layout/style/Loader.h
@@ +444,5 @@
>                              CSSStyleSheet* aParentSheet,
>                              ImportRule* aParentRule);
>  
>    nsresult InternalLoadNonDocumentSheet(nsIURI* aURL,
> +                                        bool aIsPreload,

The number of boolean arguments is getting a bit out of hand here. :(  But Loader's API needs cleanup in general...
Attachment #8644648 - Flags: review?(cam) → review+
Watch out for conflicts with bug 1035091 (which does convert one of those boolean args into an enum) when rebasing.
Comment on attachment 8644651 [details] [diff] [review]
bug_1048048_preload_types_csp.patch

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

r=dveditz
Attachment #8644651 - Flags: review?(dveditz) → review+
Attachment #8643239 - Flags: review?(dveditz) → review+
See Also: → 1189668
Just rebased all the patches, carrying over r+ for all of them.
Attachment #8643239 - Attachment is obsolete: true
Attachment #8644645 - Attachment is obsolete: true
Attachment #8644647 - Attachment is obsolete: true
Attachment #8644648 - Attachment is obsolete: true
Attachment #8644649 - Attachment is obsolete: true
Attachment #8644651 - Attachment is obsolete: true
Attachment #8663461 - Flags: review+
Hey Dan, I think it's a good sign that the web-platform test for this issue is not failing anymore. What do you think? :-)
Attachment #8663467 - Flags: review?(dveditz)
Comment on attachment 8663467 [details] [diff] [review]
bug_1048048_preload_types_web_platform_tests.patch

r=dveditz
Attachment #8663467 - Flags: review?(dveditz) → review+
I backed this out for Android bustage, but I believe it was some other patches that caused that bustage. I'll reland before I reopen, assuming the bustage is fixed by my other backouts. Sorry for the churn here.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f837630f556
Andrea, Boris, when writing those patches I was relying on the fact that IsPreload() [1] returns whether it's a script preload or not - apparently that's not always reliable, because PreloadURI() [2] sets the mElement to a nullptr. Unfortunately the patch only added a testcase for stylesheets so that problem remained undiscovered until now when I accidentally realized the problem.

Any ideas how we can fix that?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.h#93
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1683
Flags: needinfo?(bzbarsky)
Flags: needinfo?(amarchesini)
Depends on: 1210321
> apparently that's not always reliable, because PreloadURI() [2] sets the mElement to a
> nullptr.

I'm not sure I follow.  PreloadURI should be the only thing that sets mElement to nullptr, so IsPreload() is true if and only if the load came through PreloadURI, right?  Which part of that is causing problems?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(mozilla)
(In reply to Boris Zbarsky [:bz] from comment #44)
> > apparently that's not always reliable, because PreloadURI() [2] sets the mElement to a
> > nullptr.
> 
> I'm not sure I follow.  PreloadURI should be the only thing that sets
> mElement to nullptr, so IsPreload() is true if and only if the load came
> through PreloadURI, right?  Which part of that is causing problems?

Facepalm, I converted nsScriptLoader to use AsyncOpen2() but forgot to pass the *internal* policy type to content policies. All good now.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#247
Flags: needinfo?(mozilla)
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.