Closed Bug 1397091 Opened 2 years ago Closed 2 years ago

stylo: panic in Servo_ResolveStyle on betcity.ru

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: bholley, Assigned: emilio)

References

Details

Attachments

(3 files)

Hiro is going to take a look.
Priority: -- → P1
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.
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
Note that I can't reproduce the panic on normal browsing, but I can reliably reproduce the panic on private browsing mode somehow.
Assign to Emilio since he has in-progress patch for this. :) Awesome!
Assignee: hikezoe → emilio
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 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 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 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+
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
Duplicate of this bug: 1398400
Flags: needinfo?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.