1362120 - Disabling gfx.font_rendering.opentype_svg.enabled breaks image context paint

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[u589863]

User Agent: Mozilla/5.0 (Windows NT 10.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.97 Safari/537.36 Vivaldi/1.10.834.9

Steps to reproduce:

disable these

gfx.font_rendering.opentype_svg.enabled

svg.disabled


Actual results:

toolbar icon breaks
setting gfx.font_rendering.opentype_svg.enabled false
for rendering hw/software/security reasons,
breaks this and turns it into black.
can it be fixed?


Expected results:

Should not break icons,
and yes won't enable those preferences as for testing/hw/software/security reasons.

Comment 1

[u589863]

it allows fingerprinting, CVE-2016-9079(fixed),
disabling the preference help in rendering fonts correctly/better(just like woff disabling helps sometimes and more security enhance) on some hardware.

Comment 2

[Darkspirit]

Wouldn't it be better here to restrict those prefs only to the web, but not to the browser ui?

Comment 3

[u589863]

(In reply to Darkspirit from comment #2)
> Wouldn't it be better here to restrict those prefs only to the web, but not
> to the browser ui?

My question here, though, is why it has any relation to this bug.the patch here added some SVG images (not an OpenType-SVG font) just like bug 759252, so that pref shouldn't have any effect on it. If it's confirmed that this pref breaks the rendering of SVG images in general, that sounds like something that needs debugging. (I'm sure it didn't originally!)

Comment 4

[u589863]

I'd be inclined to keep it around, as opentype-svg support is still a relatively little-used and therefore probably less-tested feature; though IIUC support has now been added in Edge, which may lead to wider adoption in future

Comment 5

[u589863]

Attachment: d97y3va.png

I've just reproduced this bug in a new profile:
Try to add new bookmark by clicking on a star. Animation is bugged. I haven't found a bug on bugzilla so maybe this is the bug which caused it

Comment 6

[u589863]

tried disabling animations, still animates!!

Comment 7

[Jonathan Kew [:jfkthame]]

Mefoster: please don't set a bunch of needinfo flags on people without good reason (e.g. a specific question that you have reason to believe that person can answer). It just leads to more clutter (in the bug and in people's email).

You've already copied my comments from bug 759252 over here in comment 3 and comment 4 above (although they're a bit confusing without the context of what they were replying to), so I don't really have anything useful to add at this point.

Comment 8

[Frederik Braun [:freddy]]

Mefoster, your behavior is unacceptable.
Please be mindful of other people's time and stick to the Bugzilla etiquette as explained at <https://bugzilla.mozilla.org/page.cgi?id=etiquette.html>.

Please stop this kind of activity.

Comment 9

[Justin Dolske [:Dolske]]

This bug report is very unclear. Attempting to clean it up.

The specific problem being described here appears to be that when gfx.font_rendering.opentype_svg.enabled is set to false (not the default), the bookmark star is black instead of blue. I can reproduce this on OS X with a current Nightly by flipping this one pref, restarting, and then bookmarking a page.

As far as I can tell the svg.enabled pref is entirely irrelevant here. Nor do I see any serious "breakage" -- this one icon is simply the wrong color.

Comment 10

[Justin Dolske [:Dolske]]

This rule (in browser/themes/shared/toolbarbutton-icons.inc.css) isn't working right with this pref enabled:

toolbar:not([brighttext]) #bookmarks-menu-button[cui-areatype="toolbar"][starred] > .toolbarbutton-menubutton-button {
  -moz-context-properties: fill;
  fill: var(--toolbarbutton-icon-fill-attention);
}

DevTools says the rule has applied, so presumably there's some interaction with the pref and -moz-context-properties / fill?

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: patch - always enable SVG-in-OpenType in the parent process

We could pass through information about whether the consumer is "chrome", but since e10s is the future and flipping this pref is rare I'm not keen on adding a parameter to the necessary functions to do that. We can reconsider if necessary. This patch at least fixes things when running with e10s active.

Comment 12

[Jonathan Watt [:jwatt]]

The change that fixes this bug is the CSS parser change. The gfxPlatform change is for consistency.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8864823 [details] [diff] [review]
patch - always enable SVG-in-OpenType in the parent process

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

::: gfx/thebes/gfxPlatform.cpp
@@ +1616,5 @@
>      }
>  
> +    return mOpenTypeSVGEnabled > 0 ||
> +           // is e10s parent process:
> +           (XRE_IsParentProcess() && BrowserTabsRemoteAutostart());

(It might be nice to have a function that encapsulates this check with a more obvious name, like IsE10sParentProcess or something, since you write it here and in nsCSSParser.cpp, and I see it in at least one other place in the codebase too.)

Comment 14

[Cameron McCormack (:heycam)]

It also might be nice to have separate prefs for the keywords and for OpenType-in-SVG itself, but that's not super important.

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Cameron McCormack (:heycam) from comment #13)
> > +           // is e10s parent process:
> > +           (XRE_IsParentProcess() && BrowserTabsRemoteAutostart());
> 
> (It might be nice to have a function that encapsulates this check with a
> more obvious name, like IsE10sParentProcess or something

I filed bug 1362891.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9b7b29f956
Always enable SVG-in-OpenType in the parent process. r=heycam

Comment 17

[(no longer active)]

Sorry, this caused build failures so I backed it out:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bd9b7b29f95690ab9c3aacd7864206c68509fa8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6086ceba2e8c787768266479d694806725e90add

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50273abf9e11
Always enable SVG-in-OpenType in the parent process. r=heycam

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out for failures like 

https://treeherder.mozilla.org/logviewer.html#?job_id=97251922&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=97251951&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=97251947&repo=mozilla-inbound
and
https://treeherder.mozilla.org/logviewer.html#?job_id=97251920&repo=mozilla-inbound&lineNumber=5498

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4e9e9eab87d
Backed out changeset 50273abf9e11 for causing various reftest and browser chrome failures in different tests

Comment 21

[Jonathan Watt [:jwatt]]

(It's the nsCSSParser change that somehow results in the failures.)

Comment 22

[u589863]

build 14-05-1017 fixed this !

Comment 23

[Jonathan Watt [:jwatt]]

The patch for this got backed out, and testing '55.0a1 (2017-05-18) (64-bit)' shows this is not fixed. Maybe your profile lost the gfx.font_rendering.opentype_svg.enabled setting? At any rate, this still needs work.

Comment 24

[Jonathan Watt [:jwatt]]

The reason image context paint fails when the pref is disabled is because in CSSParserImpl::ParsePaint we parse for the variant VARIANT_OPENTYPE_SVG_KEYWORD, which means that we parse for the variant VARIANT_KEYWORD only if the pref is enabled. SVG 2 adds the 'context-fill' and 'context-stroke' keywords to <paint>, so we should now parse for the variant VARIANT_KEYWORD regardless of the pref value.

https://svgwg.org/svg2-draft/single-page.html#painting-SpecifyingPaint

Comment 25

[Jonathan Watt [:jwatt]]

Attachment: patch - Always allow the 'context-fill'/'context-stroke' keywords in SVG <paint>

Comment 26

[Cameron McCormack (:heycam)]

Comment on attachment 8873273 [details] [diff] [review]
patch - Always allow the 'context-fill'/'context-stroke' keywords in SVG <paint>

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

This makes me realize it's a bit confusing that VARIANT_OPENTYPE_SVG_KEYWORD just means "VARIANT_KEYWORD but only if the OT-in-SVG pref is set".  (I thought it should mean "parse these specific keywords if the pref is set".)

Comment 27

[Jonathan Watt [:jwatt]]

Indeed. Something like VARIANT_KEYWORD_IF_SVG_OT_ENABLED might be a better name.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/920a7465a372
Always allow the 'context-fill'/'context-stroke' keywords in SVG <paint>. r=heycam

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11c66c13481d
Update Stylo test expectations.

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/920a7465a372
https://hg.mozilla.org/mozilla-central/rev/11c66c13481d