Closed Bug 1330487 Opened 8 years ago Closed 6 years ago

Implement referrer policy for CSS

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: tnguyen, Assigned: tnguyen)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [domsecurity-active][wptsync upstream])

Attachments

(12 files, 23 obsolete files)

46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
46 bytes, text/x-phabricator-request
heycam
: review+
Details | Review
Assignee: nobody → tnguyen
Whiteboard: [domsecurity-backlog]
Priority: -- → P3
Whiteboard: [domsecurity-backlog] → [domsecurity-backlog1]
Assignee: tnguyen → nobody
Assignee: nobody → tnguyen
Priority: P3 → P2
Status: NEW → ASSIGNED
Summary: Implement referrer policy rules for CSS → Implement referrer policy for CSS
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
MozReview-Commit-ID: 84AoFANESJe
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?
Attachment #8920470 - Flags: feedback?(cam)
(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.
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.
Attachment #8920470 - Flags: feedback?(cam) → feedback+
(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.
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.
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.
Flags: needinfo?(tnguyen)
MozReview-Commit-ID: GZjLrlmoK4C
MozReview-Commit-ID: 84AoFANESJe
Flags: needinfo?(tnguyen)
Attachment #8923687 - Flags: review?(dbaron)
Attachment #8923688 - Flags: review?(dbaron)
(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.
Attachment #8920470 - Attachment is obsolete: true
Attachment #8923678 - Flags: review?(dbaron)
Attachment #8923679 - Flags: review?(dbaron)
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.
Attachment #8923688 - Flags: review?(dbaron) → review-
(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.)
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?
Attachment #8923687 - Flags: feedback?(james)
(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?
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.
See Also: → 1415044
(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)?
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.
Attachment #8923687 - Flags: feedback?(james) → feedback+
Flags: needinfo?(dbaron)
(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.
Flags: needinfo?(dbaron)
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.
Attachment #8923687 - Flags: review?(dbaron)
(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
Attachment #8923688 - Attachment is obsolete: true
MozReview-Commit-ID: 4UcUlCgWsxY
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
MozReview-Commit-ID: 35sQ5ZOMRyd
Attachment #8935672 - Attachment is obsolete: true
Attachment #8942861 - Flags: feedback?(cam)
Attachment #8942862 - Flags: feedback?(cam)
MozReview-Commit-ID: CUZ0JFzMkR6
(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?
Attachment #8923678 - Attachment is obsolete: true
Attachment #8923679 - Attachment is obsolete: true
Attachment #8935669 - Attachment is obsolete: true
Attachment #8935670 - Attachment is obsolete: true
Attachment #8935671 - Attachment is obsolete: true
Attachment #8942871 - Attachment is obsolete: true
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.
Attachment #8942861 - Flags: feedback?(cam) → feedback+
Attachment #8923687 - Attachment is obsolete: true
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.)
Attachment #8942862 - Flags: feedback?(cam) → feedback+
(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.
Assignee: tnguyen → nobody
Status: ASSIGNED → NEW
Assignee: nobody → tnguyen
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)
Attachment #8943158 - Attachment is obsolete: true
Attachment #8943159 - Attachment is obsolete: true
Attachment #8943160 - Attachment is obsolete: true
Attachment #8943161 - Attachment is obsolete: true
Attachment #8943162 - Attachment is obsolete: true
Attachment #8943163 - Attachment is obsolete: true
Attachment #8943164 - Attachment is obsolete: true
Attachment #8943165 - Attachment is obsolete: true
Attachment #8943166 - Attachment is obsolete: true
Attachment #8943167 - Attachment is obsolete: true
This also fixes loading child stylesheet case, using correct referrer policy stored
in parent sheet.

MozReview-Commit-ID: ARXQyleD9Wq
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
MozReview-Commit-ID: DKdVZMOjAQh
MozReview-Commit-ID: JA7gkRH9cDJ
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
MozReview-Commit-ID: JHNu8tqGTdi
MozReview-Commit-ID: Fl643GY6Jhc
MozReview-Commit-ID: EoAXTANJH1T
MozReview-Commit-ID: 2SLZ1zqY2Jf
MozReview-Commit-ID: H66Qst9skKu
(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?
Flags: needinfo?(tnguyen)
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
Flags: needinfo?(tnguyen)
I see what you mean, thank you!
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
Attachment #8989405 - Flags: review+
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.
Flags: needinfo?(tnguyen)
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
Attachment #8989410 - Flags: review+
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
Attachment #8989412 - Flags: review+
(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.
Flags: needinfo?(tnguyen)
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
Attachment #8989403 - Flags: review+
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
Attachment #8994121 - Flags: review+
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
Attachment #8989411 - Flags: review+
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
Attachment #8989404 - Flags: review+
Attachment #8989770 - Attachment is obsolete: true
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
Attachment #8989762 - Flags: review+
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
Attachment #8989763 - Flags: review+
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
Attachment #8989764 - Flags: review+
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
Attachment #8989766 - Flags: review+
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
Attachment #8995874 - Flags: review+
Depends on: 1486198
This bug could be landed after 1486198 (because some tests here easily trigger crash in Moz2DImageRenderer)
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)
Attachment #8942861 - Attachment is obsolete: true
Attachment #8942862 - Attachment is obsolete: true
Attachment #8989404 - Attachment description: Bug 1330487 - Part 2: Propagate referrer policy from doc/sheet to URLExtraData → Bug 1330487 - Part 3: Propagate referrer policy from doc/sheet to URLExtraData
Attachment #8989405 - Attachment description: Bug 1330487 - Part 3: Use correct referrer policy for fontface loader → Bug 1330487 - Part 4: Use correct referrer policy for fontface loader
Attachment #8989405 - Attachment description: Bug 1330487 - Part 4: Use correct referrer policy for fontface loader → Bug 1330487 - Part 3: Use correct referrer policy for fontface loader
Attachment #8989410 - Attachment description: Bug 1330487 - Part 4: Use correct referrer policy for image loader → Bug 1330487 - Part 5: Use correct referrer policy for image loader
Attachment #8989405 - Attachment description: Bug 1330487 - Part 3: Use correct referrer policy for fontface loader → Bug 1330487 - Part 4: Use correct referrer policy for fontface loader
Attachment #8989411 - Attachment description: Bug 1330487 - Part 5: Passing referrer information to svg style system → Bug 1330487 - Part 6: Passing referrer information to svg style system
Attachment #8989412 - Attachment description: Bug 1330487 - Part 6: Send correct Referrer header when referencing SVG from CSS → Bug 1330487 - Part 7: Send correct Referrer header when referencing SVG from CSS
Attachment #8989762 - Attachment description: Bug 1330487 - Part 7: Restruct, moving wpt tests to new folder → Bug 1330487 - Part 8: Restruct, moving wpt tests to new folder
Attachment #8989763 - Attachment description: Bug 1330487 - Part 8: Add referencing svg from css wpt test → Bug 1330487 - Part 9: Add referencing svg from css wpt test
Attachment #8989764 - Attachment description: Bug 1330487 - Part 9: Add loading child stylesheet wpt tests → Bug 1330487 - Part 10: Add loading child stylesheet wpt tests
Attachment #8989766 - Attachment description: Bug 1330487 - Part 10: Add loading font source wpt test → Bug 1330487 - Part 11: Add loading font source wpt test
Keywords: checkin-needed
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
Keywords: checkin-needed
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13029 for changes under testing/web-platform/tests
Whiteboard: [domsecurity-active] → [domsecurity-active][wptsync upstream]
Whiteboard: [domsecurity-active][wptsync upstream] → [domsecurity-active][wptsync upstream error]
Whiteboard: [domsecurity-active][wptsync upstream error] → [domsecurity-active][wptsync upstream]
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
(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?
Flags: needinfo?(tnguyen)
Flags: needinfo?(ckerschb)
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?
Flags: needinfo?(tnguyen)
Flags: needinfo?(james)
Flags: needinfo?(ckerschb)
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.
Flags: needinfo?(james)
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
(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
Flags: needinfo?(james)
Thanks for the detailed investigation. I'm confident we're not adding new problems, so merging.
Flags: needinfo?(james)
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?
Flags: needinfo?(tnguyen)
lgtm. Thanks
Flags: needinfo?(tnguyen)
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?

(In reply to Pulsebot from comment #82)

Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a46f689bce75
Part 6: Passing referrer information to svg style system r=heycam

This caused bug 1550531.

The change from using nsURIHashKey to using nsRefPtrHashKey here:

https://hg.mozilla.org/mozilla-central/rev/a46f689bce75692d05843dd390018bae6e57860f#l4.163

meant that when checking whether a given URLAndReferrerInfo was already in a hash table we would compare the URLAndReferrerInfo's address instead of checking for URL/value equality here:

https://hg.mozilla.org/mozilla-central/file/a46f689bce75692d05843dd390018bae6e57860f/layout/svg/SVGObserverUtils.cpp#l602

Since consumers of GetPaintingPropertyForURI generate a new URLAndReferrerInfo to pass each time, we would then never match and would allocate a new nsSVGPaintingProperty and add it to the table each time. This memory leak appears to have gone unnoticed until the frontend code started using -moz-element with the landing of:

https://bugzilla.mozilla.org/show_bug.cgi?id=1540984#c31

I think this only happens (the -moz-element gets used) when the user has a theme with a large image banner installed. E.g. https://addons.mozilla.org/en-US/firefox/addon/firefox-quantum-nightly/

Thomas, you probably want to audit the other patches for unintentional hashing changes that may have resulted in similar issues.

Flags: needinfo?(tnguyen)
Regressions: 1550531

Hashing change is only used in the patch you pointed out (and you fixed it). Thanks for the fix.

Flags: needinfo?(tnguyen)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: