1359762 - Support context paint for the video control buttons and Reader Mode buttons

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

It seems like bug 1359486 wasn't enough to support context paint in all non-Web content. In bug 1359631 the patches for the video play/pause buttons and for the Reader Mode buttons don't work. The video controls are not IsChrome() because they're an XBL binding. The Reader Mode document isn't IsChrome() since it's an about: document.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

I kind of feel like I'm playing whack-a-mole here. I can't be the first person to have needed to have an "IsNotWebContent(nsIContent*)" function of sorts, but I'm not seeing anything.

Boris, you seem like the person who is most likely to know if we have such a thing somewhere. Any thoughts?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I am not aware of any such thing, largely because the definition of "web content" is not simple.

The attached patch is definitely wrong, because the base URI of a document is under control of that document; nothing prevents a web page from having <base href="about:blank"> in it.

So what, exactly, is our definition of "web content" here, for purposes of context paint?  For a start, why is context paint disabled for "web content"?

Comment 3

[Jonathan Watt [:jwatt]]

Context paint broadly refers to our support for the 'context-fill'/'context-stroke' values that 'fill'/'stroke' can be set to in order to have an element take color from another element that referenced it. For example, the contents of a <marker> can take color from the path being marked.

In this context the definition is narrower. To help the Firefox frontend team with various issues I extended our context paint code beyond what the specifications define to allow SVG images (not documents) to take color from the element that referenced the SVG image (so from an <img>, an element with 'background-image', etc.). It is specifically this extension that is the subject of this bug.

Let's call it "image context paint" to disambiguate.

Since image context paint is not in any spec, and since I'm not entirely happy that the mechanism is a particularly good one, I do not want "web content" to use it or come to rely on it.

Ideally I want to allow any files that ship with Firefox to use image context paint, but I don't want WebExtensions authors or any content that is loaded from an external source like the web to be able to use it. Maybe that's not practical, and maybe I don't need to be concerned about being so strict. Simply blocking it from http://, https://, file:// and about:blank may be enough to prevent any external use that we'd need to worry about.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK. So...

First of all, some about: URIs are actually loaded from the web (via https://).  So just blocking by protocol is probably not sufficient if you really want to blacklist https-delivered stuff.

Here's a question.  It looks like we currently base the check on the linking element, not the linked-to image.  Would it be a problem to base it on the linked-to image?  As in, if you're loading a chrome:// or resource:// image, we allow that image to use the linking element's color; otherwise we do not.

Yes, if a web page links to a chrome:// image it would be able to rely on this feature.  But then it's also relying on that chrome:// image being around, so it's already quite Firefox-specific.

Comment 5

[Jonathan Watt [:jwatt]]

Fun fact: browser/ and toolkit/ now contain over 200 svg icons.

As best I can tell, restricting based on the scheme of the linked-to image would allow all "our" images to use image context paint, except those in our in-tree WebExtensions (moz-extension://). I don't think there's currently any need to access context paint from WebExtensions icons though, so we can cross that bridge if/when we come to it. (John-Galt tells me that once some combination of bug 1267027, bug 615708 and bug 1281571 (or something like that) are fixed we can probably make the "this linking node came from one of our extensions" info available via the linking element's nsIPrincipal.)

So yeah, let's restrict image context paint to chrome:// and resource:// images. It's not perfect but seems like the best approach so far.

(Regarding my patch, yes, I should have blacklisted about:blank and I should have been calling GetOriginalURI(), not GetDocBaseURI(). I'm not aware of any other about: URLs being an issue though given none of them are same origin.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I'm not aware of any other about: URLs being an issue though given none of them are same origin.

Not sure what that means, but it's probably not relevant if we switch to checking the URL of the image.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Expose imgIContainer's URI

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Support image context paint for more of our SVG UI icons

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8862240 [details] [diff] [review]
part 2 - Support image context paint for more of our SVG UI icons

>+    if (!((NS_SUCCEEDED(imageURI->SchemeIs("chrome", &match)) && match) ||
>+          (NS_SUCCEEDED(imageURI->SchemeIs("resource", &match)) && match))) {

Fwiw, we have a bunch of copies of this now: in nsSyncLoadService::LoadDocument and in nsXBLContentSink::OnOpenContainer and in nsXBLService::IsChromeOrResourceURI and ComputeImageFlags in image/ImageFactory.cpp and in nsPresContext::AttachShell and this new code.

Might be worth factoring this out into an nsContentUtils method, as a followup.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8862240 [details] [diff] [review]
part 2 - Support image context paint for more of our SVG UI icons

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

It seems like one possible alternative would be to pass in the context-paint regardless of the image's URI (i.e. so we don't need to expose the image URI), and then only letting the image *use* the context-paint if the image is of the right URI (which we only need to bother checking inside of SVG/VectorImage code wherever we use context-paint).

That has the benefit of keeping imgIContainer.idl a bit shorter/cleaner, and avoiding the need to mess with non-SVG imgIContainer implementations here.

Is there a reason you went with the strategy you chose here instead of that strategy?

::: layout/svg/SVGImageContext.cpp
@@ +35,5 @@
> +    // easy means to determine whether aFromFrame is a frame for a content
> +    // node that originated from Web content.  Unfortunately different types
> +    // of anonymous content, about: documents such as about:reader, etc. that
> +    // is "ours" are sometimes hard to distinguish from real Web content.  As a
> +    // instead of trying to figure out what content is "ours" here we instead

"As a instead of" --> needs some wordsmithing

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> It seems like one possible alternative would be to pass in the context-paint
> regardless of the image's URI (i.e. so we don't need to expose the image
> URI),

In particular -- once we support a "context-properties" (-moz-context-properties?) CSS property as a means of whitelisting (assuming that'll be ready before we ship this in a release), we could still have the default codepath be fast here, and not bother storing anything.

I'm imagining that, for <img src="whatever" style="context-properties:fill">, we would:
 - enter MaybeStoreContextPaint (as we already do), and:
   - bail immediately if "whatever" is raster-typed
   - otherwise, store "fill" (honoring "context-properties")

And then just before VectorImage.cpp makes its "autoContextPaint.emplace" call, we'd inspect our URI for "chrome"/"resource".  What do you think?

(To me, this seems equivalently-efficient, and somewhat cleaner, as compared to what you've got right now, because it doesn't involve growing a new method on imgIContainer & modifying all the other image subclasses [whose implementations of that new method would be currently untested/unused basically].  And the separation of concerns makes sense to me too -- the "host" makes its decision about how much (if anything) to pass in, and then the "guest" image makes its separate decision about whether it's allowed to care about that based on its origin.

If we need to get this working before we've got support for the "context-properties" whitelisting mechanism, we could just behave as if all the properties that we might care about are always whitelisted (which I think is just fill/stroke) -- and that'd make us slightly less efficient at rendering SVG images in the interim, because MaybeStoreContextPaint would always have some extra context-storing-churn for them.  But that doesn't concern me supermuch as long as we'll add the whitelisting mechanism before this ships in a release.)

Comment 12

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> In particular -- once we support a "context-properties"
> (-moz-context-properties?) CSS property as a means of whitelisting (assuming
> that'll be ready before we ship this in a release), we could still have the
> default codepath be fast here, and not bother storing anything.

Indeed, it was the cost I was thinking about here, and I also concluded that 'context-properties' should really be implemented to avoid that cost if we do things the way you suggest.

> I'm imagining that, for <img src="whatever"
> style="context-properties:fill">, we would:
>  - enter MaybeStoreContextPaint (as we already do), and:
>    - bail immediately if "whatever" is raster-typed
>    - otherwise, store "fill" (honoring "context-properties")
> 
> And then just before VectorImage.cpp makes its "autoContextPaint.emplace"
> call, we'd inspect our URI for "chrome"/"resource".  What do you think?

Right.

> (To me, this seems equivalently-efficient, and somewhat cleaner, as compared
> to what you've got right now, because it doesn't involve growing a new
> method on imgIContainer & modifying all the other image subclasses [whose
> implementations of that new method would be currently untested/unused
> basically].

It seemed like a useful method to me and, since VectorImage can be wrapped in a ClippedImage, FrozenImage or OrientedImage (maybe also MultipartImage?), the method would be used and a tested path for them too. The method for RasterImage would also be tested (if unused for actual RasterImage instances), since it's shared with VectorImage. So it's only DynamicImage I think that would be untested and unused.

> And the separation of concerns makes sense to me too -- the
> "host" makes its decision about how much (if anything) to pass in, and then
> the "guest" image makes its separate decision about whether it's allowed to
> care about that based on its origin.

I agree with that.

I'll look at implementing 'context-properties'.

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: patch - require chrome:// or resource:// images to support SVG image context paint

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8862951 [details] [diff] [review]
patch - require chrome:// or resource:// images to support SVG image context paint

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

r=me with nits addressed.

Commit message nit:
>  Bug 1359762 - Require chrome:// or resource:// images to support SVG image context paint. r=dholbert

This is a bit ambiguous -- it sounds like you're saying "Require that these images must support context paint", when I think you mean to say "Require these schemes, in order for images to support context paint".

Perhaps reword to a less-ambiguous wording like:
  Only allow SVG images to use context paint if they're from chrome:// or resource://.

::: layout/svg/SVGContextPaint.cpp
@@ +29,5 @@
> +                                 "svg.context-properties.content.enabled", false);
> +    sEnabledForContentCached = true;
> +  }
> +
> +  if (!sEnabledForContent) {

This "if" nesting made some sense in the previous home for this code, but it makes less sense here.

Let's just make this an early "return true" to save on indentation & to improve debuggability (adding a place where we can add a breakpoint to follow code flow):

  if (sEnabledForContent {
    return true;
  }

And then this function's final "return true" clause will be specifically for when we've detected a chrome:// or resource:// image.

@@ +54,5 @@
> +    // https://bugzilla.mozilla.org/show_bug.cgi?id=1359762#c5
> +    //
> +    bool match;
> +    if (!((NS_SUCCEEDED(aURI->SchemeIs("chrome", &match)) && match) ||
> +          (NS_SUCCEEDED(aURI->SchemeIs("resource", &match)) && match))) {

I suspect it would be faster & perhaps more readable to make a single call to GetScheme here (passing in a local nsAutoCString), and then checking that against our whitelisted schemes.

Something like:
====
  nsAutoCString scheme;
  if (NS_SUCCEEDED(aURI->GetScheme(scheme)) &&
      (scheme.EqualsLiteral("chrome") || EqualsLiteral("resource"))) {
    return true;
  }

  return false;
====

This involves one fewer nsIURI virtual function call, one fewer "success" check, and the logic is a bit easier to follow IMO.  (I'm assuming GetScheme will be pretty fast -- it should only need to write a few characters to stack memory in this case, so I suspect it is indeed fast.)

This is fine as-is too, though.

::: layout/svg/SVGContextPaint.h
@@ +113,5 @@
>                             "subclass");
>      return 0;
>    }
>  
> +  static bool IsAllowedForImageFromURI(nsIURI* aURI);

Please add at least a line of documentation for what this image does.

In particular, when I read this method's name, it's not clear to me whether the method is asking...
(A) "Is context-paint allowed to be used in image requests that *are made from* a page at this URI"?
...vs:
(B) "Is context-paint allowed to be used in an image that has the given URI"?

(I believe (B) is the correct interpretation, so the comment should make that clear.)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de8bbf1eb200
Only allow SVG images to use context paint if they're from chrome:// or resource://. r=dholbert

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing reftest context-fill-01.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d2edaa9631613a4d4b57d49f26bedca67e5f5b9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=de8bbf1eb20017db722ba77dbe3e187a4bbe359b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=95310838&repo=mozilla-inbound

> [task 2017-04-28T20:12:35.778117Z] 20:12:35     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/svg/as-image/context-fill-01.html == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/svg/as-image/lime100x100-ref.html | image comparison, max difference: 255, number of differing pixels: 10000

Comment 17

[Jonathan Watt [:jwatt]]

So this is interesting. The *-01 and *-02 tests are different from the others in that they are loaded once with the pref off, then a second time with the pref on.

One of the features of this patch is that we now create and store the SVGContextPaint in the key for an image's entry in the image cache regardless of whether the SVGContextPaint will be allowed to be used or not. Previously, if the pref allowed the SVGContextPaint to be used we would use it as part of the key, and if the pref didn't allow it to be used we wouldn't use it as part of the key. Because this patch makes us always include the SVGContextPaint in the key then we will always fetch the first rendering out of the cache regardless of whether the pref changes.

It would be nice to keep the pref live, but I don't see how we can do that with the approach taken by this patch.

Comment 18

[Daniel Holbert [:dholbert]]

I see, yeah.

So with the patch right now, VectorImage::Draw() is only letting this "IsAllowedForImageFromURI()" condition control the "Maybe<AutoSetRestoreSVGContextPaint> autoContextPaint" local RAII variable.  But really, we need to make it control the contents of our SurfaceCache key, too, since it absolutely affects our rendering.

So I think maybe in the "disallowed" scenario, we need to modify |params| to stomp on its svgContext's context-paint member-variable, BEFORE we call LookupCachedSurface(params).  (Possibly even stomping on the context-paint member of a local helper SVGImageContext that we set up separately, to split up the logic a bit, which we then pass in to the params(...) constructor.)

SO: you'll want to check SVGContextPaint::IsAllowedForImageFromURI(uri) a bit earlier, and store its result in a local bool, so that you can benefit from it at that point (before the LookupCachedSurface call) as well as later on before the autoContextPaint.emplace() call where you're already checking it.

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: patch - Only allow SVG images to use context paint if they're from chrome:// or resource://

I think that works, thanks. How about this?

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8865694 [details] [diff] [review]
patch - Only allow SVG images to use context paint if they're from chrome:// or resource://

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

I think this looks good! r=me with one comment nit:

::: image/VectorImage.cpp
@@ +842,5 @@
> +    }
> +
> +    if (blockContextPaint) {
> +      // The SVGImageContext must not include context paint if the image is
> +      // not going to use it:

s/not going to/not allowed to/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d63a1c18cf81
Only allow SVG images to use context paint if they're from chrome:// or resource://. r=dholbert

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d63a1c18cf81