Avoid exposing :-moz-system-metric stuff in content pages.

RESOLVED FIXED

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

({dev-doc-complete, site-compat})

unspecified
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 affected)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
See also bug 1396048.

We've been without them for a while in stylo and no bug reports so far.

Code-search in github also returns no results for it, and it looks like a fingerprinting vector (see bug 541386).

We should consider unshipping it from content pages after 57.
(Assignee)

Comment 1

2 years ago
I sent https://groups.google.com/forum/#!topic/mozilla.dev.platform/RVttfrQkXLU, let's use that thread if discussion is needed.
Assignee: nobody → emilio
(Assignee)

Updated

2 years ago
See Also: → bug 1396073
Keywords: dev-doc-needed, site-compat
Priority: -- → P3
status-firefox55: --- → wontfix
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox-esr52: --- → wontfix
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1405311
I don't think your intent to unship email in comment 1 covers patches after the first one. Could you send another intent to unship mentioning all of them?
Flags: needinfo?(emilio)
Oh, actually, I guess that one probably intended to cover all the media queries although didn't name all of them explicitly.

-moz-is-glyph isn't a system metric anyway...
(Assignee)

Comment 9

2 years ago
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8)
> Oh, actually, I guess that one probably intended to cover all the media
> queries although didn't name all of them explicitly.

Right, I posted separate patches for the ones that don't explicitly use `GetSystemMetric`, but I think they all fall in the same category.

> -moz-is-glyph isn't a system metric anyway...

That's true, but there was a comment (falsely) claiming it was internal, so I just assumed this was overlooked and fixed it. Let me know if you really want another email.
Flags: needinfo?(emilio)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8914736 [details]
Bug 1396066: Restrict :-moz-system-metric to chrome and ua sheets.

https://reviewboard.mozilla.org/r/186018/#review191252
Attachment #8914736 - Flags: review?(xidorn+moz) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8914737 [details]
Bug 1396066: Restrict system-metric media features to UA and chrome sheets only.

https://reviewboard.mozilla.org/r/186020/#review191254

r=me with the issues addressed.

::: layout/style/nsCSSParser.cpp:3505
(Diff revision 1)
>    // case insensitive from CSS - must be lower cased
>    nsContentUtils::ASCIIToLower(mToken.mIdent);
>    nsDependentString featureString(mToken.mIdent, 0);
>    uint8_t satisfiedReqFlags = 0;
>  
> +  if (EnabledState() & (CSSEnabledState::eInUASheets | CSSEnabledState::eInChrome)) {

This line is too long. Please have it less than 80 chars. Probably add a line break after `|`.

::: layout/style/nsMediaFeatures.cpp:740
(Diff revision 1)
>    {
>      &nsGkAtoms::_moz_touch_enabled,
>      nsMediaFeature::eMinMaxNotAllowed,
>      nsMediaFeature::eBoolInteger,
> -    nsMediaFeature::eNoRequirements,
> +    nsMediaFeature::eUserAgentAndChromeOnly,
>      { &nsGkAtoms::touch_enabled },
>      GetSystemMetric
>    },

I'm a bit concerned about this one. From search result, it seems to be widely used as a way to detect touch screen, and is mentioned even in published book.

We should probably add a telemetry before removing this, or at least, we should not unship this until we implement the standard equivalent in bug 1035774.

::: layout/style/nsMediaFeatures.cpp:760
(Diff revision 1)
>    },
>    {
>      &nsGkAtoms::_moz_windows_theme,
>      nsMediaFeature::eMinMaxNotAllowed,
>      nsMediaFeature::eIdent,
> +    // TODO(emilio): Unship.

You may not want to remove the comment when you haven't touched their restriction? It probably doesn't matter a lot, though.

::: layout/style/nsMediaFeatures.cpp:784
(Diff revision 1)
>    {
>      &nsGkAtoms::_moz_physical_home_button,
>      nsMediaFeature::eMinMaxNotAllowed,
>      nsMediaFeature::eBoolInteger,
> -    nsMediaFeature::eNoRequirements,
> +    nsMediaFeature::eUserAgentAndChromeOnly,
>      { &nsGkAtoms::physical_home_button },
>      GetSystemMetric
>    },

This is something we can probably remove directly. It is likely a leftover from B2G. I see there is not even any internal code using it.

::: layout/style/test/test_media_queries.html
(Diff revision 1)
> -  expression_should_be_parseable("-moz-scrollbar-start-backward");
> -  expression_should_be_parseable("-moz-scrollbar-start-forward");
> -  expression_should_be_parseable("-moz-scrollbar-end-backward");
> -  expression_should_be_parseable("-moz-scrollbar-end-forward");
> -  expression_should_be_parseable("-moz-scrollbar-thumb-proportional");
> -  expression_should_be_parseable("-moz-overlay-scrollbars");
> -  expression_should_be_parseable("-moz-windows-default-theme");
> -  expression_should_be_parseable("-moz-mac-graphite-theme");
> -  expression_should_be_parseable("-moz-mac-yosemite-theme");
> -  expression_should_be_parseable("-moz-windows-accent-color-in-titlebar");
> -  expression_should_be_parseable("-moz-windows-compositor");
> -  expression_should_be_parseable("-moz-windows-classic");
> -  expression_should_be_parseable("-moz-windows-glass");
> -  expression_should_be_parseable("-moz-touch-enabled");
> -  expression_should_be_parseable("-moz-swipe-animation-enabled");
> -
> -  expression_should_be_parseable("-moz-scrollbar-start-backward: 0");
> -  expression_should_be_parseable("-moz-scrollbar-start-forward: 0");
> -  expression_should_be_parseable("-moz-scrollbar-end-backward: 0");
> -  expression_should_be_parseable("-moz-scrollbar-end-forward: 0");
> -  expression_should_be_parseable("-moz-scrollbar-thumb-proportional: 0");
> -  expression_should_be_parseable("-moz-overlay-scrollbars: 0");
> -  expression_should_be_parseable("-moz-windows-default-theme: 0");
> -  expression_should_be_parseable("-moz-mac-graphite-theme: 0");
> -  expression_should_be_parseable("-moz-mac-yosemite-theme: 0");
> -  expression_should_be_parseable("-moz-windows-accent-color-in-titlebar: 0");
> -  expression_should_be_parseable("-moz-windows-compositor: 0");
> -  expression_should_be_parseable("-moz-windows-classic: 0");
> -  expression_should_be_parseable("-moz-windows-glass: 0");
> -  expression_should_be_parseable("-moz-touch-enabled: 0");
> -  expression_should_be_parseable("-moz-swipe-animation-enabled: 0");
> -
> -  expression_should_be_parseable("-moz-scrollbar-start-backward: 1");
> -  expression_should_be_parseable("-moz-scrollbar-start-forward: 1");
> -  expression_should_be_parseable("-moz-scrollbar-end-backward: 1");
> -  expression_should_be_parseable("-moz-scrollbar-end-forward: 1");
> -  expression_should_be_parseable("-moz-scrollbar-thumb-proportional: 1");
> -  expression_should_be_parseable("-moz-overlay-scrollbars: 1");
> -  expression_should_be_parseable("-moz-windows-default-theme: 1");
> -  expression_should_be_parseable("-moz-mac-graphite-theme: 1");
> -  expression_should_be_parseable("-moz-mac-yosemite-theme: 1");
> -  expression_should_be_parseable("-moz-windows-accent-color-in-titlebar: 1");
> -  expression_should_be_parseable("-moz-windows-compositor: 1");
> -  expression_should_be_parseable("-moz-windows-classic: 1");
> -  expression_should_be_parseable("-moz-windows-glass: 1");
> -  expression_should_be_parseable("-moz-touch-enabled: 1");
> -  expression_should_be_parseable("-moz-swipe-animation-enabled: 1");

I would probably prefer we keep these tests bug mark them `expression_should_not_be_parseable`. Although I guess removing is also fine...
Attachment #8914737 - Flags: review?(xidorn+moz) → review+

Comment 12

2 years ago
mozreview-review
Comment on attachment 8914738 [details]
Bug 1396066: Restrict -moz-windows-theme and -moz-os-version to UA and chrome only.

https://reviewboard.mozilla.org/r/186022/#review191260

Mostly looks good. Cancel r? for test change.

::: layout/style/test/chrome/bug418986-2.js
(Diff revision 1)
> -// These media queries return value 0 or 1 when the pref is off.
> -// When the pref is on, they should not match.
> -var suppressed_toggles = [
> -  "-moz-mac-graphite-theme",
> -  // Not available on most OSs.
> -//  "-moz-maemo-classic",
> -  "-moz-scrollbar-end-backward",
> -  "-moz-scrollbar-end-forward",
> -  "-moz-scrollbar-start-backward",
> -  "-moz-scrollbar-start-forward",
> -  "-moz-scrollbar-thumb-proportional",
> -  "-moz-touch-enabled",
> -  "-moz-windows-compositor",
> -  "-moz-windows-default-theme",
> -  "-moz-windows-glass",
> -];
> -
> -// Possible values for '-moz-os-version'
> -var windows_versions = [
> -  "windows-win7",
> -  "windows-win8",
> -  "windows-win10",
> -];
> -
> -// Possible values for '-moz-windows-theme'
> -var windows_themes = [
> -  "aero",
> -  "aero-lite",
> -  "luna-blue",
> -  "luna-olive",
> -  "luna-silver",
> -  "royale",
> -  "generic",
> -  "zune"
> -];

Majority of the test change should be in the previous part?

I would prefer we keep each part self-contained so that we can backout any specific part if necessary.

Also if we are not removing `-moz-touch-enabled`, we probably need to keep most of the test code below.

Actually I would prefer we don't remove the test code, but treat them as if they are always resisted so that we don't regress. Probably marking them not parseable in the media query test is enough, though...

::: layout/style/test/test_media_queries.html
(Diff revision 1)
> -  expression_should_be_parseable("-moz-windows-theme: aero");
> -  expression_should_be_parseable("-moz-windows-theme: aero-lite");
> -  expression_should_be_parseable("-moz-windows-theme: luna-blue");
> -  expression_should_be_parseable("-moz-windows-theme: luna-olive");
> -  expression_should_be_parseable("-moz-windows-theme: luna-silver");
> -  expression_should_be_parseable("-moz-windows-theme: royale");
> -  expression_should_be_parseable("-moz-windows-theme: generic");
> -  expression_should_be_parseable("-moz-windows-theme: zune");
> -  expression_should_be_parseable("-moz-windows-theme: garbage");

As before, I would prefer we do `not_be_parseable`.
Attachment #8914738 - Flags: review?(xidorn+moz)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8914739 [details]
Bug 1396066: Remove -moz-physical-home-button.

https://reviewboard.mozilla.org/r/186024/#review191262

::: layout/style/test/test_media_queries.html
(Diff revision 1)
> -  expression_should_be_parseable("-moz-os-version: windows-win7");
> -  expression_should_be_parseable("-moz-os-version: windows-win8");
> -  expression_should_be_parseable("-moz-os-version: windows-win10");

`not_be_parseable`
Attachment #8914739 - Flags: review?(xidorn+moz) → review+
Is this disabling the equivalent non-standard media features [1] as well, or just :-moz-system-metric pseudo-classes?

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/Mozilla_Extensions#Media_features
(In reply to Kohei Yoshino [:kohei] from comment #14)
> Is this disabling the equivalent non-standard media features [1] as well, or
> just :-moz-system-metric pseudo-classes?
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Web/CSS/
> Mozilla_Extensions#Media_features

Both of them, although I'm arguing that we should probably not unship -moz-touch-enabled before bug 1035774.

The last patch also unships :-moz-is-glyph in addition to all the system metrics stuff, which I guess is probably fine given there is unlikely any reason people want to use it anywhere.

Comment 16

2 years ago
mozreview-review
Comment on attachment 8914740 [details]
Bug 1396066: Restrict -moz-is-glyph to UA and chrome only.

https://reviewboard.mozilla.org/r/186026/#review191266

It is probably fine that we don't test anything around `:-moz-is-glyph`... if it doesn't work properly, it should be easily caught by refests.
Attachment #8914740 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15)
> Both of them, although I'm arguing that we should probably not unship
> -moz-touch-enabled before bug 1035774.

Thanks for your clarification. I was actually thinking about it.

Comment 18

2 years ago
mozreview-review-reply
Comment on attachment 8914738 [details]
Bug 1396066: Restrict -moz-windows-theme and -moz-os-version to UA and chrome only.

https://reviewboard.mozilla.org/r/186022/#review191260

> Majority of the test change should be in the previous part?
> 
> I would prefer we keep each part self-contained so that we can backout any specific part if necessary.
> 
> Also if we are not removing `-moz-touch-enabled`, we probably need to keep most of the test code below.
> 
> Actually I would prefer we don't remove the test code, but treat them as if they are always resisted so that we don't regress. Probably marking them not parseable in the media query test is enough, though...

I think keeping them in both this test and media queries test is probably more preferable, since if we need to re-enable anything for webcompat, we wouldn't forget that it should still be surpressed when resisting fingerprinting.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8914737 [details]
Bug 1396066: Restrict system-metric media features to UA and chrome sheets only.

https://reviewboard.mozilla.org/r/186020/#review191418


C/C++ static analysis didn't find any defects in this patch. Hooray!

Comment 25

2 years ago
mozreview-review
Comment on attachment 8914738 [details]
Bug 1396066: Restrict -moz-windows-theme and -moz-os-version to UA and chrome only.

https://reviewboard.mozilla.org/r/186022/#review191640
Attachment #8914738 - Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

2 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e5b9aed44133
Restrict :-moz-system-metric to chrome and ua sheets. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/87b44ea10407
Restrict system-metric media features to UA and chrome sheets only. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/b9d8e4588584
Restrict -moz-windows-theme and -moz-os-version to UA and chrome only. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/19c9ea492f5e
Restrict -moz-is-glyph to UA and chrome only. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/5b1997aeaead
Remove -moz-physical-home-button. r=xidorn

Comment 32

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 74a0c5bdb21c -d a701f7c8334f: rebasing 424496:74a0c5bdb21c "Bug 1396066: Restrict :-moz-system-metric to chrome and ua sheets. r=xidorn"
note: rebase of 424496:74a0c5bdb21c created no changes to commit
rebasing 424497:7d8e0e0215f2 "Bug 1396066: Restrict system-metric media features to UA and chrome sheets only. r=xidorn"
merging layout/style/nsMediaFeatures.cpp
merging layout/style/test/chrome/bug418986-2.js
merging layout/style/test/test_media_queries.html
warning: conflicts while merging layout/style/nsMediaFeatures.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
So autocomplete-item.css worries me.  Why is the test failing for that?  It should be loaded from a chrome:// URI, I would think...
Flags: needinfo?(emilio)

Comment 36

2 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a22efa8df6b
followup.  Fix the expected message to be the right non-ASCII thing, admit that devtools don't use UA-only media features.

Comment 37

2 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e41f9295ee00
another followup.  Load input.css from a chrome uri, so it can use :-moz-system-metric.  This requires exposing the reftest chrome package to content, but that should be OK.  r=bzbarsky
windows/autocomplete-item.css contains -moz-windows-default-theme, so that should probably be available on chrome (it is even possible we need to keep exposing it to content, since it seems to be in an XBL.)

That should definitely not be whitelisted.

The whitelist for svg.css also looks too loose. It should probably be specific to only the violating ones that are ua-only.
(In reply to Pulsebot from comment #37)
> Pushed by bzbarsky@mozilla.com:
> https://hg.mozilla.org/integration/autoland/rev/e41f9295ee00
> another followup.  Load input.css from a chrome uri, so it can use
> :-moz-system-metric.  This requires exposing the reftest chrome package to
> content, but that should be OK.  r=bzbarsky

I suspect that it wouldn't work. IIRC availability of chrome-only features depends on document url rather than stylesheet url?
Depends on: 1406582
Just figured out that there are several media features that we still need to expose because chrome code uses them in matchMedia. Also there are some features that can be removed completely, because they are no longer used.
(Assignee)

Comment 41

2 years ago
Yeah, that still doesn't explain why https://hg.mozilla.org/integration/autoland/rev/e41f9295ee00 doesn't work, but anyway, Xidorn and I discussed and we're going to back this out for now, and reland more incrementally.
Flags: needinfo?(emilio)

Comment 42

2 years ago
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4279ffa1e0da
Backed out 9 changesets for Windows reftest failures

Comment 43

2 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2c404f982003
Add a mechanism to make media features chrome / UA-only. r=xidorn
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Updated

2 years ago
Depends on: 1406631
(Assignee)

Updated

a year ago
Depends on: 1408838
(Assignee)

Updated

a year ago
Depends on: 1408839
(Assignee)

Updated

a year ago
Depends on: 1410074
(Assignee)

Comment 45

a year ago
Bug 1410074 takes care of the last ones, yay!
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Keywords: leave-open
I've documented this; first of all take a look at the Fx 58 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/58#CSS_2

All of the pages I've touched are listed there, and they match the pages listed in Kohei's compat article. On each of the pages I've added a note stating that they are no longer available to web content in Firefox 58.

I've also updated the Mozilla CSS extensions page, moving the affected pseudo-classes to this section:

https://developer.mozilla.org/en-US/docs/Web/CSS/Mozilla_Extensions#Mozilla-only_properties_and_pseudo-classes_(avoid_using_on_websites)

Let me know if that look OK.
Keywords: dev-doc-needed → dev-doc-complete

Comment 47

a year ago
Is it intentional that these should be unavailable to userChrome.css too? Not being able to define styles per-os and per-theme can be quite limiting.
(Assignee)

Comment 48

a year ago
(In reply to Alex Vallat from comment #47)
> Is it intentional that these should be unavailable to userChrome.css too?
> Not being able to define styles per-os and per-theme can be quite limiting.

I think you should be able to still style per theme with :-moz-lwtheme-* pseudoclasses, but... Per [1], it looks these are loaded with file:// URIs, so they're not loaded as "chrome" stylesheets. Also, there's nothing in the CSS system that makes these available to user sheets...

I can see no reason why they shouldn't though, mind filing a bug and needinfo me?

[1]: https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/layout/style/nsLayoutStylesheetCache.cpp#443

Comment 49

a year ago
Sorry, I meant windows theme, like -moz-windows-classic, not -moz-lwtheme themes.

I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1418963 as requested.
(Assignee)

Updated

a year ago
Depends on: 1418963
You need to log in before you can comment on or make changes to this bug.