stylo: 1272475-1.html and 1272475-2.html fails with Assertion failure: !mozilla::IsNaN(mValue.mFloat)

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: hiro, Assigned: boris)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
This is because servo's transform function parser does not check infinity in the first place, whereas we do clamp it in nsCSSParserImple::ParseFunctionInternals.
(Reporter)

Comment 1

2 years ago
If we need to fix in servo side, here is a relevant part:
https://hg.mozilla.org/mozilla-central/file/f4f374622111/servo/components/style/properties/longhand/box.mako.rs#l1118
Priority: -- → P3
(Assignee)

Updated

a year ago
Assignee: nobody → boris.chiou
(Assignee)

Updated

a year ago
See Also: → bug 1343153
(Assignee)

Comment 2

a year ago
After fixing the parser problem, we still have this assertions in both crashtests:

Hit MOZ_CRASH(Array not thread-safe) at objdirs/obj-browser-stylo-debug/dist/include/nsCSSValue.h:1150 [1]

1149
1150 DECLARE_CSS_ARRAY(Array, NS_INLINE_DECL_REFCOUNTING)
1151 DECLARE_CSS_ARRAY(ThreadSafeArray, NS_INLINE_DECL_THREADSAFE_REFCOUNTING)
1152 #undef DECLARE_CSS_ARRAY

[1] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/layout/style/nsCSSValue.h#1150
(Reporter)

Comment 3

a year ago
It's bug 1350244.
(Assignee)

Updated

a year ago
Depends on: 1350244
(Reporter)

Comment 4

a year ago
I don't think this bug needs to depend on bug 1350244 because we don't hit the assertion on our servers.
No longer depends on: 1350244
(Assignee)

Comment 5

a year ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> I don't think this bug needs to depend on bug 1350244 because we don't hit
> the assertion on our servers.

I see. We haven't turn these crash tests on yet. Thanks. :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
(In reply to Boris Chiou [:boris] from comment #2)
> After fixing the parser problem, we still have this assertions in both
> crashtests:
> 
> Hit MOZ_CRASH(Array not thread-safe) at
> objdirs/obj-browser-stylo-debug/dist/include/nsCSSValue.h:1150 [1]
> 
> 1149
> 1150 DECLARE_CSS_ARRAY(Array, NS_INLINE_DECL_REFCOUNTING)
> 1151 DECLARE_CSS_ARRAY(ThreadSafeArray,
> NS_INLINE_DECL_THREADSAFE_REFCOUNTING)
> 1152 #undef DECLARE_CSS_ARRAY
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 7419b368156a6efa24777b21b0e5706be89a9c2f/layout/style/nsCSSValue.h#1150

Running crashtests with STYLO_THREADS=1 can pass these two tests without the above assertions.

Comment 9

a year ago
mozreview-review
Comment on attachment 8851521 [details]
Bug 1336769 - Enable crashtests, 1272475-1.html and 1272475-2.html.

https://reviewboard.mozilla.org/r/123834/#review126232
Attachment #8851521 - Flags: review?(cam) → review+

Comment 10

a year ago
mozreview-review
Comment on attachment 8851520 [details]
Bug 1336769 - Make Angle constructors return a finite value.

https://reviewboard.mozilla.org/r/123832/#review126258

This feels a lot like whack-a-mole, and this patch doesn't fix `Angle` values that come from `calc()`, etc.

In https://github.com/servo/servo/pull/16144, I'm adding mandatory constructor functions for `Angle`, `Integer` and other primitives. I believe it'd be better to wait for that and do it in `Angle::from_radians` and `Angle::from_calc` instead, or in `Angle::to_computed_value`. wdyt?

::: servo/components/style/values/specified/mod.rs:355
(Diff revision 1)
>          match_ignore_ascii_case! { unit,
>              "deg" => Ok(Angle(value * RAD_PER_DEG)),
>              "grad" => Ok(Angle(value * RAD_PER_GRAD)),
> -            "turn" => Ok(Angle(value * RAD_PER_TURN)),
> +            "turn" => {
> +                // RAD_PER_TURN is 2.0 (large than 1.0), so |value * RAD_PER_TURN| might be inf.
> +                let turns = value * RAD_PER_TURN;

Also, for reference, I believe you could write this as `Angle(turns.min(f32::MAX).max(f32::MIN))`, which is a little nicer: https://is.gd/IWx0Qo
Attachment #8851520 - Flags: review?(emilio+bugs)
(Assignee)

Comment 11

a year ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Comment on attachment 8851520 [details]
> Bug 1336769 - Make parse_dimension() return a finite value.
> 
> https://reviewboard.mozilla.org/r/123832/#review126258
> 
> This feels a lot like whack-a-mole, and this patch doesn't fix `Angle`
> values that come from `calc()`, etc.
> 
> In https://github.com/servo/servo/pull/16144, I'm adding mandatory
> constructor functions for `Angle`, `Integer` and other primitives. I believe
> it'd be better to wait for that and do it in `Angle::from_radians` and
> `Angle::from_calc` instead, or in `Angle::to_computed_value`. wdyt?

OK, It sounds better. let's wait for that PR, and I will fix it as your suggestion.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a year ago
mozreview-review
Comment on attachment 8851520 [details]
Bug 1336769 - Make Angle constructors return a finite value.

https://reviewboard.mozilla.org/r/123832/#review127142

Much cleaner :)
Attachment #8851520 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 15

a year ago
Thanks, emilio. However, we still need to use STYLO-THREADS=1 to pass the crashtests, so I will merge the servo side first, and hold the gecko change until we fix the assertion mentioned in comment 2 (bug 1350244).
(Reporter)

Comment 16

a year ago
(In reply to Boris Chiou [:boris] from comment #15)
> Thanks, emilio. However, we still need to use STYLO-THREADS=1 to pass the
> crashtests, so I will merge the servo side first, and hold the gecko change
> until we fix the assertion mentioned in comment 2 (bug 1350244).

Did you push the patches to try?  I think you don't need to wait for that bug. AFAIK, we run stylo tests on one thread.
(Assignee)

Comment 17

a year ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> (In reply to Boris Chiou [:boris] from comment #15)
> > Thanks, emilio. However, we still need to use STYLO-THREADS=1 to pass the
> > crashtests, so I will merge the servo side first, and hold the gecko change
> > until we fix the assertion mentioned in comment 2 (bug 1350244).
> 
> Did you push the patches to try?  I think you don't need to wait for that
> bug. AFAIK, we run stylo tests on one thread.

Not yet, I only tried the first patch (servo side). Let me push these patches to try together. (Sorry, I thought we need to make sure both local and try server tests successfully.)
(Assignee)

Updated

a year ago
Attachment #8851520 - Attachment is obsolete: true
(Assignee)

Comment 18

a year ago
Created attachment 8852409 [details] [review]
Servo PR, #16178

Comment 19

a year ago
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d124e2cc2e68
Enable crashtests, 1272475-1.html and 1272475-2.html. r=heycam

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d124e2cc2e68
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Updated

a year ago
Blocks: 1324693
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.