1069192 - Allow restricting CSS properties to UA sheets and chrome

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Markus Stange [:mstange]]

Attachment: patch

For back story see bug 944836 comment 73 and following.

What's left to do here is:
 - Either remove the -moz-window-shadow and -moz-window-dragging sections from property_database.js or add an ignore-chrome-only mode that tests can use for testing chrome-only properties
 - Fix browser/base/content/test/general/browser_parsable_css.js to attach stylesheets to a chrome document, so that it doesn't complain about unrecognized properties.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Since I want to add a UA-only property for bug 1165538, I'd like to revisit this bug and see if there is anything doable.

Let me list the requirement first. Basically, we have two sets of properties which should be restricted:
1. experimental web features, which are controlled by prefs
2. internal-only properties

For experimental web features, we want the following levels:
1. disabled for all
2. allow in UA sheets
3. allow in UA sheets + chrome + certificated apps
4. enabled for all

For internal-only properties:
1. allow in UA sheets
2. allow in UA sheets + chrome

(Do we want to have internal-only properties also work for certificated apps?)

For the requirement above, I propose the following change:
* remove ALWAYS_ENABLED_IN_{UA_SHEETS,CHROME_OR_CERTIFIED_APP};
* reuse the two bits to form four restrict levels:
  00) no restrict,
  01) UA sheets (LEVEL_UA_SHEETS),
  10) UA sheets and chrome (LEVEL_CHROME),
  11) UA sheets, chrome, and certificated apps (LEVEL_CERTIFICATED_APP);
* if a prop has empty pref but non-zero restrict level, set it off in gPropertyEnabled;
* check IsEnabled() according to the levels described above.

Does it sound like a reasonable plan?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #1)
> Since I want to add a UA-only property for bug 1165538, I'd like to revisit
> this bug and see if there is anything doable.

One additional note is that Zack's work in bug 1035091 might be useful here (and some of the refactoring might be able to reland before the actual @-moz-document changes).

> Let me list the requirement first. Basically, we have two sets of properties
> which should be restricted:
> 1. experimental web features, which are controlled by prefs
> 2. internal-only properties
> 
> For experimental web features, we want the following levels:
> 1. disabled for all
> 2. allow in UA sheets
> 3. allow in UA sheets + chrome + certificated apps
> 4. enabled for all

We can probably remove the code for option (3) now.  I filed bug 1202908 to do this.

And, for that matter, I'm not even sure that we really need (1); we might be able to use (2) for the cases where we've done (1) in the past.  But that would be more work to remove.

> For internal-only properties:
> 1. allow in UA sheets
> 2. allow in UA sheets + chrome
> 
> (Do we want to have internal-only properties also work for certificated
> apps?)

No.

> For the requirement above, I propose the following change:
> * remove ALWAYS_ENABLED_IN_{UA_SHEETS,CHROME_OR_CERTIFIED_APP};
> * reuse the two bits to form four restrict levels:
>   00) no restrict,
>   01) UA sheets (LEVEL_UA_SHEETS),
>   10) UA sheets and chrome (LEVEL_CHROME),
>   11) UA sheets, chrome, and certificated apps (LEVEL_CERTIFICATED_APP);

I don't think we're short of bits.  I'd rather keep separate bits for things that need to be separate.

> * if a prop has empty pref but non-zero restrict level, set it off in
> gPropertyEnabled;
> * check IsEnabled() according to the levels described above.

Could you explain more clearly what you want the bits to mean (and what you would call them), both with and without a pref?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #2)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #1)
> > Since I want to add a UA-only property for bug 1165538, I'd like to revisit
> > this bug and see if there is anything doable.
> 
> One additional note is that Zack's work in bug 1035091 might be useful here
> (and some of the refactoring might be able to reland before the actual
> @-moz-document changes).

Yeah, it looks like that should be landed first before any work can be done here.

> > Let me list the requirement first. Basically, we have two sets of properties
> > which should be restricted:
> > 1. experimental web features, which are controlled by prefs
> > 2. internal-only properties
> > 
> > For experimental web features, we want the following levels:
> > 1. disabled for all
> > 2. allow in UA sheets
> > 3. allow in UA sheets + chrome + certificated apps
> > 4. enabled for all
> 
> We can probably remove the code for option (3) now.  I filed bug 1202908 to
> do this.
> 
> And, for that matter, I'm not even sure that we really need (1); we might be
> able to use (2) for the cases where we've done (1) in the past.  But that
> would be more work to remove.

I'm not quite sure either. I see we had some code in ua.css wrapped in @supports blocks, (e.g. ruby, although that checked availability of a value instead of a property) which makes me wonder we probably need (1) here.

> > For the requirement above, I propose the following change:
> > * remove ALWAYS_ENABLED_IN_{UA_SHEETS,CHROME_OR_CERTIFIED_APP};
> > * reuse the two bits to form four restrict levels:
> >   00) no restrict,
> >   01) UA sheets (LEVEL_UA_SHEETS),
> >   10) UA sheets and chrome (LEVEL_CHROME),
> >   11) UA sheets, chrome, and certificated apps (LEVEL_CERTIFICATED_APP);
> 
> I don't think we're short of bits.  I'd rather keep separate bits for things
> that need to be separate.

We are going to be short. The prop flags are stored in a uint32_t, and we have used 27 bits. But sure it doesn't seem to be hard to extend that to uint64_t when all 32 bits are occupied.

> > * if a prop has empty pref but non-zero restrict level, set it off in
> > gPropertyEnabled;
> > * check IsEnabled() according to the levels described above.
> 
> Could you explain more clearly what you want the bits to mean (and what you
> would call them), both with and without a pref?

The proposal was to call them CSS_PROPERTY_EXPOSAL_LEVEL_{UA_SHEETS,CHROME,CERTIFICATED_APP}.

The logic is:
* if a property has neither pref nor any of the levels, it is a standard web property exposed to all
* if it has a pref, then it is an experimental web property, and when the pref is off, it is exposed according to the exposal level
* otherwise it is an internal property, which is always restricted by the exposal level


If we are going to not restrict anything in the UA sheets, and we don't want to expose to certificated apps anything unavailable on the web, then we would only need one flag to determine whether chrome can use it.

It seems we currently don't even have requirement to expose some experimental properties to the chrome. Given that, we can just call the new bit "CSS_PROPERTY_INTERNAL", which indicates it is off by default in gPropertyEnabled, but can be used in the chrome.

Sounds good?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #3)
> The proposal was to call them
> CSS_PROPERTY_EXPOSAL_LEVEL_{UA_SHEETS,CHROME,CERTIFICATED_APP}.

I think it would be clearer to use EXPOSED_IN rather than EXPOSAL_LEVEL.

> The logic is:
> * if a property has neither pref nor any of the levels, it is a standard web
> property exposed to all
> * if it has a pref, then it is an experimental web property, and when the
> pref is off, it is exposed according to the exposal level
> * otherwise it is an internal property, which is always restricted by the
> exposal level

I think that's fine; we should just document it clearly, and probably also email dev-platform about the change when it happens.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #4)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #3)
> > The proposal was to call them
> > CSS_PROPERTY_EXPOSAL_LEVEL_{UA_SHEETS,CHROME,CERTIFICATED_APP}.
> 
> I think it would be clearer to use EXPOSED_IN rather than EXPOSAL_LEVEL.

or, actually, ENABLED_IN rather than EXPOSED_IN

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #5)
> (In reply to David Baron [:dbaron] ⌚UTC-7 from comment #4)
> > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #3)
> > > The proposal was to call them
> > > CSS_PROPERTY_EXPOSAL_LEVEL_{UA_SHEETS,CHROME,CERTIFICATED_APP}.
> > 
> > I think it would be clearer to use EXPOSED_IN rather than EXPOSAL_LEVEL.
> 
> or, actually, ENABLED_IN rather than EXPOSED_IN

But ENABLED_IN sounds like, it is only enabled in the specific case, while "level" sounds like all levels above is included.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6)
> But ENABLED_IN sounds like, it is only enabled in the specific case, while
> "level" sounds like all levels above is included.

ENABLED_IN_UA_SHEETS, ENABLED_IN_UA_SHEETS_AND_CHROME should work, then.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Note from bug 1202908: the patch there leaves nsDOMCSSDeclaration.cpp:IsCSSPropertyExposedToJS() unremoved for this bug. If this bug ends up not using that function, we should remove it and its template variant.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Hmmmm, it seems DOMUtils.parseStyleSheet() parses properities with ALWAYS_ENABLED_IN_UA_SHEETS when it is called from chrome code. Not sure whether it is what we want. I guess not.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d8899a6c87b

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ace9b9fcabea

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

It seems it is easier to just make the restricted properties (which have flags but no pref) not accessible via DOM. We have several tests for checking exposal of DOM accessibility. If we hardly need that, it doesn't make much sense to fix those tests.

I guess it should be fine because people can just still put them in the stylesheet and change them via changing class/attribute on elements if they really need to.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c3f6690d545

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9645a5260573

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1069192 part 1 - Force users of nsCSSProps::IsEnabled() to pass in the enabled state. r=dbaron

Bug 1069192 part 1 - Force users of nsCSSProps::IsEnabled() to pass in the enabled state.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1069192 part 2 - Add a flag for chrome-only properties and change semantics of enabling flags.

Bug 1069192 part 2 - Define the new CSS_PROPERTY_ENABLED_IN_* flags and update related logic.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1069192 part 3 - Add pref to allow unsafe rules in content for testing.

Bug 1069192 part 3 - Add pref to allow unsafe rules in content for testing.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1069192 part 4 - Add UA sheets restriction for some existing internal properties.

Bug 1069192 part 4 - Add UA sheets restriction for some existing internal properties.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1069192 part 5 - Move property flags to an independent header file.

Bug 1069192 part 5 - Move property flags to an independent header file.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1069192 part 6 - Change the logic of accessible-check in ListCSSProperties.

Bug 1069192 part 6 - Change the logic of accessible-check in ListCSSProperties.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1069192 part 3 - Restrict -moz-window-{dragging,shadow} to be chrome-only. r=dbaron

Bug 1069192 part 7 - Restrict -moz-window-{dragging,shadow} to be chrome-only.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8661073 [details]
MozReview Request: Bug 1069192 part 1 - Force users of nsCSSProps::IsEnabled() to pass in the enabled state. r=dbaron

https://reviewboard.mozilla.org/r/19271/#review17899

Otherwise looks good.  r=dbaron with that fixed

::: layout/style/Declaration.cpp:1255
(Diff revision 1)
> -    if (!nsCSSProps::IsEnabled(property)) {
> +    if (!nsCSSProps::IsEnabled(property, nsCSSProps::eIgnoreEnabledState)) {

Don't you mean eEnabledForAllContent instead of eIgnoreEnabledState?

(Using eIgnoreEnabledState makes the entire if a no-op.)


It *might* be plausible to remove the if-statement -- but if you want to do that, it ought to be a separate patch, explaining why it was originally added and why we don't want it anymore.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8661074 [details]
MozReview Request: Bug 1069192 part 2 - Add a flag for chrome-only properties and change semantics of enabling flags.

https://reviewboard.mozilla.org/r/19273/#review17901

I think this could use a clearer commit message:  maybe something like:

Add a flag for whether CSS properties are enabled for chrome: URLs, change semantics of enabling flags so that their presence makes the property default to off when there is no pref, and remove ALWAYS from their name to match.

(Does that cover the changes?)

r=dbaron with that, and the other two comments fixed

::: layout/style/nsCSSProps.cpp:35
(Diff revision 1)
> +  static_assert(!((flags_) & CSS_PROPERTY_ENABLED_MASK) || pref_[0], \

Please test that both of these new static_asserts fail as you expect them to with an entry in nsCSSPropList.h that you think should cause them to fail.

::: layout/style/nsCSSProps.cpp:229
(Diff revision 1)
> -      // also has the flag.
> +      // that all of its component longhands also has the flag.

I know you're not changing it, but "has" -> "have".

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8661075 [details]
MozReview Request: Bug 1069192 part 3 - Add pref to allow unsafe rules in content for testing.

https://reviewboard.mozilla.org/r/19275/#review17903

I guess I'm ok with this, although if you can find a way to make the pref name scarier, that would be nice.  (See also the pref security.turn_off_all_security_so_that_viruses_can_take_over_this_computer.)

::: layout/style/nsCSSParser.cpp:1487
(Diff revision 1)
> +  if (MOZ_UNLIKELY(nsLayoutUtils::AllowsUnsafeRulesInContent())) {

You could replace the 4 lines with just:

  mUnsafeRulesEnabled = aAllowUnsafeRules ||
                        nsLayoutUtils::AllowUnsafeRulesInContent();

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8661076 [details]
MozReview Request: Bug 1069192 part 4 - Add UA sheets restriction for some existing internal properties.

https://reviewboard.mozilla.org/r/19277/#review17905

I think changing existing properties should be in their own bugs, with appropriate discussion.  (e.g., I'd want to ask jfkthame if we want to make the change for -moz-control-character-visibility, and the MathML owners for some of th others).

I think it's appropriate to refactor the code in ParseSingleValueProperty:
  // We only allow 'script-level' when unsafe rules are enabled, because
  // otherwise it could interfere with rulenode optimizations if used in
  // a non-MathML-enabled document. We also only allow math-display when
  // unsafe rules are enabled.
  if (!mUnsafeRulesEnabled &&
      (aPropID == eCSSProperty_script_level ||
       aPropID == eCSSProperty_math_display))
    return false;
though.  But you should probably *remove* that existing code when you do.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://reviewboard.mozilla.org/r/19277/#review17907

::: layout/style/nsCSSPropList.h:3798
(Diff revision 1)
> -    // NOTE: CSSParserImpl::ParseSingleValueProperty only accepts this
> +    CSS_PROPERTY_PARSE_VALUE |

reviewboard doesn't seem to record it as a review- unless I open an issue, so here's an issue

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8661076 [details]
MozReview Request: Bug 1069192 part 4 - Add UA sheets restriction for some existing internal properties.

ok, manual use of bugzilla again

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8661077 [details]
MozReview Request: Bug 1069192 part 5 - Move property flags to an independent header file.

https://reviewboard.mozilla.org/r/19279/#review17911

::: layout/style/nsCSSPropFlags.h:11
(Diff revision 1)
> +#ifndef nsCSSPropFlags_h___

Change from three trailing _ to one.

(The C++ standard reserves all names with two or more consecutive underscores to itself, as well as those with leading underscore, I believe.)

I guess I'll see in the next patches why this is needed.

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #25)
> Comment on attachment 8661076 [details]
> MozReview Request: Bug 1069192 part 4 - Add UA sheets restriction for some
> existing internal properties.
> 
> https://reviewboard.mozilla.org/r/19277/#review17905
> 
> I think changing existing properties should be in their own bugs, with
> appropriate discussion.  (e.g., I'd want to ask jfkthame if we want to make
> the change for -moz-control-character-visibility, and the MathML owners for
> some of th others).

I hope them to be fixed in this bug for the test change in part 6. I'll split this patch and ask people you mentioned to review the patches.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8661078 [details]
MozReview Request: Bug 1069192 part 6 - Change the logic of accessible-check in ListCSSProperties.

This was intentionally the way it was, since the tests should duplicate what they're testing as much as possible.  I'd prefer that people have to fix the test separately from the code here.

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #30)
> Comment on attachment 8661078 [details]
> MozReview Request: Bug 1069192 part 6 - Change the logic of accessible-check
> in ListCSSProperties.
> 
> This was intentionally the way it was, since the tests should duplicate what
> they're testing as much as possible.  I'd prefer that people have to fix the
> test separately from the code here.

OK, then I can remove part 3~6 and just fix this test in the last patch.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8661079 [details]
MozReview Request: Bug 1069192 part 3 - Restrict -moz-window-{dragging,shadow} to be chrome-only. r=dbaron

https://reviewboard.mozilla.org/r/19283/#review17913

Instead of removing the chunks from property_database.js, could you move them to an if(0) at the end of the file, with a comment (and maybe a bug) that we should find a way to run tests on the properties that are enabled for chrome only?

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fafe50b270b

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8661073 [details]
MozReview Request: Bug 1069192 part 1 - Force users of nsCSSProps::IsEnabled() to pass in the enabled state. r=dbaron

Bug 1069192 part 1 - Force users of nsCSSProps::IsEnabled() to pass in the enabled state. r=dbaron

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8661074 [details]
MozReview Request: Bug 1069192 part 2 - Add a flag for chrome-only properties and change semantics of enabling flags.

Bug 1069192 part 2 - Define the new CSS_PROPERTY_ENABLED_IN_* flags and update related logic. r=dbaron

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8661079 [details]
MozReview Request: Bug 1069192 part 3 - Restrict -moz-window-{dragging,shadow} to be chrome-only. r=dbaron

Bug 1069192 part 3 - Restrict -moz-window-{dragging,shadow} to be chrome-only. r?dbaron

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

Ah... forgot to change the commit message of part 2... Add ni? myself so I won't forget it again.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's probably worth still doing this part too, previously in part 4:

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #25)
> I think it's appropriate to refactor the code in ParseSingleValueProperty:
>   // We only allow 'script-level' when unsafe rules are enabled, because
>   // otherwise it could interfere with rulenode optimizations if used in
>   // a non-MathML-enabled document. We also only allow math-display when
>   // unsafe rules are enabled.
>   if (!mUnsafeRulesEnabled &&
>       (aPropID == eCSSProperty_script_level ||
>        aPropID == eCSSProperty_math_display))
>     return false;
> though.  But you should probably *remove* that existing code when you do.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://reviewboard.mozilla.org/r/19273/#review17925

::: layout/style/nsCSSProps.cpp:34
(Diff revision 2)
> +#define CSS_PROP(name_, id_, method_, flags_, pref_, ...) \
> +  static_assert(!((flags_) & CSS_PROPERTY_ENABLED_MASK) || pref_[0], \
> +                "Internal-only property '" #name_ "' should be wrapped in " \
> +                "#ifndef CSS_PROP_LIST_EXCLUDE_INTERNAL");

One other comment here, actually:

It's not clear to me why you're requiring this.  Is it because an internal-only property without a pref won't trigger the IsEnabled call, and this is the way you're forcing them not to be reflected in the CSSOM?  If so, you should explain that in a comment, because it's something somebody might need to fix someday, e.g., to make a chrome-only property reflected in the CSSOM, with [ChromeOnly] in the WebIDL.

(If that's not why, then I'd like to know why.)

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8661079 [details]
MozReview Request: Bug 1069192 part 3 - Restrict -moz-window-{dragging,shadow} to be chrome-only. r=dbaron

https://reviewboard.mozilla.org/r/19283/#review17927

::: layout/style/test/property_database.js:6588
(Diff revision 2)
> +    domProp: "MozWindowDragging",

You should comment out these domProp lines in the test as well since there isn't really any dom property for these.  (Probably with an English comment explaining it, too.)

Comment 41

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #38)
> It's probably worth still doing this part too, previously in part 4:

I'll do that in bug 1207002 instead of this one, since if we do not want to change the test in the way I proposed, it is not necessary to have that part done in this bug.

Comment 42

[Xidorn Quan [:xidorn] UTC+10]

I wonder why my try push in comment 14 doesn't show the failure from browser_parsable_css.js...

Comment 43

[Xidorn Quan [:xidorn] UTC+10]

Attaching stylesheets to a chrome document won't fix that test, because the current impl checks the URI of the sheet instead of the principal. I guess we probably could change the impl to check the principal instead, but I don't think we can reference file:/jar: styles in chrome document, can we?

Comment 44

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0fe904d0869

Comment 45

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #43)
> Attaching stylesheets to a chrome document won't fix that test, because the
> current impl checks the URI of the sheet instead of the principal. I guess
> we probably could change the impl to check the principal instead, but I
> don't think we can reference file:/jar: styles in chrome document, can we?

It seems we can... at least in my local build. Let's see what the try server says.

Comment 46

[Xidorn Quan [:xidorn] UTC+10]

I have no idea how to reproduce the failure on the try server... Locally, the test always uses file: which works just fine with my new code. However, the server always uses jar:file: which doesn't seem to work fine, but I want to figure out why. But I cannot set my local build to use the way try server use. Any idea?

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Are you building with --enable-chrome-format=flat or something like that?  (Also, does it fail on your platform on try?)

Comment 48

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #47)
> Are you building with --enable-chrome-format=flat or something like that? 

I didn't have such thing in my mozconfig. And even if I explicitly set --enable-chrome-format=onmi, I still do not see onmi.ja in my obj dir, and the test doesn't fail either.

> (Also, does it fail on your platform on try?)

I could reproduce the same failure on Windows when I removed the platform condition (XP_MACOSX) around the to-be-restricted property usage in browser.css. And this failure no longer appears locally with my new patches.

I also tried on my OS X, where I cannot reproduce this failure as well (because it doesn't use jar:file: either).

Comment 49

[Xidorn Quan [:xidorn] UTC+10]

So to run tests on packaged build, we need to execute `mach package` after build, and then add `--appname dist` to the mochitest command. I successfully reproduce the failure with this steps.

Comment 50

[Xidorn Quan [:xidorn] UTC+10]

Hmmm, interesting... Stylesheets loaded from "file:" inherit the system principal from its owner chrome document because it is a local file, however, those from "jar:" do not inherit. This behavior is coded here: https://dxr.mozilla.org/mozilla-central/rev/197af2fb7e29ff8e4b3b6ced723b6172e954e17d/layout/style/Loader.cpp#1436-1450

I have no idea why we want local file to inherit the principal, and I have no idea why we don't want local jar file to inherit the principal either. Any idea?

I guess the best solution would be changing the test so that everything is loaded from "chrome:". But I have no idea how can I convert a local file path or jar path to its corresponding chrome path.

Comment 51

[Boris Zbarsky [:bzbarsky]]

The file:// special case there was added in bug 402983.  The basic idea was that if you have a file:// HTML page that links to a file:// stylesheet it should be able to script it.  The fact that it happens to also inherit when a system sheet links to a file:// page is mostly an accident (and not necessarily a desirable one) due to the use of CheckMayLoad, which always passes for system.

I'm a bit confused about why the system principal is involved here, thought; isn't the page involved <resource://testing-common/resource_test_file.html>?

Comment 52

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Boris Zbarsky [:bz] from comment #51)
> I'm a bit confused about why the system principal is involved here, thought;
> isn't the page involved <resource://testing-common/resource_test_file.html>?

I hope the whole test happens in within "chrome:" because that's how those style files are typically loaded.

The page involved is on URI with "file:" or "jar:file:". The "resource:" one is resolved to one of these local protocols.

Comment 53

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Boris Zbarsky [:bz] from comment #51)
> The file:// special case there was added in bug 402983.  The basic idea was
> that if you have a file:// HTML page that links to a file:// stylesheet it
> should be able to script it.  The fact that it happens to also inherit when
> a system sheet links to a file:// page is mostly an accident (and not
> necessarily a desirable one) due to the use of CheckMayLoad, which always
> passes for system.

I guess it means I should not rely on the behavior that file:// stylesheet inherits the principal, and should try to load everything from chrome protocol. Then I'll need to figure out how can I do that :/

Comment 54

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a925e158b21f

Comment 55

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8661073 [details]
MozReview Request: Bug 1069192 part 1 - Force users of nsCSSProps::IsEnabled() to pass in the enabled state. r=dbaron

Bug 1069192 part 1 - Force users of nsCSSProps::IsEnabled() to pass in the enabled state. r=dbaron

Comment 56

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8661074 [details]
MozReview Request: Bug 1069192 part 2 - Add a flag for chrome-only properties and change semantics of enabling flags.

Bug 1069192 part 2 - Add a flag for chrome-only properties and change semantics of enabling flags.

This patch changes the semantics of enabling flags so that their presence makes
the property default to off when there is no pref, and also removes "ALWAYS"
from their name to match.

Comment 57

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8661079 [details]
MozReview Request: Bug 1069192 part 3 - Restrict -moz-window-{dragging,shadow} to be chrome-only. r=dbaron

Bug 1069192 part 3 - Restrict -moz-window-{dragging,shadow} to be chrome-only. r=dbaron

Comment 58

[Xidorn Quan [:xidorn] UTC+10]

The part 2 has only one line different with the previously r+ed revision. Could you review that line, and say whether we should use this or just keep using the previous one? The difference can be seen here: https://reviewboard.mozilla.org/r/19273/diff/2-3/

I guess the security of the two approaches should be almost identical (except the case mentioned in comment 51), however, checking principal should be a bit faster than checking scheme because the former is just comparing two pointers and the latter is a string comparison.

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, I'll have a look.

I can't figure out how to tell which of the four versions of the patch in MozReview was the previous version that I reviewed, or to easily find my previous review comments (since there's no separate bugzilla attachment with a review+ on it in the bugzilla attachments list).  So I'll probably have to re-review the entire patch.

Given that, I think I've decided that MozReview's bugzilla integration is bad enough that I'd like to ask you not to use it any more.  Please make future review requests to me using normal bugzilla attachments rather than MozReview.  While MozReview's diff viewer and comment interface is nice, it's too hard to understand state and history.

Comment 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8661074 [details]
MozReview Request: Bug 1069192 part 2 - Add a flag for chrome-only properties and change semantics of enabling flags.

https://reviewboard.mozilla.org/r/19273/#review18889

OK, I think I've figured out MozReview now, so I guess it's ok to keep using it.  It took a while to figure out, but now that I have, I understand how to do re-reviews.

r=dbaron with the comment addressed

::: layout/style/nsCSSProps.cpp:37
(Diff revisions 1 - 3)
> +// do this rather than adding regulation elsewhere.

"adding regulation elsewhere" -> "exposing them conditionally" or something like that.

("regulation" isn't the right English word in this context, and is probably best replaced by something more concrete as well)

Comment 61

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4aaaac8f133194a65eda12a9f8d357ec3b9b13a
Bug 1069192 part 1 - Force users of nsCSSProps::IsEnabled() to pass in the enabled state. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f11c5c5bcbb95e411c4c8547115e209ebfee96
Bug 1069192 part 2 - Add a flag for chrome-only properties and change semantics of enabling flags. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8611141205006fee2fc845d6f4f0b8c47d58fcd
Bug 1069192 part 3 - Restrict -moz-window-{dragging,shadow} to be chrome-only. r=dbaron

Comment 62

[Nigel Babu [:nigelb]]

Backed out for causing crashes that look like this -> https://treeherder.mozilla.org/logviewer.html#?job_id=15086086&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/9acf4458a79d

Comment 63

[Xidorn Quan [:xidorn] UTC+10]

I reasonably doubt that the failure was due to part 3, becuase that only happens on OS X. Since this the ability of restricting properties is what actually blocking other bugs, I'd like to land part 1 and part 2 first, and land part 3 in a separate bug.

Comment 64

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90ea96512ed46af7f3617ce0dc22284d598c52e7
Bug 1069192 part 1 - Force users of nsCSSProps::IsEnabled() to pass in the enabled state. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e42cb1acb5dcfb2a7feafde941199a522defd48
Bug 1069192 part 2 - Add a flag for chrome-only properties and change semantics of enabling flags. r=dbaron

Comment 65

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/90ea96512ed4
https://hg.mozilla.org/mozilla-central/rev/7e42cb1acb5d