Closed
Bug 1336769
Opened 8 years ago
Closed 8 years ago
stylo: 1272475-1.html and 1272475-2.html fails with Assertion failure: !mozilla::IsNaN(mValue.mFloat)
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: hiro, Assigned: boris)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
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•8 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
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → boris.chiou
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 2•8 years 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•8 years ago
|
||
It's bug 1350244.
Reporter | ||
Comment 4•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years ago
|
Attachment #8851520 -
Attachment is obsolete: true
Assignee | ||
Comment 18•8 years ago
|
||
Comment 19•8 years 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
status-firefox54:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•