"ASSERTION: unexpected color unit" using the wrong platform's special color values

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: jruderman, Assigned: xidorn)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla53
x86_64
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 776215 [details]
testcase

###!!! ASSERTION: unexpected color unit: 'eCSSUnit_Null == backColorValue->GetUnit()', file layout/style/nsRuleNode.cpp, line 5770
What should the behaviour be when we use one of these platform-specific L&F colours on the wrong platform?  I guess we should be setting bg->mBackgroundColor to something.
Yes; this code is assuming that nsLookAndFeel::GetColor will never fail, and it's really not obvious what it should do when it does fail.  Worst case, I suppose fall back to either black or transparent and issue a console warning.
It would be better to make GetColor not fail, though.
(Reporter)

Comment 4

5 years ago
So you'd prefer to change nsLookAndFeel::GetColor to [fall back to either black or transparent and issue a console warning], rather than having the caller do that?

Or maybe the CSS *parser* should reject wrong-platform colors?
Yeah, in this case I bet we could make the parser do it.  That's a good idea, and should work.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8822601 [details]
Bug 894245 part 1 - Align windows widget behavior with other widgets to return failure when the specified color is unknown.

https://reviewboard.mozilla.org/r/101466/#review102018

seems odd that the call fails yet it still returns a value for COLOR_WINDOW. This said, sounds like styling code prefers this and is the main consumer of LookAndFeel so, ok with me. Lets add a warning like we have on osx.

http://searchfox.org/mozilla-central/source/widget/cocoa/nsLookAndFeel.mm#310
Attachment #8822601 - Flags: review?(jmathies) → review+
Assignee: nobody → xidorn+moz

Comment 10

2 years ago
mozreview-review
Comment on attachment 8822602 [details]
Bug 894245 part 2 - Reject unknown enum color in CSS parser.

https://reviewboard.mozilla.org/r/101468/#review102052

r=me with issues addressed:

::: layout/style/nsCSSParser.cpp:61
(Diff revision 2)
>  #include "nsRuleData.h"
>  #include "mozilla/CSSVariableValues.h"
>  #include "mozilla/dom/AnimationEffectReadOnlyBinding.h"
>  #include "mozilla/dom/URL.h"
>  #include "gfxFontFamilyList.h"
> +#include "mozilla/LookAndFeel.h"

Let's group this new #include alongside some other "#include mozilla/Foo" stuff here, so that you're not reducing the (admittedly-already-poor) sortedness of this list.

e.g. maybe a few lines up before mozilla/Preferences, or up at the top just before your Maybe.h include.

::: layout/style/nsCSSParser.cpp:6695
(Diff revision 2)
>    declaration->CompressFrom(&mData);
>    return declaration.forget();
>  }
>  
> +static Maybe<int32_t>
> +GetEnumColorValue(nsCSSKeyword aKeyword, bool aIsChrome)

Technically you could simplify things by getting rid of the "aIsChrome" arg here.

You only use it in the LookAndFeel::GetColor Function-call, and there, it only influences *what* color is returned -- not *whether* a color is returned.  And in this function, we're only interested in *whether* a color is returned -- we don't actually use the resulting color.  So you could just as easily use the 2-arg LookAndFeel::GetColor API.

But I can see a theoretical-purity argument for using the identical API that we later use to actually get the color (in nsRuleNode), so this is fine the way you have it, too.

::: layout/style/nsCSSParser.cpp:6703
(Diff revision 2)
> +  if (!nsCSSProps::FindKeyword(aKeyword, nsCSSProps::kColorKTable, value)) {
> +    // Unknown color keyword.
> +    return Nothing();
> +  }
> +  if (value < 0) {
> +    // Known special color keyword handled by style system.

Reading this, I'm left wondering which sorts of color keywords fall into this category.  I initially assumed (incorrectly, I think) that this was for the colors in the included crashtest.  Only after a bit of searching did I find the colors in question:
https://dxr.mozilla.org/mozilla-central/rev/6f63f95e28ffc05c0d2f5ef6cd6e05905fe8ea5a/layout/style/nsStyleConsts.h#317-325

To make this clearer/more-discoverable, perhaps add "(e.g. NS_COLOR_CURRENTCOLOR)" at the end of this comment, so that people who are curious have something they can look up to find the list of negative color keywords that fall into this codepath.

::: layout/style/nsCSSParser.cpp:6709
(Diff revision 2)
> +    return Some(value);
> +  }
> +  nscolor color;
> +  auto colorID = static_cast<LookAndFeel::ColorID>(value);
> +  if (NS_FAILED(LookAndFeel::GetColor(colorID, !aIsChrome, &color))) {
> +    // Known color but not present from LookAndFeel.

This comment doesn't make sense to me. "Known color" sounds like we know the color, and yet we're returning Nothing().  And "not present from LookAndFeel" is also confusing given that we have a LookAndFeel::ColorID already.  (It's confusing without context about how LookAndFeel::GetColor works, at least.)

Perhaps clarify to something like:
    // Known LookAndFeel::ColorID, but this platform's LookAndFeel impl
    // doesn't map it it to a color. (This might be a platform-specific
    // ColorID, which only makes sense on another platform.)

::: layout/style/nsCSSParser.cpp:6759
(Diff revision 2)
>          aValue.SetStringValue(tk->mIdent, eCSSUnit_Ident);
>          return CSSParseResult::Ok;
>        }
> -      else {
> -        nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(tk->mIdent);
> +      nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(tk->mIdent);
> -        if (eCSSKeyword_UNKNOWN < keyword) { // known keyword
> +      if (Maybe<int32_t> value = GetEnumColorValue(keyword, mIsChrome)) {

Observation: you're removing this eCSSKeyword_UNKNOWN comparison -- and initially I thought that was bad, but after some further inspection I think it's actually a good change, since nsCSSProps::FindKeyword handles that check internally now. I filed bug 1326457 on removing more similar eCSSKeyword_UNKNOWN comparisons.
Attachment #8822602 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #10)
> Observation: you're removing this eCSSKeyword_UNKNOWN comparison -- and
> initially I thought that was bad, but after some further inspection I think
> it's actually a good change [...]

I was going to skip this chunk in bug 1326457, but now I actually think it'd be nice to clean this up (i.e. remove the _UNKNOWN check) over there instead of here, to reduce the number of unrelated simplifications in this patch.  So then this would need to be (slightly) rebased on top of bug 1326457 (and this "part 2" patch would be a bit simpler).

Would that be OK with you?
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8822602 [details]
Bug 894245 part 2 - Reject unknown enum color in CSS parser.

https://reviewboard.mozilla.org/r/101468/#review102052

> Technically you could simplify things by getting rid of the "aIsChrome" arg here.
> 
> You only use it in the LookAndFeel::GetColor Function-call, and there, it only influences *what* color is returned -- not *whether* a color is returned.  And in this function, we're only interested in *whether* a color is returned -- we don't actually use the resulting color.  So you could just as easily use the 2-arg LookAndFeel::GetColor API.
> 
> But I can see a theoretical-purity argument for using the identical API that we later use to actually get the color (in nsRuleNode), so this is fine the way you have it, too.

It influences whether a color is returned. When `aUseStandinsForNativeColors` is `true` and pref "ui.use_standins_for_native_colors" is set to `true`, `LookAndFeel::GetColor` would never call into platform-dependent function to get the colors [1], which means whether a color is returned is indeed affected by it.

[1] https://dxr.mozilla.org/mozilla-central/rev/6f63f95e28ffc05c0d2f5ef6cd6e05905fe8ea5a/widget/nsXPLookAndFeel.cpp#807-810
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d7eb00d02c7
part 1 - Align windows widget behavior with other widgets to return failure when the specified color is unknown. r=jimm
https://hg.mozilla.org/integration/autoland/rev/4172b6f24000
part 2 - Reject unknown enum color in CSS parser. r=dholbert
Huh -- thanks for looking into that & correcting my mistake!

The documentation suggests that aUseStandinsForNativeColors merely "substitutes" hardcoded values in place of actual native colors, which led me to believe that it'd consistently pass/fail regardless of the param's value (and would simply return a different color).
https://dxr.mozilla.org/mozilla-central/rev/6f63f95e28ffc05c0d2f5ef6cd6e05905fe8ea5a/widget/LookAndFeel.h#512-515

But I suppose this ^ documentation is ambiguous about whether substitution does or doesn't happen, in cases where the native color may or may not be defined.  And my assumed behavior happened to be wrong.  We should perhaps clarify the LookAndFeel documentation to be clearer about that...

Thanks, in any case!

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6d7eb00d02c7
https://hg.mozilla.org/mozilla-central/rev/4172b6f24000
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

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