1564208 - Setting `svg.disabled` to `true` causes extension icons to disappear

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Georg Koppen]

We got a bug report over in https://trac.torproject.org/projects/tor/ticket/31015 stating that setting svg.disabled to true causes extension UI buttons to disappear (in this case for uBlock Origin and uMatrix).

Optionally disabling SVG is not meant for browser chrome or extension code but rather for SVG content loaded by websites. Thus, I suspect this is a bug in the original patch that landed in bug 1216893.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screenshot of bug

Here's a screenshot of what this bug looks like, in latest Firefox Nightly with svg.disabled = true.

In the upper squarish white area of uBlock Origin's panel in this screenshot, there's supposed to be a power-button icon with four smaller icons below it. And at the very bottom in the gray area with "2" and "18", there are supposed to be 5 more icons.

Comment 2

[Daniel Holbert [:dholbert]]

I suspect the relevant logic is here inside of bool nsNodeInfoManager::InternalSVGEnabled() {:
https://searchfox.org/mozilla-central/rev/da3f3eaaacb6fb344fd21ac29ace2da0e33f12d3/dom/base/nsNodeInfoManager.cpp#337-347

Comment 3

[Daniel Holbert [:dholbert]]

Yeah, if I force that function to end up with conclusion = true (e.g. in my debugger) then I get the expected rendering. So the conclusion logic probably needs a bit more subtlety to allow SVG in "trusted" web-extension content.

Also, for the record: in this case with uBlock origin, the popup itself is just an HTML document with URI moz-extension://[some-long-random-UUID]/popup.html, and the SVG content is directly included as inline nodes in the HTML of that page.

Comment 4

[Daniel Holbert [:dholbert]]

zombie mentioned on IRC that we can test mPrincipal for webextension-ness by checking this expression for truthiness:

 BasePrincipal::Cast(mPrincipal)->AddonPolicy()

We may want to extend the LoadingPrincipal and TriggeringPrincipal cases as well (might be required in order for images/embeds in WebExtension panels to work properly, particularly if they're not moz-extension:// URIs themselves).

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a task, but please change it back in case of error.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1564208 part 1: Allow extensions to use SVG content regardless of whether the svg.disabled pref is toggled. r?smaug

Comment 7

[Daniel Holbert [:dholbert]]

I tested the attached patch locally with uBlock origin, FWIW, and it seems to work.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Do we want just addonPolicy check or isAddonOrExpandedAddonPrincipal ?
See for example what bug 1453916 did.
But perhaps addonPolicy is enough for now.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 1564208 part 2: Use Maybe<bool> to represent lazily-initialized bools for SVG/MathML enabling. r?smaug

Depends on D37461

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Olli Pettay [:smaug] from comment #8)

Do we want just addonPolicy check or isAddonOrExpandedAddonPrincipal ?
See for example what bug 1453916 did.

I don't know enough about addons or expanded principals to know for sure what sort of scenario would require that more lenient (I think?) check.

I did test Firefox Screenshots (which was the use case in bug 1453916), and I determined that the current patch is fine for that use-case at least. Specifically, Firefox Screenshots does have some SVG icons which indeed do not show up in its UI in current Nightly with svg.disabled=false, and the patch as it stands right now makes those icons start showing up. (So: no need to swap to isAddonOrExpandedAddonPrincipal to make that work.)

But perhaps addonPolicy is enough for now.

Unless you think there's a compelling case where we need a isAddonOrExpandedAddonPrincipal check, then yeah, I'm leaning towards sticking with the addonPolicy check.

Comment 11

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9178a40128a8cb7b104daa842c691b505354a693
Edit: I forgot to activate testruns in the try-fuzzy UI. New Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58d5ef4651ac57bdc1e5404b89d61702935c183e

Comment 12

[Daniel Holbert [:dholbert]]

Assuming this sticks, we should probably request uplift to the ESR68 (which is being released this week -- I don't imagine this patch will make the first esr68 release but it could make 68.01 or whatever)

This is pretty low-risk from a Firefox perspective, since it's a small patch that just tweaks an off-by-default feature that's only enabled in TorBrowser as far as we know (not exposed in the Firefox UI anywhere).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e6dada64af0
part 1: Allow extensions to use SVG content regardless of whether the svg.disabled pref is toggled. r=smaug
https://hg.mozilla.org/integration/autoland/rev/dd4671ce4c73
part 2: Use Maybe<bool> to represent lazily-initialized bools for SVG/MathML enabling. r=smaug

Comment 14

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/9e6dada64af0
https://hg.mozilla.org/mozilla-central/rev/dd4671ce4c73

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 9076890 [details]
Bug 1564208 part 1: Allow extensions to use SVG content regardless of whether the svg.disabled pref is toggled. r?smaug

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a fixing bug in a feature that's used by Tor Browser (which is based off of ESR). The feature isn't exposed in Firefox (users would have to manually go to about:config and toggle the pref in order to activate this feature i.e. to disable SVG content), so this change is highly unlikely to affect many folks beyond TorBrowser users.

• User impact if declined: Broken extension UI (missing icons) in e.g. uBlock Origin, for TorBrowser users who've dialed up their security settings (which sets this pref svg.disabled = true under the hood). See screenshot in comment 1 for an example.

• Fix Landed on Version: 70

• Risk to taking this patch: Low

• Why is the change risky/not risky? (and alternatives if risky): Not risky because:
  (a) It's a very targeted fix
  (b) ...to allow SVG in one additional "trusted" context (extensions)
  (c) ...and SVG is default-allowed anyway, except when this pref is flipped (mainly in special TorBrowser configurations)
  (d) ...and the pref only intends to turn off SVG in web content, so this makes the setting better-match the pref's intent.

• String or UUID changes made by this patch: None

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9076890 [details]
Bug 1564208 part 1: Allow extensions to use SVG content regardless of whether the svg.disabled pref is toggled. r?smaug

Fixes a bug affecting Tor browser which is non-default for Firefox. Approved for 68.1esr.

Comment 17

[Georg Koppen]

Daniel: I might be a bit late with this but still: many thanks from our side to get this issue fixed so quickly and backported to esr68. That's been really impressive and very kind!

Comment 18

[Daniel Holbert [:dholbert]]

You're welcome! Thanks for the bug report. I'm glad it was a straightforward fix. :)

(BTW, I don't think this is actually fixed on the ESR68 branch yet, but it'll be there soon I think; it's got approval and someone will land all the approved bugs before long.)

Comment 19

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/db12384577c9
https://hg.mozilla.org/releases/mozilla-esr68/rev/c7a79bce7bcd