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

VERIFIED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P1
normal
VERIFIED FIXED
5 months ago
4 months ago

People

(Reporter: truber, Assigned: emilio)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

unspecified
mozilla57
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Reporter)

Description

5 months ago
Created attachment 8903310 [details]
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*)
Flags: in-testsuite?
This isn't unfortunately fixed by bug 1394935.
I thought initially this bug was caused by bug 1383332, but it's not. It's bug 1389743.
Blocks: 1389743
Bug 1389743 might unveil an underlying issue.
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
Flags: needinfo?(emilio)
(Assignee)

Comment 5

5 months ago
It is. I'll take a look ASAP.
(Assignee)

Updated

5 months ago
Assignee: nobody → emilio
(Assignee)

Updated

5 months ago
Depends on: 1396018

Updated

5 months ago
Priority: -- → P1
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.
Crash Signature: [@ core::option::expect_failed | geckoservo::glue::Servo_ResolveStyle]
status-firefox55: --- → unaffected
status-firefox56: --- → ?
status-firefox57: --- → affected
status-firefox-esr52: --- → unaffected
(Assignee)

Comment 7

5 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

4 months ago
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.
Flags: needinfo?(emilio) → needinfo?(bzbarsky)
Comment on attachment 8905500 [details]
Bug 1395719: Make ReconstructDocElementHierarchy take an InsertionKind.

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

r=me
Attachment #8905500 - Flags: review+
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...
Attachment #8905501 - Flags: review+
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.
Attachment #8905502 - Flags: review+
Comment on attachment 8905503 [details]
Bug 1395719: Minimal cleanup when using the accessibility service.

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

r=me
Attachment #8905503 - Flags: review+

Comment 20

4 months ago
Pushed by ecoal95@gmail.com:
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
(Assignee)

Updated

4 months ago
Blocks: 1398041
status-firefox56: ? → unaffected
Flags: needinfo?(bzbarsky)
Flags: in-testsuite?
Flags: in-testsuite+
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.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

4 months ago
See Also: → bug 1402366
You need to log in before you can comment on or make changes to this bug.