1401256 - stylo: thread '<unnamed>' panicked at 'should check whether we're a non-inherited property'

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev 20170919-e4261f5b96eb.

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: minidump_stack.txt

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This assertion was added recently in bug 1367635.  

It adds a `for_non_inherited_property` property to the computed value context, and asserts that it is correct for `ExtremumLength`s like `-moz-max-content` from the test case.

For the regular styling path, this appears to be handled correctly, but for animations there are code paths that don't set the correct state in the context.

Should have a patch soon.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1401256 - Crashtest for animating lengths.

Review commit: https://reviewboard.mozilla.org/r/181864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181864/

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1401256 - Update inherited prop state for animation.

`AnimationValue` has its own path for conversion to computed values, so we need
to update `for_non_inherited_property` in the context to match the property
being converted.

I didn't notice any other generic conversions, so hopefully this is the only
missing path.

Review commit: https://reviewboard.mozilla.org/r/181866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181866/

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b45b072757b8f54d02a30e5c12d5e9e056341b1

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8910432 [details]
Bug 1401256 - Crashtest for animating lengths.

https://reviewboard.mozilla.org/r/181864/#review187316

::: layout/style/crashtests/1401256.html:4
(Diff revision 1)
> +<script>
> +  let o1 = document.createElement('p');
> +  document.documentElement.appendChild(o1);
> +  o1.animate({'':[''],'minWidth':['-moz-max-content']});

Curious why the '':[''] is needed.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8910433 [details]
Bug 1401256 - Update inherited prop state for animation.

https://reviewboard.mozilla.org/r/181866/#review187318

Thanks!

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://github.com/servo/servo/pull/18593

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8910432 [details]
Bug 1401256 - Crashtest for animating lengths.

https://reviewboard.mozilla.org/r/181864/#review187316

> Curious why the '':[''] is needed.

Ah, I missed this when borrowing from the fuzzer output.  It doesn't affect the result, so I'll remove it.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8910432 [details]
Bug 1401256 - Crashtest for animating lengths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181864/diff/1-2/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> Comment on attachment 8910432 [details]
> Bug 1401256 - Crashtest for animating lengths.
> 
> https://reviewboard.mozilla.org/r/181864/#review187316
> 
> > Curious why the '':[''] is needed.
> 
> Ah, I missed this when borrowing from the fuzzer output.  It doesn't affect
> the result, so I'll remove it.

That's good to know. I was also wondering how interact with the length property and was going to investigate it later.

Comment 12

[Bobby Holley (:bholley)]

Requesting tracking on all outstanding p2 stylo bugs.

Comment 13

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1422deda0bcf -d 443fe7c5dd42: rebasing 421951:1422deda0bcf "Bug 1401256 - Crashtest for animating lengths. r=heycam" (tip)
merging layout/style/crashtests/crashtests.list
warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8910432 [details]
Bug 1401256 - Crashtest for animating lengths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181864/diff/2-3/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34d60bb9e16b
Crashtest for animating lengths. r=heycam

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Servo portion previously landed as:

https://hg.mozilla.org/integration/autoland/rev/443fe7c5dd42bb6207fb778f58f94354d19fac28

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/34d60bb9e16b

Comment 18

[Sylvestre Ledru [:Sylvestre]]

We don't want to crash, tracking.
Can we have an uplift request?
Thanks

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Servo portion

Approval Request Comment
[Feature/Bug causing the regression]: Stylo bug 1367635
[User impact if declined]: If declined, animations that process certain properties might not be styled correctly.  Debug builds would crash the content process.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Unifies some flag setting in the style system's animations path.  The same thing was already being done for regular styling.
[String changes made/needed]: None

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8910432 [details]
Bug 1401256 - Crashtest for animating lengths.

Approval Request Comment

See comment 19.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19)
> Created attachment 8911291 [details]
> Servo portion
> 
> Approval Request Comment
> [Feature/Bug causing the regression]: Stylo bug 1367635
> [User impact if declined]: If declined, animations that process certain
> properties might not be styled correctly.  Debug builds would crash the
> content process.

This is not right, fwiw. There's no correctness issue, since the for_non_inherited_property bit is only useful for the rule cache, which isn't used for animations.

That being said the fix is trivial enough that uplifting should be ok.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #21)
> That being said the fix is trivial enough that uplifting should be ok.

Yeah. In general I think it's worth uplifting all/most targeted fuzz bug fixes in the first few weeks of the beta cycle, as long as they're not particularly risky.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8911291 [details]
Servo portion

Fix a crash, let's take it.
Should be in 57b3.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/61dd4d2e2a78
https://hg.mozilla.org/releases/mozilla-beta/rev/46a47d4a1f5c

Comment 25

[Andrei Vaida [:avaida]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on J. Ryan's assessment on manual testing needs and the fact that this fix has automated coverage.