Closed Bug 894245 Opened 10 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: jruderman, Assigned: xidorn)

References

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

testcase
123 bytes, text/html
Details
Bug 894245 part 1 - Align windows widget behavior with other widgets to return failure when the specified color is unknown.
59 bytes, text/x-review-board-request
jimm
: review+
Details
Bug 894245 part 2 - Reject unknown enum color in CSS parser.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Attached file 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.
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 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 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?
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
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!
https://hg.mozilla.org/mozilla-central/rev/6d7eb00d02c7
https://hg.mozilla.org/mozilla-central/rev/4172b6f24000
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1323336
You need to log in before you can comment on or make changes to this bug.