1395719 - stylo: panicked at 'Resolving style on <mo> without current styles: ...'

Status: VERIFIED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes a panic in m-c rev 20170831-fb22415719a9 with stylo enabled by pref

thread '<unnamed>' panicked at 'Resolving style on <mo> (0x7f39c5505d40) without current styles: ElementData { styles: ElementStyles { primary: Some(Some(StrongRuleNode { p: 0x7f39de1b5790 })), pseudos: EagerPseudoStyles(None) }, restyle: RestyleData { hint: RESTYLE_SELF | RESTYLE_DESCENDANTS, flags: , damage: GeckoRestyleDamage(nsChangeHint(0)) } }', /builds/worker/workspace/build/src/servo/ports/geckolib/glue.rs:2900
stack backtrace:
   0:     0x7f39ea93bff3 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::hcab99e0793da62c7
   1:     0x7f39ea937316 - std::sys_common::backtrace::_print::hbfe5b0c7e79c0711
   2:     0x7f39ea94968a - std::panicking::default_hook::{{closure}}::h9ba2c6973907a2be
   3:     0x7f39ea94928b - std::panicking::default_hook::he4d55e2dd21c3cca
   4:     0x7f39ea949ada - std::panicking::rust_panic_with_hook::ha138c05cd33ad44d
   5:     0x7f39ea949974 - std::panicking::begin_panic::hcdbfa35c94142fa2
   6:     0x7f39ea9498a9 - std::panicking::begin_panic_fmt::hc09fe500d9b7be81
   7:     0x7f39ea273c67 - Servo_ResolveStyle
   8:     0x7f39e87f0719 - mozilla::ServoStyleSet::ResolveServoStyle(mozilla::dom::Element*)
   9:     0x7f39e8804024 - mozilla::ServoStyleSet::ResolveStyleFor(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::LazyComputeBehavior)
  10:     0x7f39e89084ba - nsCSSFrameConstructor::ResolveStyleContext(nsStyleContext*, nsIContent*, nsFrameConstructorState*, mozilla::dom::Element*)
  11:     0x7f39e8908822 - nsCSSFrameConstructor::ResolveStyleContext(nsIFrame*, nsIContent*, nsIContent*, nsFrameConstructorState*)
  12:     0x7f39e890916d - nsCSSFrameConstructor::ResolveStyleContext(nsCSSFrameConstructor::InsertionPoint const&, nsIContent*, nsFrameConstructorState*)
  13:     0x7f39e89260b4 - nsCSSFrameConstructor::AddFrameConstructionItems(nsFrameConstructorState&, nsIContent*, bool, nsCSSFrameConstructor::InsertionPoint const&, nsCSSFrameConstructor::FrameConstructionItemList&)
  14:     0x7f39e8927ba1 - nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*)
  15:     0x7f39e8928c5d - nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&)
  16:     0x7f39e892913c - nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&)
  17:     0x7f39e89292c2 - nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, bool, nsFrameItems&)
  18:     0x7f39e89278c3 - nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*)
  19:     0x7f39e8928c5d - nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&)
  20:     0x7f39e892913c - nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&)
  21:     0x7f39e89292c2 - nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, bool, nsFrameItems&)
  22:     0x7f39e892a17e - nsCSSFrameConstructor::ConstructInline(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&)
  23:     0x7f39e89284c3 - nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&)
  24:     0x7f39e892913c - nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&)
  25:     0x7f39e89292c2 - nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, bool, nsFrameItems&)
  26:     0x7f39e89278c3 - nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*)
  27:     0x7f39e892dd0d - nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsContainerFrame*, nsStyleContext*, nsContainerFrame**, nsFrameItems&, nsIFrame*, PendingBinding*)
  28:     0x7f39e892e937 - nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*)
  29:     0x7f39e892f486 - nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool, bool, TreeMatchContext*)
  30:     0x7f39e893105e - nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind, nsCSSFrameConstructor::RemoveFlags)
  31:     0x7f39e8932158 - nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval(nsIFrame*, nsCSSFrameConstructor::InsertionKind, nsCSSFrameConstructor::RemoveFlags)
  32:     0x7f39e8930fa5 - nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind, nsCSSFrameConstructor::RemoveFlags)
  33:     0x7f39e892f85f - nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool, bool, TreeMatchContext*)
  34:     0x7f39e893105e - nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind, nsCSSFrameConstructor::RemoveFlags)
  35:     0x7f39e8933205 - nsCSSFrameConstructor::CharacterDataChanged(nsIContent*, CharacterDataChangeInfo*)
  36:     0x7f39e88d8a33 - mozilla::PresShell::CharacterDataChanged(nsIDocument*, nsIContent*, CharacterDataChangeInfo*)
  37:     0x7f39e7822536 - nsNodeUtils::CharacterDataChanged(nsIContent*, CharacterDataChangeInfo*)
  38:     0x7f39e7819a71 - nsGenericDOMDataNode::SetTextInternal(unsigned int, unsigned int, char16_t const*, unsigned int, bool, CharacterDataChangeInfo::Details*)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

This isn't unfortunately fixed by bug 1394935.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I thought initially this bug was caused by bug 1383332, but it's not. It's bug 1389743.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Bug 1389743 might unveil an underlying issue.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Though I don't quite understand about bug 1389743, replacing |aInsertionKind| in  ReframeContainingBlock() [1] stops this panic, something like this.

--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -13203,7 +13203,8 @@ nsCSSFrameConstructor::ReframeContaining
         printf("  ==> blockContent=%p\n", static_cast<void*>(blockContent));
       }
 #endif
-      RecreateFramesForContent(blockContent->AsElement(), aInsertionKind,
+      RecreateFramesForContent(blockContent->AsElement(),
+                               InsertionKind::Async,
                                REMOVE_FOR_RECONSTRUCTION);
       return;
     }


Emilio, does this fact help to know what's going on in the test case?  Also, we don't use |aFlags| in ReframeContainingBlock(), is it intentional (it was replaced by REMOVE_FOR_RECONSTRUCTION)? If so, we can drop the argument from the function.

[1] https://hg.mozilla.org/mozilla-central/rev/5c0a8f5eb8c9#l7.991

Comment 5

[Emilio Cobos Álvarez (:emilio)]

It is. I'll take a look ASAP.

Comment 6

[Chris Peterson [:cpeterson]]

In bug 1394560 comment 13, Hiro said we see this same crash signature in some Nightly crash reports like bp-885c44ec-48ac-4814-bd86-721330170904.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

This is a debug assertion, so this can't crash on nightly. It's still not totally clear to me that the underlying issue is the same, but seems possible.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7291acab0d457a4a4a2eb7640721142080d0d63d

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1395719: Make ReconstructDocElementHierarchy take an InsertionKind.

Otherwise we may unexpectedly sync-construct it.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1395719: Convert aAllowLazyFrameConstruction to an enum class.

It's clearer what it means from the callsites.

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1395719: Reconstruct ancestors asynchronously when lazy frame construction is allowed.

The main purpose of this change is avoiding doing frame construction when the
style tree is not up-to-date.

In the test-case above, for example, a MathML element is changed, and we
schedule style invalidation for it, but another MathML element is inserted, and
we reconstruct the whole thing, using the out-to-date styles from the MathML
element.

There are other cases where this is more problematic than "we just happened to
use out-of-date styles, and we'll restyle eventually", which is when this
reconstruct happens to hit an element which was recently inserted but happened
to lazily construct.

In that case, we haven't even performed initial styling on the element, so we
just panic and crash.

After this the only sync frame construction case remaining is
CharacterDataChanged, which I'll address in bug 1397091.

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1395719: Minimal cleanup when using the accessibility service.

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1395719: Crashtest.

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Boris, let me know if you have the time to review these. I think you're the most suitable one to do so given our conversation yesterday is hopefully fresh enough ;). But I can also redirect to Cam or Mats I guess.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8905500 [details]
Bug 1395719: Make ReconstructDocElementHierarchy take an InsertionKind.

https://reviewboard.mozilla.org/r/177318/#review182420

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8905501 [details]
Bug 1395719: Convert aAllowLazyFrameConstruction to an enum class.

https://reviewboard.mozilla.org/r/177320/#review182422

r=me

::: layout/base/PresShell.cpp:1818
(Diff revision 1)
>        mFrameConstructor->BeginUpdate();
>  
>        // Have the style sheet processor construct frame for the root
>        // content object down
> -      mFrameConstructor->ContentInserted(nullptr, root, nullptr, false);
> +      mFrameConstructor->ContentInserted(
> +          nullptr, root, nullptr, nsCSSFrameConstructor::LazyConstructionAllowed::No);

This is over 80 chars.  Please put the last arg on a separate line.

::: layout/base/nsCSSFrameConstructor.cpp:7471
(Diff revision 1)
>  
>  nsCSSFrameConstructor::InsertionPoint
>  nsCSSFrameConstructor::GetRangeInsertionPoint(nsIContent* aContainer,
>                                                nsIContent* aStartChild,
>                                                nsIContent* aEndChild,
> -                                              bool aAllowLazyConstruction,
> +                                              LazyConstructionAllowed aLazyConstructionAllowed,

This might be over 80 chars too...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8905502 [details]
Bug 1395719: Reconstruct ancestors asynchronously when lazy frame construction is allowed.

https://reviewboard.mozilla.org/r/177322/#review182424

::: commit-message-e245b:19
(Diff revision 1)
> +to lazily construct.
> +
> +In that case, we haven't even performed initial styling on the element, so we
> +just panic and crash.
> +
> +After this the only sync frame construction case remaining is

You mean the only case remaining when we might not have up-to-date style info, right?

::: layout/base/nsCSSFrameConstructor.h:1943
(Diff revision 1)
>    // aIsAppend is true, then the caller MUST call
>    // nsCSSFrameConstructor::AppendFramesToParent (as opposed to
>    // nsFrameManager::InsertFrames directly) to add the new frames.
>    // @return true if we reconstructed the containing block, false
>    // otherwise
>    bool WipeContainingBlock(nsFrameConstructorState& aState,

So WipeContainingBlock used to always do async reconstructs, right?  And now it might end up doing sync ones in some cases?

This worries me; it can have some nasty performance implications.  For example, say we're reframing a bunch of kids of something that would end up doing WipeContainingBlock.  Will we now end up doing multiple wipes?  I'm not actually sure; I'd need to either audit code or step through in a debugger.

Since this part is not actually needed for the change you're really making (which is eliminating sync cases), it would be best to split it into a separate bug, for regression-tracking purposes.  You can leave all the changes in this bug, just pass "Async" at the callsites of WipeContainingBlock for now, and update to passing the "for ancestor" thing in the separate bug.

::: layout/base/nsCSSFrameConstructor.cpp:7602
(Diff revision 1)
>    MOZ_ASSERT(!aProvidedTreeMatchContext ||
>               aLazyConstructionAllowed == LazyConstructionAllowed::No);
>    MOZ_ASSERT(aLazyConstructionAllowed == LazyConstructionAllowed::No ||
>               !RestyleManager()->IsInStyleRefresh());
>  
> +  const InsertionKind insertionKind =

Maybe call this insertionKindForAncestorReframe?  This is not the insertionKind we plan to use for ourselves, after all.

::: layout/base/nsCSSFrameConstructor.cpp:8029
(Diff revision 1)
>    NS_PRECONDITION(mUpdateCount != 0,
>                    "Should be in an update while creating frames");
>  
>    NS_PRECONDITION(aStartChild, "must always pass a child");
>  
> +  const InsertionKind insertionKind =

Again, insertionKindForAncestorReframe.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8905503 [details]
Bug 1395719: Minimal cleanup when using the accessibility service.

https://reviewboard.mozilla.org/r/177324/#review182430

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8905504 [details]
Bug 1395719: Crashtest.

https://reviewboard.mozilla.org/r/177326/#review182432

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f432a118c6e0
Make ReconstructDocElementHierarchy take an InsertionKind. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/08951d83c938
Convert aAllowLazyFrameConstruction to an enum class. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/45f8148d33c8
Reconstruct ancestors asynchronously when lazy frame construction is allowed. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae543bcdc2f
Minimal cleanup when using the accessibility service. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/dee58c528463
Crashtest. r=bz

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f432a118c6e0
https://hg.mozilla.org/mozilla-central/rev/08951d83c938
https://hg.mozilla.org/mozilla-central/rev/45f8148d33c8
https://hg.mozilla.org/mozilla-central/rev/4ae543bcdc2f
https://hg.mozilla.org/mozilla-central/rev/dee58c528463

Comment 22

[Nicholas Nethercote [inactive]]

Good news: this signature was the Windows #8 topcrash in Nightly 20170908100218, which was the last Nightly before these patches landed, but hasn't been seen since.