Setting `svg.disabled` to `true` causes extension icons to disappear
Categories
(Core :: SVG, defect, P3)
Tracking
()
People
(Reporter: gk, Assigned: dholbert)
References
Details
(Whiteboard: [tor 31015])
Attachments
(3 files)
222.83 KB,
image/png
|
Details | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
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.
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
•
|
||
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.
Assignee | ||
Comment 2•5 years ago
|
||
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
Assignee | ||
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
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).
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 6•5 years ago
|
||
Assignee | ||
Comment 7•5 years ago
|
||
I tested the attached patch locally with uBlock origin, FWIW, and it seems to work.
Comment 8•5 years ago
|
||
Do we want just addonPolicy check or isAddonOrExpandedAddonPrincipal ?
See for example what bug 1453916 did.
But perhaps addonPolicy is enough for now.
Updated•5 years ago
|
Assignee | ||
Comment 10•5 years ago
•
|
||
(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.
Assignee | ||
Comment 11•5 years ago
•
|
||
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
Assignee | ||
Comment 12•5 years ago
|
||
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•5 years ago
|
||
Pushed by dholbert@mozilla.com: 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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9e6dada64af0
https://hg.mozilla.org/mozilla-central/rev/dd4671ce4c73
Assignee | ||
Comment 15•5 years ago
•
|
||
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
Assignee | ||
Updated•5 years ago
|
Comment 16•5 years ago
|
||
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.
Updated•5 years ago
|
Reporter | ||
Comment 17•5 years ago
|
||
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!
Assignee | ||
Comment 18•5 years ago
|
||
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•5 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr68/rev/db12384577c9
https://hg.mozilla.org/releases/mozilla-esr68/rev/c7a79bce7bcd
Updated•5 years ago
|
Description
•