Thomas Nguyen (:tnguyen) Assignee
	Description • 7 years ago

See
https://w3c.github.io/webappsec-referrer-policy/#integration-with-css

	Thomas Nguyen (:tnguyen) Assignee
	Comment 1 • 7 years ago

MozReview-Commit-ID: 84AoFANESJe

	Thomas Nguyen (:tnguyen) Assignee
	Comment 2 • 7 years ago

Comment on attachment 8920470 [details] [diff] [review]
Apply referrer policy to reequest resources referenced from stylesheet

The basic requirements in this bug are all resources fetching from stylesheet should follow referrer policy. The request should take referrer as 
- Sheet uri if stylesheet has a location (for example external stylesheet)
- Document uri for null location or css declaration
Then apply referrer policy which is defined:
- Referrer-Policy header in response if there's any
- Document's referrer policy for null location and css declaration.
The spec does not mention explicitly, but I think referrer policy should propagate inheritably from parent to child stylesheet.
The patch works fine locally for the wpt test in 
https://github.com/w3c/web-platform-tests/commit/445cf62a150698f8c67fd1757304139f5d462576
The test does not cover the child sheet loading and @font-face and we may have to complement that. Before doing that, I may need to ask a feedback from css expert to see if I am missing anything.
Hi Cameron, could you please take a look at this?

	Cameron McCormack (:heycam)
	Comment 3 • 7 years ago

(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #2)
> The test does not cover the child sheet loading and @font-face and we may
> have to complement that. Before doing that, I may need to ask a feedback
> from css expert to see if I am missing anything.

The only other thing I can think of is SVG resource documents referenced by CSS properties, e.g. filter:url(...), marker-start:url(...), etc.  These loads get initiated nsExternalResourceMap::PendingLoad::StartLoad, though that's also used for non-CSS SVG resource document references too, like <linearGradient xlink:href="...">.  It's probably worth checking what referrer we're actually sending here.

There's also -moz-binding but I guess we don't have to worry about that.

	Cameron McCormack (:heycam)
	Comment 4 • 7 years ago

Comment on attachment 8920470 [details] [diff] [review]
Apply referrer policy to reequest resources referenced from stylesheet

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

Looks good to me.

::: layout/style/FontFaceSet.cpp
@@ +657,5 @@
>  
>    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(channel));
>    if (httpChannel) {
>      rv = httpChannel->SetReferrerWithPolicy(aFontFaceSrc->mReferrer,
> +                                            aFontFaceSrc->mReferrerPolicy);

Should we check if it's RP_Unset and fall back to the document's referrer policy here?

::: layout/style/Loader.cpp
@@ +637,5 @@
> +  }
> +
> +  NS_ConvertUTF8toUTF16 headerValue(tRPHeaderCValue);
> +  net::ReferrerPolicy policy =
> +    nsContentUtils::GetReferrerPolicyFromHeader(headerValue);

It might be good to have an nsContentUtils::GetReferrerPolicyFromChannel function so we don't have to do all this in here.

@@ +940,5 @@
>    }
>    return NS_OK;
>  }
>  
>  nsresult

May as well make this void, since we only ever return NS_OK.

::: layout/style/Loader.h
@@ +473,5 @@
>    // cache.
>    nsresult ObsoleteSheet(nsIURI* aURI);
>  
> +  nsresult UpdateLoadingData(URIPrincipalReferrerPolicyAndCORSModeHashKey* aKey,
> +                             SheetLoadData* aData);

Probably this shouldn't be public, since we only need to call it from SheetLoadData (a friend).

::: layout/style/ServoStyleSheet.cpp
@@ +208,5 @@
>  {
>    MOZ_ASSERT(!mMedia || mMedia->IsServo());
>    RefPtr<URLExtraData> extraData =
>      new URLExtraData(aBaseURI, aSheetURI, aSheetPrincipal);
> +  extraData->SetReferrerPolicy(mInner->mReferrerPolicy);

Let's just add the referrer policy as an extra URLExtraData constructor argument.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 5 • 7 years ago

(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #3)
> (In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #2)
> > The test does not cover the child sheet loading and @font-face and we may
> > have to complement that. Before doing that, I may need to ask a feedback
> > from css expert to see if I am missing anything.
> 
> The only other thing I can think of is SVG resource documents referenced by
> CSS properties, e.g. filter:url(...), marker-start:url(...), etc.  These
> loads get initiated nsExternalResourceMap::PendingLoad::StartLoad, though
> that's also used for non-CSS SVG resource document references too, like
> <linearGradient xlink:href="...">.  It's probably worth checking what
> referrer we're actually sending here.
> 
> There's also -moz-binding but I guess we don't have to worry about that.

Great, thanks Cameron. I am looking at the SVG loading, it seems we only use default policy in this case.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 8 • 7 years ago

Hi David, sorry for tagging you here, could you please take a look at this patch?
- The basic requirements in this bug are all resources fetching from stylesheet should follow referrer policy in 
https://w3c.github.io/webappsec-referrer-policy/#integration-with-css
- The basic idea of wpt test is : the stylesheet request will store the request headers in server side then we later send another checking request again via XHR (adding ?report-header). The server will reply back a json of the headers that were used to request in the first place via CSS.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 9 • 7 years ago

Due to MozReview's inability to inspect old patch state in a usable way
(some combination of bugs like bug 1234279, bug 1249468, bug 1296135),
I'm no longer accepting review requests in MozReview.   If you'd like me
to review patches, please submit those review requests as attached patch
files.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 10 • 7 years ago

MozReview-Commit-ID: GZjLrlmoK4C

	Thomas Nguyen (:tnguyen) Assignee
	Comment 11 • 7 years ago

MozReview-Commit-ID: 84AoFANESJe

	Thomas Nguyen (:tnguyen) Assignee
	Comment 12 • 7 years ago

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #9)
> Due to MozReview's inability to inspect old patch state in a usable way
> (some combination of bugs like bug 1234279, bug 1249468, bug 1296135),
> I'm no longer accepting review requests in MozReview.   If you'd like me
> to review patches, please submit those review requests as attached patch
> files.

I see, attached the patches as you suggested. Thanks.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 13 • 7 years ago

Comment on attachment 8923688 [details] [diff] [review]
Apply referrer policy to request resources referenced from stylesheet

This isn't quite a complete review, but I think there are enough issues here to justify asking for a new patch for further review.  (I'll be unavailable next week, so it might be better to ask somebody else (probably heycam or bzbarsky).


css::URLValue should probably have the referrer policy as an added
parameter to the (now) 4-argument constructor and the (now) 5-argument
constructor.  This will simplify the code change in
CSSParserImpl::SetValueToURL, and require that you also change
nsGenericHTMLElement::ParseBackgroundAttribute.  (Presumably there
should be an appropriate referrer policy set there too?)  It seems
better to make it a parameter to the constructor since it seems like it
should be required.

I'd note that there are also some cases in SVG where we send an
incorrect referrer -- at least pre-Stylo.  I have part of a patch to fix
this in my tree, the current state of which is:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/53f130d100e7/unapplied.pass-cssvalue-url-through
Implementing Referrer-Policy seems like it might make this more
important to fix.  (I probably don't have time.)




The method name on nsContentUtils, GetReferrerPolicyFromResponse, seems odd,
given that it's getting it from a channel.  Why not call it
GetReferrerPolicyFromChannel?

In nsContentUtils:
>+  rv = httpChannel->GetRequestSucceeded(&requestSucceeded);

What in the spec justifies ignoring Referrer-Policy when displaying error
responses?  If something, it should probably be cited here; if nothing,
this code seems wrong.

>+  if (NS_FAILED(rv) || !requestSucceeded) {
>+    return NS_ERROR_FAILURE;
>+  }

In the NS_FAILED(rv) case, perhaps this should propagate rv?

In the !requestSucceeded case, it seems like this should just be NS_OK; getting
an HTTP 4xx response isn't an error in our code.

That said, I don't see any justification for returning nsresult from this function
at all.  It should just return mozilla::net::ReferrerPolicy like
GetReferrerPolicyFromHeader does.

>+  NS_ConvertUTF8toUTF16 headerValue(tRPHeaderCValue);
>+  aPolicy = GetReferrerPolicyFromHeader(headerValue);
I'd suggest just writing:

  aPolicy =
    GetReferrerPolicyFromHeader(NS_ConvertUTF8toUTF16(tRPHeaderCValue));

to avoid having to name an extra variable.

Also, the name tRPHeaderCValue seems very strange.  Why not just call
that headerValue?




In nsExternalResourceMap::PendingLoad::StartLoad, it's not clear to me
why the correct referrer is the document.  Isn't this used for various
references between SVG files?  Shouldn't the referrer thus be the
referencing file (and the policy used be its referrer-policy)?

It seems like the spec is also short here; the integration with SVG
should be explained in the spec, given the complexity of referencing in
SVG.

What do other browsers do here?

Also, other code seems to warn if SetReferrerWithPolicy fails, such as:
https://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/layout/style/FontFaceSet.cpp#660-662



In FontFaceSet::FindOrCreateUserFontEntryFromFontFace, the code appears
to fall back to the document's referrer policy if the style sheet's
referrer policy is unset.  The spec doesn't say to do this, so you shouldn't.
(Do other implementations?  Or is there a reason you think this is desirable?
If so, maybe a spec issue should be raised.)

The same is true of ImageLoader::LoadImage.


Why does SheetLoadData::GetReferrerPolicy do this same sort of
inheritance, but both to parent sheets and to documents?  I don't see
the spec saying to do this.  (See my comments on FontFaceSet.)


SheetLoadData::SetReferrerPolicyFromHeader should just return void, not
nsresult.  (It might also be good if bz looked at this part of the
patch.  That said, I'm not even sure how much this code is used
post-Stylo.)

Loader::UpdateLoadingData should probably rename:
  aKey -> aOldKey
  key -> newKey
since that's clearer.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 14 • 7 years ago

(Note that all of these behavior differences I mention should be testable -- so if there aren't such tests already, you should be adding tests to web-platform-tests that fail with the current patch but pass with the behavior fixed.)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 15 • 7 years ago

Comment on attachment 8923687 [details] [diff] [review]
Add more referrer policy css tests.

I'm not sure what the policies on renaming files within wpt (in order to restructure tests) are.  James?

	Thomas Nguyen (:tnguyen) Assignee
	Comment 16 • 7 years ago

(In reply to David Baron :dbaron: ⌚️UTC-8 (busy week of November 6-10) from comment #13)
> Comment on attachment 8923688 [details] [diff] [review]
> Apply referrer policy to request resources referenced from stylesheet
> 
> This isn't quite a complete review, but I think there are enough issues here
> to justify asking for a new patch for further review.  (I'll be unavailable
> next week, so it might be better to ask somebody else (probably heycam or
> bzbarsky).

Thanks for sharing your time, that is a very insightful review.


> 
> The method name on nsContentUtils, GetReferrerPolicyFromResponse, seems odd,
> given that it's getting it from a channel.  Why not call it
> GetReferrerPolicyFromChannel?
> 
> In nsContentUtils:
> >+  rv = httpChannel->GetRequestSucceeded(&requestSucceeded);
> 
> What in the spec justifies ignoring Referrer-Policy when displaying error
> responses?  If something, it should probably be cited here; if nothing,
> this code seems wrong.
> 
> >+  if (NS_FAILED(rv) || !requestSucceeded) {
> >+    return NS_ERROR_FAILURE;
> >+  }
> 
> In the NS_FAILED(rv) case, perhaps this should propagate rv?
> 
> In the !requestSucceeded case, it seems like this should just be NS_OK;
> getting
> an HTTP 4xx response isn't an error in our code.

You are right, I moved this from style/Loader.cpp to more common place nsContentUtils and did not removing !requestSucceeded.
We can bail out !requestSucceeded in Css loader but it seems wrong in common case
> That said, I don't see any justification for returning nsresult from this
> function
> at all.  It should just return mozilla::net::ReferrerPolicy like
> GetReferrerPolicyFromHeader does.
> 
> >+  NS_ConvertUTF8toUTF16 headerValue(tRPHeaderCValue);
> >+  aPolicy = GetReferrerPolicyFromHeader(headerValue);
> I'd suggest just writing:
> 
>   aPolicy =
>     GetReferrerPolicyFromHeader(NS_ConvertUTF8toUTF16(tRPHeaderCValue));
> 
> to avoid having to name an extra variable.
> 
> Also, the name tRPHeaderCValue seems very strange.  Why not just call
> that headerValue?
> 
Thank you, I will refactor that.
> 
> In nsExternalResourceMap::PendingLoad::StartLoad, it's not clear to me
> why the correct referrer is the document.  Isn't this used for various
> references between SVG files?  Shouldn't the referrer thus be the
> referencing file (and the policy used be its referrer-policy)?
> 
> It seems like the spec is also short here; the integration with SVG
> should be explained in the spec, given the complexity of referencing in
> SVG.
> 
> What do other browsers do here?

I see Chrome has landed Referrer Policy in CSS
https://codereview.chromium.org/2780533002 and it seems they did not care about SVG case. Referrer policy integration is not explicitly shown in the specs so I think we should at least use the referrer and referrer policy from document. For further details, I filed an open bug and we could discuss how to integrate referrer policy in SVG exactly.
We could fix that in a follow up bug, shouldn't we?
> I'd note that there are also some cases in SVG where we send an
> incorrect referrer -- at least pre-Stylo.  I have part of a patch to fix
> this in my tree, the current state of which is:
> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> 53f130d100e7/unapplied.pass-cssvalue-url-through
> Implementing Referrer-Policy seems like it might make this more
> important to fix.  (I probably don't have time.)
> 
Thanks for the patch. I think we should fix that in a follow up bug, wait for a while until we could determine how to do with SVG

> Also, other code seems to warn if SetReferrerWithPolicy fails, such as:
> https://searchfox.org/mozilla-central/rev/
> af86a58b157fbed26b0e86fcd81f1b421e80e60a/layout/style/FontFaceSet.cpp#660-662
> 
> 
> 
> In FontFaceSet::FindOrCreateUserFontEntryFromFontFace, the code appears
> to fall back to the document's referrer policy if the style sheet's
> referrer policy is unset.  The spec doesn't say to do this, so you shouldn't.
> (Do other implementations?  Or is there a reason you think this is desirable?
> If so, maybe a spec issue should be raised.)
> 
> The same is true of ImageLoader::LoadImage.
> 
> 
> Why does SheetLoadData::GetReferrerPolicy do this same sort of
> inheritance, but both to parent sheets and to documents?  I don't see
> the spec saying to do this.  (See my comments on FontFaceSet.)

You are right, spec does not mention explicitly about that, but I added this behaviour intentionally. Spec says Referrer policy can be defined "Implicitly, via inheritance.". So in the case unset, Referrer policy will fall back to parent's policy or document's policy.  Does it look more reasonable?

	Thomas Nguyen (:tnguyen) Assignee
	Comment 17 • 7 years ago

A couple of open bugs in spec
https://github.com/w3c/webappsec-referrer-policy/issues/109#issuecomment-342202657
That clarifies the behavior of referencing resource from child. I will update the patch based on that
SVG:
https://github.com/w3c/webappsec-referrer-policy/issues/108#issuecomment-342135981
That's still uncertain, I will file a follow-up bug so we can keep eye on that.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 18 • 7 years ago

(In reply to David Baron :dbaron: ⌚️UTC-8 (busy week of November 6-10) from 
> 
> I'd note that there are also some cases in SVG where we send an
> incorrect referrer -- at least pre-Stylo.  I have part of a patch to fix
> this in my tree, the current state of which is:
> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> 53f130d100e7/unapplied.pass-cssvalue-url-through
> Implementing Referrer-Policy seems like it might make this more
> important to fix.  (I probably don't have time.)
> 

May I get more details of failure cases so we could test that correctly? Given that the spec of referencing in SVG is still uncertain, do you agree that I move svg things to a separate bug (bug 1415044)?

	James Graham [:jgraham]
	Comment 19 • 7 years ago

There's no general imposition against renaming files, so this is fine. For Gecko in particular you might need to move any corresponding expectation files under testing/web-platform/meta/ and we are not currently good about doing that automatically on import, but that will improve in the future.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 20 • 7 years ago

(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #18)
> (In reply to David Baron :dbaron: ⌚️UTC-8 (busy week of November 6-10) from 
> > 
> > I'd note that there are also some cases in SVG where we send an
> > incorrect referrer -- at least pre-Stylo.  I have part of a patch to fix
> > this in my tree, the current state of which is:
> > https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> > 53f130d100e7/unapplied.pass-cssvalue-url-through
> > Implementing Referrer-Policy seems like it might make this more
> > important to fix.  (I probably don't have time.)
> > 
> 
> May I get more details of failure cases so we could test that correctly?
> Given that the spec of referencing in SVG is still uncertain, do you agree
> that I move svg things to a separate bug (bug 1415044)?

I don't think it's that distinct.  The issue I'm pointing out here is references *from CSS* (but for the CSS properties originally introduced by SVG), not references from the SVG to other SVGs.  I'm OK with punting the issue of what to do about references from one SVG to another, but I think it would be best to fix references from CSS properly in this bug, given that the referrer policy spec makes it more important that we get the referrer right.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 21 • 7 years ago

Comment on attachment 8923687 [details] [diff] [review]
Add more referrer policy css tests.

I should probably cancel this, since I'm waiting for revision of the main patch before reviewing the tests.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 22 • 7 years ago

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #21)
> Comment on attachment 8923687 [details] [diff] [review]
> Add more referrer policy css tests.
> 
> I should probably cancel this, since I'm waiting for revision of the main
> patch before reviewing the tests.

Sorry for my delayed working on this bug. I will continue working on this bug this week based on your information.
Thanks you

	Thomas Nguyen (:tnguyen) Assignee
	Comment 26 • 7 years ago

MozReview-Commit-ID: 4UcUlCgWsxY

	Thomas Nguyen (:tnguyen) Assignee
	Comment 27 • 6 years ago

We pass mozilla::css::URLValue objects instead of nsIURI. URLxtraData in
that object brings referrer and referrer policy so we can send correct Referer
headers.

MozReview-Commit-ID: 8GyUgLyvGDI

	Thomas Nguyen (:tnguyen) Assignee
	Comment 28 • 6 years ago

MozReview-Commit-ID: 35sQ5ZOMRyd

	Thomas Nguyen (:tnguyen) Assignee
	Comment 29 • 6 years ago

MozReview-Commit-ID: CUZ0JFzMkR6

	Thomas Nguyen (:tnguyen) Assignee
	Comment 30 • 6 years ago

(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #29)
> Created attachment 8942871 [details] [diff] [review]
> Add referencing svg from css wpt test
> 
> MozReview-Commit-ID: CUZ0JFzMkR6

I added some wpt tests contain: filter, clip-path, marker(start, mid, end), mask(-image), fill and stroke. The test context is in internal, inline and external styling, PI and presentation attributes. Hi Cameron, am I missing anything in the fixing svg patch and test cases?

	Cameron McCormack (:heycam)
	Comment 31 • 6 years ago

Comment on attachment 8942861 [details] [diff] [review]
Passing URLValue object to svg style system

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

f+ but there a couple of problems with the patch.

I think you need to do something for <mpath xlink:href=""> and <feImage xlink:href=""> too.  (Well, like <textPath xlink:href=""> it is not a CSS feature, so you may or may not want to do that in a separate bug.)

::: layout/painting/nsImageRenderer.cpp
@@ +184,4 @@
>        nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(targetURI), elementId,
> +                                                content->GetUncomposedDoc(), base);
> +
> +      RefPtr<mozilla::css::URLValue> url =

It's probably worth a comment here saying that -moz-element() only ever allows local document references, so we don't really need to provide accurate referrer policy etc. data, but it's required for the nsSVGPaintingProperty API we (now) have.

::: layout/svg/SVGObserverUtils.cpp
@@ -306,2 @@
>        ? SVGObserverUtils::GetFilterURI(aFilteredFrame, i)
> -      : aFilters[i].GetURL()->ResolveLocalRef(aFilteredElement);

We can't drop these ResolveLocalRef calls, since they are what let us handle url(#bare_fragment) values in SVG specially (where they resolve to elements in the document, not resolved against the URL of the style sheet).

@@ +400,4 @@
>      bool hasRef = false;
>      if (maskUri) {
> +      nsCOMPtr<nsIURI> uri = maskUri->GetURI();
> +      uri->GetHasRef(&hasRef);

You can just call HasRef() on the URLValue directly.

@@ +1027,1 @@
>  SVGObserverUtils::GetMarkerURI(nsIFrame* aFrame,

For all of these GetXXXURI methods, since we'll need to maintain something like the ResolveURLUsingLocalRef call, we might need to construct a new URLValue object (if it was indeed a local reference URL).  So I think it will make sense to have their return types be already_AddRefed<css::URLValue>.

@@ +1075,5 @@
>    const nsStyleSVGReset* svgReset = aFrame->StyleSVGReset();
>    MOZ_ASSERT(svgReset->mMask.mLayers.Length() > aIndex);
>  
> +  return static_cast<mozilla::css::URLValue*>(svgReset->mMask.mLayers[aIndex]
> +                                                             .mImage.GetURLValue());

I don't think this cast is correct, since GetURLValue() might return a css::URLValue object or a css::ImageValue object, depending on the nsStyleImage::mType.

::: layout/svg/SVGObserverUtils.h
@@ +461,5 @@
>  class SVGObserverUtils
>  {
>  public:
>    typedef mozilla::dom::Element Element;
> +  typedef nsInterfaceHashtable<nsPtrHashKey<mozilla::css::URLValue>,

I think this should be nsRefPtrHashKey, otherwise we will hold on to a raw URLValue pointer and the URLValue might be destroyed somewhere.

	Cameron McCormack (:heycam)
	Comment 42 • 6 years ago

Comment on attachment 8942862 [details] [diff] [review]
Send correct Referrer header when referencing SVG from CSS

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

::: dom/svg/SVGAnimationElement.cpp
@@ +452,5 @@
>    nsCOMPtr<nsIURI> targetURI;
>    nsCOMPtr<nsIURI> baseURI = GetBaseURI();
>    nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(targetURI),
>                                              aHrefStr, OwnerDoc(), baseURI);
> +  mHrefTarget.Reset(aNodeForContext,

Can SVG SMIL animations target elements in other documents?  I can't remember.  If not, then you could add a similar comment here about needing to pass in referrer information even though we know mHrefTarget won't end up loading an external document.

::: layout/svg/SVGObserverUtils.cpp
@@ +194,5 @@
> +    referrerPolicy = aURI->mExtraData->GetReferrerPolicy();
> +  }
> +
> +  mObservedElementTracker.Reset(aObservingContent, uri, referrer,
> +                                referrerPolicy, true, aReferenceImage);

(I guess another option would be to make IDTracker take a css::URLValue too, although maybe that seems a bit odd as IDTracker is also used for non-CSS things.)

	Thomas Nguyen (:tnguyen) Assignee
	Comment 43 • 6 years ago

(In reply to Cameron McCormack (:heycam) from comment #31)
> Comment on attachment 8942861 [details] [diff] [review]
> Passing URLValue object to svg style system
> 
> Review of attachment 8942861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ but there a couple of problems with the patch.
> 
> I think you need to do something for <mpath xlink:href=""> and <feImage
> xlink:href=""> too.  (Well, like <textPath xlink:href=""> it is not a CSS
> feature, so you may or may not want to do that in a separate bug.)

Yes, fixing the IDTracker.Reset would fix some of the referencing from svg to svg, but not all.
I filed bug 1415044 for that and we can fix the rest in that bug.

> (I guess another option would be to make IDTracker take a css::URLValue too,
> although maybe that seems a bit odd as IDTracker is also used for non-CSS
> things.)

I see that seems odd too. I will keep the current idea and will change if you think adding a new function in IDTracker is a better idea.

Thanks for your feedback.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 44 • 6 years ago

I retake it after 6 months idle. It seems that the patch does not work anymore, because at the moment URLValue is expected to be created from Rust with rust string argument. I talked a bit with Cameron, well... while we could add the Gecko string to the URLValue, but it's a little awkward and there're several things there we will not use.
Since we need to pass referrer and referrer policy through svg style system, I think we could use URLExtraData instead ( or may create a new struct with only referrer + referrer policy)

	Thomas Nguyen (:tnguyen) Assignee
	Comment 45 • 6 years ago

This also fixes loading child stylesheet case, using correct referrer policy stored
in parent sheet.

MozReview-Commit-ID: ARXQyleD9Wq

	Thomas Nguyen (:tnguyen) Assignee
	Comment 46 • 6 years ago

Referrer policy argurment is passed from sheet/doc to URLExtraData, default
value is RP_Unset. We use default value in some cases, particularly when there's
no certain spec talks about that (svg for example)

MozReview-Commit-ID: 5VAX1ZUXD3i

	Thomas Nguyen (:tnguyen) Assignee
	Comment 47 • 6 years ago

MozReview-Commit-ID: DKdVZMOjAQh

	Thomas Nguyen (:tnguyen) Assignee
	Comment 48 • 6 years ago

MozReview-Commit-ID: JA7gkRH9cDJ

	Thomas Nguyen (:tnguyen) Assignee
	Comment 49 • 6 years ago

We create new object URLExtraRefferINfo and pass it to svg system instead of
nsIURI. The object brings referrer and referrer policy so we can send correct
Referer headers.

MozReview-Commit-ID: 2gLnQPEE9t5

	Thomas Nguyen (:tnguyen) Assignee
	Comment 50 • 6 years ago

MozReview-Commit-ID: DAkZ1wjuEe0

	Thomas Nguyen (:tnguyen) Assignee
	Comment 51 • 6 years ago

MozReview-Commit-ID: JHNu8tqGTdi

	Thomas Nguyen (:tnguyen) Assignee
	Comment 52 • 6 years ago

MozReview-Commit-ID: Fl643GY6Jhc

	Thomas Nguyen (:tnguyen) Assignee
	Comment 53 • 6 years ago

MozReview-Commit-ID: EoAXTANJH1T

	Thomas Nguyen (:tnguyen) Assignee
	Comment 54 • 6 years ago

MozReview-Commit-ID: 2SLZ1zqY2Jf

	Thomas Nguyen (:tnguyen) Assignee
	Comment 55 • 6 years ago

MozReview-Commit-ID: H66Qst9skKu

	Cameron McCormack (:heycam)
	Comment 56 • 6 years ago

(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #45)
> This also fixes loading child stylesheet case, using correct referrer policy
> stored in parent sheet.

Can you explain how this works?

	Thomas Nguyen (:tnguyen) Assignee
	Comment 57 • 6 years ago

Thanks you.
Ah, correcting referrer policy from the referrer-policy header could simply fix the case request resource is child sheet loading 
https://searchfox.org/mozilla-central/source/layout/style/Loader.cpp#1527)
But that's not all for other loadings (font face and image), we need further modifications

	Cameron McCormack (:heycam)
	Comment 58 • 6 years ago

I see what you mean, thank you!

	Phabricator Automation
	Comment 59 • 6 years ago

Comment on attachment 8989405 [details]
Bug 1330487 - Part 4: Use correct referrer policy for fontface loader

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1923

	Cameron McCormack (:heycam)
	Comment 60 • 6 years ago

Can you check with your patches whether we are correctly setting the referrer policy for images loaded from CSS generated content?  For example

  <style>
  p::before { content: url(image.png); }
  </style>
  <p></p>

and, now that bug 215083 has landed:

  <style>
  p { content: url(image.png); width: 100px; height: 100px; }
  </style>
  <p></p>

since I'm not sure if they go through the same codepath for image loading as e.g. background-image.

	Phabricator Automation
	Comment 61 • 6 years ago

Comment on attachment 8989410 [details]
Bug 1330487 - Part 5: Use correct referrer policy for image loader

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1924

	Phabricator Automation
	Comment 62 • 6 years ago

Comment on attachment 8989412 [details]
Bug 1330487 - Part 7: Send correct Referrer header when referencing SVG from CSS

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1926

	Thomas Nguyen (:tnguyen) Assignee
	Comment 63 • 6 years ago

MozReview-Commit-ID: LC3jzw8mli7

	Thomas Nguyen (:tnguyen) Assignee
	Comment 64 • 6 years ago

(In reply to Cameron McCormack (:heycam) from comment #60)
> Can you check with your patches whether we are correctly setting the
> referrer policy for images loaded from CSS generated content?  For example
> 
>   <style>
>   p::before { content: url(image.png); }
>   </style>
>   <p></p>
> 
> and, now that bug 215083 has landed:
> 
>   <style>
>   p { content: url(image.png); width: 100px; height: 100px; }
>   </style>
>   <p></p>
> 
> since I'm not sure if they go through the same codepath for image loading as
> e.g. background-image.
There's a test for inline css div.styled {content: url(...). And I added a debug message in https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/layout/style/ImageLoader.cpp#355, it should be the samepath.

	Phabricator Automation
	Comment 65 • 6 years ago

Comment on attachment 8989403 [details]
Bug 1330487 - Part 1: Parse referrer policy from header and propagate to stylesheet

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1921

	Phabricator Automation
	Comment 66 • 6 years ago

Comment on attachment 8994121 [details]
Bug 1330487 - Part 2 : Refactor, new constructor of URIPrincipalReferrerPolicyAndCorsModeHashKey from SheetLoadData

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D2290

	Phabricator Automation
	Comment 67 • 6 years ago

Comment on attachment 8989411 [details]
Bug 1330487 - Part 6: Passing referrer information to svg style system

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1925

	Phabricator Automation
	Comment 68 • 6 years ago

Comment on attachment 8989404 [details]
Bug 1330487 - Part 3: Propagate referrer policy from doc/sheet to URLExtraData

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1922

	Phabricator Automation
	Comment 70 • 6 years ago

Comment on attachment 8989762 [details]
Bug 1330487 - Part 8: Restruct, moving wpt tests to new folder

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1942

	Phabricator Automation
	Comment 71 • 6 years ago

Comment on attachment 8989763 [details]
Bug 1330487 - Part 9: Add referencing svg from css wpt test

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1943

	Phabricator Automation
	Comment 72 • 6 years ago

Comment on attachment 8989764 [details]
Bug 1330487 - Part 10: Add loading child stylesheet wpt tests

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1944

	Phabricator Automation
	Comment 73 • 6 years ago

Comment on attachment 8989766 [details]
Bug 1330487 - Part 11: Add loading font source wpt test

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1945

	Phabricator Automation
	Comment 74 • 6 years ago

Comment on attachment 8995874 [details]
Bug 1330487 - Part 12: Update web-platform tests meta using ---manifest-update

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D2497

	Thomas Nguyen (:tnguyen) Assignee
	Comment 75 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29b451758da8a39079cb6713020e800ea9531776

	Thomas Nguyen (:tnguyen) Assignee
	Comment 76 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&tochange=fa218e33eefc623e73a0abf3986361c34726f6fc&fromchange=a65ee6ac6f57633123a661affd82d87bd06723ef&selectedJob=198362349

	Thomas Nguyen (:tnguyen) Assignee
	Comment 77 • 6 years ago

This bug could be landed after 1486198 (because some tests here easily trigger crash in Moz2DImageRenderer)

	Thomas Nguyen (:tnguyen) Assignee
	Comment 78 • 6 years ago

The crashes have gone after landing 1486198, but try failed at TVW3
https://treeherder.mozilla.org/#/jobs?repo=try&revision=666f850709769b9315dc549621d6cecf5491e188&selectedJob=199447301

The reason is that svg test runs slow (if the test is 0.8*TIME_OUT, the test will be marked as flaky failed)

	Thomas Nguyen (:tnguyen) Assignee
	Comment 79 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=137d7dfa3a4cdd0e1e926f69dddfb3e1a8ebd37a&selectedJob=199460347

	Thomas Nguyen (:tnguyen) Assignee
	Comment 80 • 6 years ago

try looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=137d7dfa3a4cdd0e1e926f69dddfb3e1a8ebd37a&selectedJob=199460347

	Pulsebot
	Comment 81 • 6 years ago

Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7af45723ee38
Part 1: Parse referrer policy from header and propagate to stylesheet r=heycam

	Pulsebot
	Comment 82 • 6 years ago

Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f0e98df4e42f
Part 2 : Refactor, new constructor of URIPrincipalReferrerPolicyAndCorsModeHashKey from SheetLoadData r=heycam
https://hg.mozilla.org/integration/autoland/rev/8ea1aad78f1f
Part 3: Propagate referrer policy from doc/sheet to URLExtraData r=heycam
https://hg.mozilla.org/integration/autoland/rev/f5e373fc941f
Part 4: Use correct referrer policy for fontface loader r=heycam
https://hg.mozilla.org/integration/autoland/rev/a68d1fcf5b77
Part 5: Use correct referrer policy for image loader r=heycam
https://hg.mozilla.org/integration/autoland/rev/a46f689bce75
Part 6: Passing referrer information to svg style system r=heycam
https://hg.mozilla.org/integration/autoland/rev/6bab7a8f441f
Part 7: Send correct Referrer header when referencing SVG from CSS r=heycam
https://hg.mozilla.org/integration/autoland/rev/0f19433e45b7
Part 8: Restruct, moving wpt tests to new folder r=heycam
https://hg.mozilla.org/integration/autoland/rev/b20b9b86b510
Part 9: Add referencing svg from css wpt test r=heycam
https://hg.mozilla.org/integration/autoland/rev/2180d6ea9c6f
Part 10: Add loading child stylesheet wpt tests r=heycam
https://hg.mozilla.org/integration/autoland/rev/221f5b9e37ed
Part 11: Add loading font source wpt test r=heycam
https://hg.mozilla.org/integration/autoland/rev/9332515c425d
Part 12: Update web-platform tests meta using ---manifest-update r=heycam

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 83 • 6 years ago

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13029 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 84 • 6 years ago

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13029
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/429550021?utm_source=github_status&utm_medium=notification)

	Raul Gurzau (:RaulG)
	Comment 85 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7af45723ee38
https://hg.mozilla.org/mozilla-central/rev/f0e98df4e42f
https://hg.mozilla.org/mozilla-central/rev/8ea1aad78f1f
https://hg.mozilla.org/mozilla-central/rev/f5e373fc941f
https://hg.mozilla.org/mozilla-central/rev/a68d1fcf5b77
https://hg.mozilla.org/mozilla-central/rev/a46f689bce75
https://hg.mozilla.org/mozilla-central/rev/6bab7a8f441f
https://hg.mozilla.org/mozilla-central/rev/0f19433e45b7
https://hg.mozilla.org/mozilla-central/rev/b20b9b86b510
https://hg.mozilla.org/mozilla-central/rev/2180d6ea9c6f
https://hg.mozilla.org/mozilla-central/rev/221f5b9e37ed
https://hg.mozilla.org/mozilla-central/rev/9332515c425d

	James Graham [:jgraham]
	Comment 86 • 6 years ago

It seems like some tests failed the upstream stability checks (see [1]).

Do we know if those are existing failures or newly introduced?

[1] https://travis-ci.org/web-platform-tests/wpt/builds/429550021?utm_source=github_status&utm_medium=notification

	Ethan Tseng [:ethan]
	Comment 87 • 6 years ago

(In reply to James Graham [:jgraham] from comment #86)
> It seems like some tests failed the upstream stability checks (see [1]).
> Do we know if those are existing failures or newly introduced?

Thomas or Chris, could you check this?

	Thomas Nguyen (:tnguyen) Assignee
	Comment 88 • 6 years ago

One of them, the test is taking too long > 10000:

Hi James, do you know any try jobs or any equivalent test jobs so that I could reproduce the failures?

	James Graham [:jgraham]
	Comment 89 • 6 years ago

The "too long" failures are all in Chrome; there isn't a job on CI that will run the tests in Chrome (although you can try ./mach wpt --product=chrome --binary=/path/to/chrome -- path/to/test). I think the fact that the time is longer than the supposed timeout is an indication of a bug in that check, so I'm not *super* worried about that.

In Firefox Travis is showing unstable tests, which is more concerning. If these are only existing tests that moved I can force-merge upstream; otherwise we should figure out how to fix them.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 90 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9685fc7d4b76a05f5d644b8badc2a056d0737815&selectedJob=200356861
Running wpt tests 20 times

	Thomas Nguyen (:tnguyen) Assignee
	Comment 91 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9685fc7d4b76a05f5d644b8badc2a056d0737815&selectedJob=200356861
Another running 20 times wpt tests but I don't see any failures.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 92 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2c3ff1c31a79c1b2b34779ff8a25795e406c0b1&selectedJob=200392618
2

	Thomas Nguyen (:tnguyen) Assignee
	Comment 93 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f74bf015805bb691ffd9effc3c04ea452516f63&selectedJob=200449907

	Thomas Nguyen (:tnguyen) Assignee
	Comment 94 • 6 years ago

I tried to run test verification ( I guess that will be similar to travis ci). That test will be run 10 times, then the same test will be run another 5 times, each time in a new browser instance. Once this is done, the whole sequence will be repeated in the test chaos mode 
Running wpt tests again 20 times, and also verification test 20 times and I don't see any failure. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f74bf015805bb691ffd9effc3c04ea452516f63&selectedJob=200449907

	Thomas Nguyen (:tnguyen) Assignee
	Comment 95 • 6 years ago

(In reply to James Graham [:jgraham] from comment #89)
> The "too long" failures are all in Chrome; there isn't a job on CI that will
> run the tests in Chrome (although you can try ./mach wpt --product=chrome
> --binary=/path/to/chrome -- path/to/test). I think the fact that the time is
> longer than the supposed timeout is an indication of a bug in that check, so
> I'm not *super* worried about that.
> 
> In Firefox Travis is showing unstable tests, which is more concerning. If
> these are only existing tests that moved I can force-merge upstream;
> otherwise we should figure out how to fix them.

Thank James for looking at this.
Travis fails in every test we moved. I tried a lot of tests on nightly (you could see try jobs in previous comments) and I don't see any failure. I have no idea why we had such failures
I created a dummy PR with the same commit
https://travis-ci.org/web-platform-tests/wpt/builds/432030764?utm_source=github_status&utm_medium=notification
and it passed. Do you think it is ok if we merge the tests?

The only failure now is in Chrome, ./mach wpt --product=chrome --binary=/path/to/chrome -- path/to/test.
It probably out of topic here but when I try to run the command I get an error:

"Missing hosts file configuration. Run

./wpt make-hosts-file | sudo tee -a /etc/hosts" Do you know about that? It seems be related to https://github.com/web-platform-tests/wpt/issues/13112

	James Graham [:jgraham]
	Comment 96 • 6 years ago

Thanks for the detailed investigation. I'm confident we're not adding new problems, so merging.

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 97 • 6 years ago

Upstream PR merged

	Florian Scholz (Open Web Docs)
	Comment 98 • 6 years ago

I've documented this here https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referrer-Policy#Integration_with_CSS

Does it look good to you?

	Thomas Nguyen (:tnguyen) Assignee
	Comment 99 • 6 years ago

lgtm. Thanks

	Chris Mills [:cmills]
	Comment 100 • 6 years ago

I added a release note covering this:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#Security

Is this OK, or is it behind a pref at the moment?