CSS filters break site layout when the filter value is empty and the closing semicolon is missing

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rowbot, Assigned: mvujovic)

Tracking

Trunk
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Posted image filters enabled.png
If you visit [1], with layout.css.filters.enabled = true, the layout of the site is completely broken.  Setting layout.css.filters.enabled = false fixes the problem.  The cause of this appears to be that the one of the stylesheets contains filter properties with empty values AND the filter property is not ended with a semicolon. An empty value with a closing semicolon does not cause a problem.

The offending rules appear to be on lines 293, 297, and 307 of the main-rollup.css file at [1].  Removing those lines in the Style Editor fixes the problem.

I'm not sure how common it is to find the filter property with an empty value and no closing semicolon out there in the wild.  So I'm not sure if this is something worth fixing in the parser or if it should just be turned into a tech evangelism bug.

[1] https://forum.myrcon.com/
Posted image filters disabled.png
Blocks: 1057180
> So I'm not sure if this is something worth fixing in the parser

This needs to be fixed in the parser, for sure.

The relevant rules look like this:

  .cke_buttonarrow{filter:}

As far as I can tell, CSSParserImpl::ParseSingleFilter is just buggy.  It gets the first non-whitespace token, and if that's not a url and filters are enabled and it's not a function, then it returns false... without ungetting the token.  Which means in this case it consumes the '}' and then we see we have an invalid value and try to find end-of-declaration, which we never do (because the '}' got consumes).

Max, it looks like you have blame for this code?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mvujovic)
Thanks for the report, Trevor, and thanks for pinpointing the problem, Boris. I'll post a fix for this.
Assignee: nobody → mvujovic
Flags: needinfo?(mvujovic)
Posted patch Patch (obsolete) — Splinter Review
The site is fixed when this patch is applied.
Attachment #8477937 - Flags: review?(bzbarsky)
I'm not convinced the test is necessary -- if you enable the pref, I think you should get a failure in test_property_syntax_errors.html (from the check_empty_value_rejected function).

(It's worth testing if you get any other errors in the layout/style mochitests with the pref enabled.)

If you don't get an error from that test with the pref enabled, I'd like to know why that test isn't working.
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #5)
> I'm not convinced the test is necessary -- if you enable the pref, I think
> you should get a failure in test_property_syntax_errors.html (from the
> check_empty_value_rejected function).

You're exactly right. I get the expected failures in test_property_syntax_errors.html without the code change, and they're fixed by the code change. Thanks for pointing that test out- I'll remove mine.

> 
> (It's worth testing if you get any other errors in the layout/style
> mochitests with the pref enabled.)

Looks like I get some other errors. I've filed bug 1057900 to track them.

> 
> If you don't get an error from that test with the pref enabled, I'd like to
> know why that test isn't working.
Posted patch Patch v2 (obsolete) — Splinter Review
Same code change, minus the new test, since test_property_syntax_errors.html already covers this bug when CSS filters is enabled.
Attachment #8477937 - Attachment is obsolete: true
Attachment #8477937 - Flags: review?(bzbarsky)
Attachment #8478038 - Flags: review?(bzbarsky)
Comment on attachment 8478038 [details] [diff] [review]
Patch v2

r=dbaron
Attachment #8478038 - Flags: review?(bzbarsky) → review+
Status: NEW → ASSIGNED
Posted patch Patch v3Splinter Review
Change commit message to specify dbaron as reviewer.

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/236906d22acf

Try results:
https://tbpl.mozilla.org/?tree=Try&rev=40ebd30fadf8
Attachment #8478038 - Attachment is obsolete: true
Attachment #8478538 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/236906d22acf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.