Closed Bug 1401256 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: jkratzer, Assigned: jryans)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase)

Attachments

(4 files, 1 obsolete file)

Attached file trigger.html (deleted) —
Testcase found while fuzzing mozilla-central rev 20170919-e4261f5b96eb.
Attached file minidump_stack.txt (deleted) —
Priority: -- → P2
Assignee: nobody → jryans
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.
Blocks: 1367635
Keywords: regression
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.
Attachment #8910432 - Flags: review?(cam) → review+
Comment on attachment 8910433 [details] Bug 1401256 - Update inherited prop state for animation. https://reviewboard.mozilla.org/r/181866/#review187318 Thanks!
Attachment #8910433 - Flags: review?(cam) → review+
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.
(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.
Requesting tracking on all outstanding p2 stylo bugs.
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)
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/34d60bb9e16b Crashtest for animating lengths. r=heycam
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
We don't want to crash, tracking. Can we have an uplift request? Thanks
Flags: needinfo?(jryans)
Attached file Servo portion (deleted) —
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
Flags: needinfo?(jryans)
Attachment #8911291 - Flags: approval-mozilla-beta?
Comment on attachment 8910432 [details] Bug 1401256 - Crashtest for animating lengths. Approval Request Comment See comment 19.
Attachment #8910432 - Flags: approval-mozilla-beta?
(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.
(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 on attachment 8911291 [details] Servo portion Fix a crash, let's take it. Should be in 57b3.
Attachment #8911291 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8910432 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: