1048048 - Add internal contentPolicyTypes to allow identification of preloads within CSP

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Alan Stearns]

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

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)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

bz knows the CSS loader better than I do, though I could have a look if he's busy.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_tests.patch

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(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().

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Since we are about to add some new internal content policy types, I am marking the work from ehsan [Bug 1174307] blocking this bug.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types.patch

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

Comment 9

[Tanvi Vyas[:tanvi]]

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!

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types_tests.patch

So, here is more robust test that also works in e10s.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

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 13

[(no longer active)]

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.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 15

[(no longer active)]

(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?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types.patch

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

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types_images.patch

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types_stylesheets.patch

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types_scripts.patch

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types_csp.patch

Comment 21

[(no longer active)]

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.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could you get someone else to review the patches assigned to me, unless there's a specific reason you want my review?

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 26

[Seth Fowler [:seth] [:s2h]]

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).

Comment 27

[(no longer active)]

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.

Comment 28

[Cameron McCormack (:heycam)]

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...

Comment 29

[Cameron McCormack (:heycam)]

Watch out for conflicts with bug 1035091 (which does convert one of those boolean args into an enum) when rebasing.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 8644651 [details] [diff] [review]
bug_1048048_preload_types_csp.patch

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

r=dveditz

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types_tests.patch

Just rebased all the patches, carrying over r+ for all of them.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types.patch

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types_images.patch

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types_stylesheets.patch

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types_scripts.patch

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types_csp.patch

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1048048_preload_types_web_platform_tests.patch

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? :-)

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 8663467 [details] [diff] [review]
bug_1048048_preload_types_web_platform_tests.patch

r=dveditz

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02c6d6aef163
https://hg.mozilla.org/integration/mozilla-inbound/rev/740ab1ecd079
https://hg.mozilla.org/integration/mozilla-inbound/rev/88c2333ff745
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a727c40eb68
https://hg.mozilla.org/integration/mozilla-inbound/rev/450d4a13c90e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f91b10e8be0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5abe23a4ea5

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7398104fda0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e81b19b0f22
https://hg.mozilla.org/integration/mozilla-inbound/rev/dadd2ff2f761
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d75db26d40d
https://hg.mozilla.org/integration/mozilla-inbound/rev/56058bf8ec8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/94c62c16128d
https://hg.mozilla.org/integration/mozilla-inbound/rev/54777c071b5d

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7398104fda0b
https://hg.mozilla.org/mozilla-central/rev/0e81b19b0f22
https://hg.mozilla.org/mozilla-central/rev/dadd2ff2f761
https://hg.mozilla.org/mozilla-central/rev/7d75db26d40d
https://hg.mozilla.org/mozilla-central/rev/56058bf8ec8c
https://hg.mozilla.org/mozilla-central/rev/94c62c16128d
https://hg.mozilla.org/mozilla-central/rev/54777c071b5d

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 44

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 45

[Christoph Kerschbaumer [:ckerschb]]

(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