Closed
Bug 1397091
Opened 7 years ago
Closed 7 years ago
stylo: panic in Servo_ResolveStyle on betcity.ru
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: bholley, Assigned: emilio)
References
Details
Attachments
(3 files)
Pulled this one from the crash reports in [1]. I get a reliable crash at https://betcity.ru/ru/line/competions/ft=1; [1] https://crash-stats.mozilla.com/signature/?product=Firefox&version=57.0a1&signature=core%3A%3Aoption%3A%3Aexpect_failed%20%7C%20geckoservo%3A%3Aglue%3A%3AServo_ResolveStyle&date=%3E%3D2017-08-30T01%3A15%3A50.000Z&date=%3C2017-09-06T01%3A15%3A50.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-build_id&_sort=-date&page=1
Comment 2•7 years ago
|
||
The site causes an assertion on debug build. ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle()', file /home/ikezoe/central/layout/base/RestyleManager.cpp, line 1135 CCing animation people.
Comment 3•7 years ago
|
||
This is regressed by bug 1374235. Also I guess this is somewhat related to bug 1395719 since I got sometimes this panic; thread '<unnamed>' panicked at 'Resolving style on <span class="logo-text-rs"> (0x7fffbeec4f70) without current styles: ElementData { styles: ElementStyles { primary: Some(Some(StrongRuleNode { p: 0x7fffd3bec470 })), pseudos: EagerPseudoStyles(None) }, restyle: RestyleData { hint: , flags: , damage: GeckoRestyleDamage(nsChangeHint(0)) } }', /home/ikezoe/central/servo/ports/geckolib/glue.rs:2923 Mostly the site crashes in TransformList.animate() [1], but it is a result of memory corruption happened before we reach there. [1] https://hg.mozilla.org/mozilla-central/file/f64e2b4dcf5e/servo/components/style/properties/helpers/animated_properties.mako.rs
Blocks: 1374235
Comment 4•7 years ago
|
||
Note that I can't reproduce the panic on normal browsing, but I can reliably reproduce the panic on private browsing mode somehow.
Comment 5•7 years ago
|
||
Assign to Emilio since he has in-progress patch for this. :) Awesome!
Assignee: hikezoe → emilio
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=adcd0fc5cd8921d56d97a286b16a4571dd0439fa
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Boris, same comment as for bug 1395719. This is on top of that one :) I'll try construct a test-case if there isn't any from the fuzzers (need to look).
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8905554 [details] Bug 1397091: Allow calling RecreateFramesForContent with async insertion for non-elements. https://reviewboard.mozilla.org/r/177346/#review182434 ::: commit-message-18a89:3 (Diff revision 1) > +Bug 1397091: Allow calling RecreateFramesForContent with async insertion for non-elements. > + > +Kind of bothers me to relax that assertion, but it's the easiest and less risky s/less/least/ I think relaxing the assertion is fine, since we now actually support the "async + non-element" case. The only reason the assertion was there was that we used to not support it! Feel free to rewrite/update the commit message accordingly. ::: layout/base/nsCSSFrameConstructor.cpp:10062 (Diff revision 1) > bool didReconstruct = > ContentRemoved(container, aContent, nextSibling, aInsertionKind, > REMOVE_FOR_RECONSTRUCTION); > > if (!didReconstruct) { > - if (aInsertionKind == InsertionKind::Async) { > + if (aInsertionKind == InsertionKind::Async && aContent->IsElement()) { This could use some comments now. Why do we even have the PostRestyleEvent path? Why can't we just always ContentRangeInserted and let lazy FC handle the async case? I _think_ the reason for that is that it's just cheaper to PostRestyleEvent here and then later coalesce things into lazy frame construction, instead of doing the up-front ContentRangeInserted work? But that's not actually all that clear to me. Anyway, we should either document why we have the two separate codepaths, or file a bug to remove the PostRestyleEvent codepath and comment with the bug number here, or something.
Attachment #8905554 -
Flags: review+
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8905555 [details] Bug 1397091: Merge InsertionKind and LazyConstructionAllowed. https://reviewboard.mozilla.org/r/177348/#review182438 r=me. It might be worth looking into eliminating the cases when MaybeConstructLazily returns false... Longer-term, though.
Attachment #8905555 -
Flags: review+
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8905579 [details] Remove aForReconstruct. https://reviewboard.mozilla.org/r/177386/#review182440 r=me ::: layout/base/nsCSSFrameConstructor.cpp:7666 (Diff revision 1) > } > return; > } > } > > - // We couldn't construct lazily. Make Servo eagerly traverse the new content. > + // We couldn't construct lazily. Make Servo eagerly traverse the new content This could use documentation about why aInsertionKind == InsertionKind::Async is the right thing to check: because it indicates that the styles may not be up to date yet... ::: layout/base/nsCSSFrameConstructor.cpp:8142 (Diff revision 1) > } > return; > } > } > > - // We couldn't construct lazily. Make Servo eagerly traverse the new content. > + // We couldn't construct lazily. Make Servo eagerly traverse the new content Similar comment here.
Attachment #8905579 -
Flags: review+
Comment 14•7 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e51ad4583913 Allow calling RecreateFramesForContent with async insertion for non-elements. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/cb5a5cf16f1b Merge InsertionKind and LazyConstructionAllowed. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3f27bf4b47 Remove aForReconstruct. r=bz
Comment 15•7 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0830048b6e3 Crashtest. r=me
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e51ad4583913 https://hg.mozilla.org/mozilla-central/rev/cb5a5cf16f1b https://hg.mozilla.org/mozilla-central/rev/1e3f27bf4b47 https://hg.mozilla.org/mozilla-central/rev/d0830048b6e3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Flags: needinfo?(bzbarsky)
You need to log in
before you can comment on or make changes to this bug.
Description
•