1497601 - Crash when attempting to land a Custom Element definition for XUL label: "panicked at 'Resolving style on unstyled element'"

Status: RESOLVED FIXED

(Toolkit :: XUL Widgets, defect, P3)

Description

[Brian Grinstead [:bgrins]]

When attempting to migration <label> to a Custom Element I see crashes on talos tests like tresize (https://treeherder.mozilla.org/logviewer.html#?job_id=204090867&repo=try&lineNumber=1731):

14:10:03     INFO -  TEST-INFO | started process 24158 (/home/cltbld/workspace/build/application/firefox/firefox -profile /tmp/tmpv2x6iR/profile http://localhost:33163/startup_test/tresize/addon/content/tresize-test.html)
14:10:04     INFO -  PID 24158 | thread '<unnamed>' panicked at 'Resolving style on unstyled element', libcore/option.rs:989:5
14:10:04     INFO -  PID 24158 | stack backtrace:
14:10:04     INFO -  PID 24158 |    0:     0x7f5570191262 - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::hced3a4ed290bc7f5
14:10:04     INFO -  PID 24158 |    1:     0x7f55701833ee - std::panicking::default_hook::{{closure}}::h15181aeeb7b4bc75
14:10:04     INFO -  PID 24158 |    2:     0x7f55701828ff - std::panicking::default_hook::h63ab9a1a074f1506
14:10:04     INFO -  PID 24158 |    3:     0x7f557018251e - std::panicking::rust_panic_with_hook::hba05450791beb933
14:10:04     INFO -  PID 24158 |    4:     0x7f55701844cd - std::panicking::continue_panic_fmt::hd2a4220d2e2291e2
14:10:04     INFO -  PID 24158 |    5:     0x7f5570184475 - rust_begin_unwind
14:10:04     INFO -  PID 24158 |    6:     0x7f55701a077b - core::panicking::panic_fmt::h2c383e888dce42c0
14:10:04     INFO -  PID 24158 |    7:     0x7f55701a0881 - core::option::expect_failed::h7d982926f8026caa
14:10:04     INFO -  PID 24158 |    8:     0x7f556fedd9cb - Servo_ResolveStyle
14:10:04     INFO -  PID 24158 |    9:     0x7f556e3394dc - _ZN21nsCSSFrameConstructor20ResolveComputedStyleEP10nsIContent
14:10:04     INFO -  PID 24158 |   10:     0x7f556e333e7f - _ZN21nsCSSFrameConstructor34ConstructAnonymousContentForCanvasER23nsFrameConstructorStateP8nsIFrameP10nsIContent
14:10:04     INFO -  PID 24158 |   11:     0x7f556e332f58 - _ZN21nsCSSFrameConstructor24ConstructDocElementFrameEPN7mozilla3dom7ElementEP21nsILayoutHistoryState
14:10:04     INFO -  PID 24158 |   12:     0x7f556e33d4b3 - _ZN21nsCSSFrameConstructor20ContentRangeInsertedEP10nsIContentS1_P21nsILayoutHistoryStateNS_13InsertionKindE
14:10:04     INFO -  PID 24158 |   13:     0x7f556e307b85 - _ZN7mozilla9PresShell10InitializeEv
14:10:04     INFO -  PID 24158 |   14:     0x7f556ce9850c - _ZN13nsContentSink11StartLayoutEb
14:10:04     INFO -  PID 24158 |   15:     0x7f556e01c371 - _ZN16nsXMLContentSink18HandleStartElementEPKDsPS1_jjjb
14:10:04     INFO -  PID 24158 |   16:     0x7f556e01c48a - _ZThn200_N16nsXMLContentSink18HandleStartElementEPKDsPS1_jjj
14:10:04     INFO -  PID 24158 |   17:     0x7f556c9bb1a1 - _ZN13nsExpatDriver18HandleStartElementEPKDsPS1_
14:10:04     INFO -  PID 24158 |   18:     0x7f556e946c81 - doContent
14:10:04     INFO -  PID 24158 |   19:     0x7f556e944144 - doProlog
14:10:04     INFO -  PID 24158 |   20:     0x7f556e941287 - prologInitProcessor
14:10:04     INFO -  PID 24158 |   21:     0x7f556e940b9d - MOZ_XML_Parse
14:10:04     INFO -  PID 24158 |   22:     0x7f556c9bca47 - _ZN13nsExpatDriver11ParseBufferEPKDsjbPj
14:10:04     INFO -  PID 24158 |   23:     0x7f556c9bcdeb - _ZN13nsExpatDriver12ConsumeTokenER9nsScannerRb
14:10:04     INFO -  PID 24158 |   24:     0x7f556c9c0102 - _ZN8nsParser8TokenizeEb
14:10:04     INFO -  PID 24158 |   25:     0x7f556c9bf219 - _ZN8nsParser11ResumeParseEbbb
14:10:04     INFO -  PID 24158 |   26:     0x7f556c9c05af - _ZN8nsParser15OnDataAvailableEP10nsIRequestP11nsISupportsP14nsIInputStreammj
14:10:04     INFO -  PID 24158 |   27:     0x7f556c830c23 - _ZN12nsJARChannel15OnDataAvailableEP10nsIRequestP11nsISupportsP14nsIInputStreammj
14:10:04     INFO -  PID 24158 |   28:     0x7f556c1698bc - _ZN17nsInputStreamPump15OnStateTransferEv
14:10:04     INFO -  PID 24158 |   29:     0x7f556c169443 - _ZN17nsInputStreamPump18OnInputStreamReadyEP19nsIAsyncInputStream
14:10:04     INFO -  PID 24158 |   30:     0x7f556c0bcab0 - _ZN23nsInputStreamReadyEvent3RunEv
14:10:04     INFO -  PID 24158 |   31:     0x7f556c0dc80e - _ZN8nsThread16ProcessNextEventEbPb
14:10:04     INFO -  PID 24158 |   32:     0x7f556c0de837 - _Z19NS_ProcessNextEventP9nsIThreadb
14:10:04     INFO -  PID 24158 |   33:     0x7f556c51c4ed - _ZN7mozilla3ipc11MessagePump3RunEPN4base11MessagePump8DelegateE
14:10:04     INFO -  PID 24158 |   34:     0x7f556c4ed55b - _ZN11MessageLoop3RunEv
14:10:04     INFO -  PID 24158 |   35:     0x7f556e185618 - _ZN14nsBaseAppShell3RunEv
14:10:04     INFO -  PID 24158 |   36:     0x7f556f1ad82d - _ZN12nsAppStartup3RunEv
14:10:04     INFO -  PID 24158 |   37:     0x7f556f25c0df - _ZN7XREMain11XRE_mainRunEv
14:10:04     INFO -  PID 24158 |   38:     0x7f556f25c943 - _ZN7XREMain8XRE_mainEiPPcRKN7mozilla15BootstrapConfigE
14:10:04     INFO -  PID 24158 |   39:     0x7f556f25cf28 - _Z8XRE_mainiPPcRKN7mozilla15BootstrapConfigE
14:10:04     INFO -  PID 24158 |   40:     0x55a8fc31bcab - main
14:10:04     INFO -  PID 24158 |   41:     0x7f557b10882f - __libc_start_main

I think this is due to us creating a XUL label in the NAC for a XUL tooltip (bug 1461798). If I switch the XUL label created here https://searchfox.org/mozilla-central/rev/29aea2a2a3bd0f5e25ce0b60a76053fb25ba5149/dom/xul/XULTooltipElement.cpp#30 to XUL description (which won't have a CE attached), that makes the crash go away locally.

I'm thinking we should either stop using xul:label inside of tooltip NAC (possibly using xul:description or html:label instead), or somehow skip whatever Custom Element logic is causing the panic for content in NAC.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

If you have a patch and a test-case I'd be interested in debugging this, but yeah, chances are that we're running the CE reactions after sync-styling the anonymous content.

We should just forbid any kind of JS-running during frame construction fwiw... :(

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I bet removing this would make the crash go away as well:

  https://hg.mozilla.org/try/rev/f30aa7b51e315ae09231f1e4a333546e332a12d5#l4.182

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1497601 - Test case for crash with empty Custom Element on label

Run with `./mach talos-test -a tresize`

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)
> I bet removing this would make the crash go away as well:
> 
>  
> https://hg.mozilla.org/try/rev/f30aa7b51e315ae09231f1e4a333546e332a12d5#l4.
> 182

That's what I thought too - but I just uploaded a version that registers an empty CE class and it reproduces in `./mach talos-test -a tresize` (the first time the window opens it succeeds on my computer, and the crash happens on the second time).

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)
> If you have a patch and a test-case I'd be interested in debugging this, but
> yeah, chances are that we're running the CE reactions after sync-styling the
> anonymous content.
> 
> We should just forbid any kind of JS-running during frame construction
> fwiw... :(

Would there be some way to either ignore CE reactions entirely inside of NAC, or at least assert when they are happening so we don't accidentally introduce CE into NAC?

Comment 6

[Olli Pettay [:smaug]]

hmm, I thought we explicitly disabled CEs in NAC. Was that check removed at some point.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #6)
> hmm, I thought we explicitly disabled CEs in NAC. Was that check removed at
> some point.

How does that work? By the time the element is created, it doesn't know whether it's in a NAC subtree, right?

Comment 8

[Olli Pettay [:smaug]]

sure, in sync case. But in async case that shouldn't an issue.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: Bug 1497601 - Use a xul:description instead of a xul:label for tooltips;r=smaug

We'd like to be able to implement label features with a Custom Element, and we
don't want to run CE reactions inside of the NAC.

Comment 10

[Brian Grinstead [:bgrins]]

I pushed up a patch to start using xul:description inside of tooltip. It seems to work fine from some manual testing, but are there any cases where we rely on xul:label layout features in tooltips?

Comment 11

[Brian Grinstead [:bgrins]]

If we want to go this route, I guess we should also update all the <label class="tooltip-label"> elements in XUL docs to be <description class="tooltip-label">: https://searchfox.org/mozilla-central/search?q=tooltip-label&path=

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I took a look at this. We're creating the tooltip in:

  chrome://global/content/backgroundPageThumbs.xhtml

GetEntryGlobal() returns null in:

  https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/dom/base/nsContentUtils.cpp#10082

It's not a XUL document, so we don't grab the scope object.

Since there's no global the tooltip construction fails, we bail out, and end up trying to construct frames for the unstyled nodes that we had already appended before... So that's basically bug 1482421.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Note that the label we're creating is:

  https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/dom/xul/XULTooltipElement.cpp#37

From the following stack:

#0  0x00007f08c7973e77 in nsContentUtils::NewXULOrHTMLElement(mozilla::dom::Element**, mozilla::dom::NodeInfo*, mozilla::dom::FromParser, nsAtom*, mozilla::dom::CustomElementDefinition*) (aResult=0x7ffdb805d208, aNodeInfo=0x7f089ba83380, aFromParser=mozilla::dom::NOT_FROM_PARSER, aIsAtom=0x0, aDefinition=0x0) at /home/emilio/src/moz/gecko-2/dom/base/nsContentUtils.cpp:10091
#1  0x00007f08ca28666c in NS_NewXULElement(mozilla::dom::Element**, already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser, nsAtom*, mozilla::dom::CustomElementDefinition*) (aResult=0x7ffdb805d208, aNodeInfo=..., aFromParser=mozilla::dom::NOT_FROM_PARSER, aIsAtom=0x0, aDefinition=0x0) at /home/emilio/src/moz/gecko-2/dom/xul/nsXULElement.cpp:283
#2  0x00007f08ca286322 in mozilla::dom::XULTooltipElement::Init() (this=0x7f08a4f8c670) at /home/emilio/src/moz/gecko-2/dom/xul/XULTooltipElement.cpp:35
#3  0x00007f08ca286164 in mozilla::dom::NS_NewXULTooltipElement(already_AddRefed<mozilla::dom::NodeInfo>&&) (aNodeInfo=...) at /home/emilio/src/moz/gecko-2/dom/xul/XULTooltipElement.cpp:20
#4  0x00007f08ca28f202 in nsXULElement::Construct(already_AddRefed<mozilla::dom::NodeInfo>&&) (aNodeInfo=...) at /home/emilio/src/moz/gecko-2/dom/xul/nsXULElement.cpp:161
#5  0x00007f08c79744bb in nsContentUtils::NewXULOrHTMLElement(mozilla::dom::Element**, mozilla::dom::NodeInfo*, mozilla::dom::FromParser, nsAtom*, mozilla::dom::CustomElementDefinition*) (aResult=0x7f089ba81170, aNodeInfo=0x7f089ba83300, aFromParser=mozilla::dom::NOT_FROM_PARSER, aIsAtom=0x0, aDefinition=0x0) at /home/emilio/src/moz/gecko-2/dom/base/nsContentUtils.cpp:10166
#6  0x00007f08ca28666c in NS_NewXULElement(mozilla::dom::Element**, already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser, nsAtom*, mozilla::dom::CustomElementDefinition*) (aResult=0x7f089ba81170, aNodeInfo=..., aFromParser=mozilla::dom::NOT_FROM_PARSER, aIsAtom=0x0, aDefinition=0x0) at /home/emilio/src/moz/gecko-2/dom/xul/nsXULElement.cpp:283
#7  0x00007f08ca9bb221 in nsCanvasFrame::CreateAnonymousContent(nsTArray<nsIAnonymousContentCreator::ContentInfo>&) (this=0x7f089ba810b0, aElements=nsTArray<nsIAnonymousContentCreator::ContentInfo> & = {...}) at /home/emilio/src/moz/gecko-2/layout/generic/nsCanvasFrame.cpp:178
#8  0x00007f08ca8c0ffa in nsCSSFrameConstructor::GetAnonymousContent(nsIContent*, nsIFrame*, nsTArray<nsIAnonymousContentCreator::ContentInfo>&) (this=0x7f089bd12300, aParent=0x7f089ba24550, aParentFrame=0x7f089ba810b0, aContent=nsTArray<nsIAnonymousContentCreator::ContentInfo> & = {...}) at /home/emilio/src/moz/gecko-2/layout/base/nsCSSFrameConstructor.cpp:4053
#9  0x00007f08ca8c03e8 in nsCSSFrameConstructor::ConstructAnonymousContentForCanvas(nsFrameConstructorState&, nsIFrame*, nsIContent*) (this=0x7f089bd12300, aState=..., aFrame=0x7f089ba810b0, aDocElement=0x7f089ba24550) at /home/emilio/src/moz/gecko-2/layout/base/nsCSSFrameConstructor.cpp:2895
#10 0x00007f08ca8bf312 in nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) (this=0x7f089bd12300, aDocElement=0x7f089ba24550, aFrameState=0x0)
    at /home/emilio/src/moz/gecko-2/layout/base/nsCSSFrameConstructor.cpp:2633
#11 0x00007f08ca8cddd4 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, nsCSSFrameConstructor::InsertionKind) (this=0x7f089bd12300, aStartChild=0x7f089ba24550, aEndChild=0x0, aFrameState=0x0, aInsertionKind=nsCSSFrameConstructor::InsertionKind::Sync) at /home/emilio/src/moz/gecko-2/layout/base/nsCSSFrameConstructor.cpp:7321
#12 0x00007f08ca8cd26b in nsCSSFrameConstructor::ContentInserted(nsIContent*, nsILayoutHistoryState*, nsCSSFrameConstructor::InsertionKind) (this=0x7f089bd12300, aChild=0x7f089ba24550, aFrameState=0x0, aInsertionKind=nsCSSFrameConstructor::InsertionKind::Sync) at /home/emilio/src/moz/gecko-2/layout/base/nsCSSFrameConstructor.cpp:7231
#13 0x00007f08ca85fc39 in mozilla::PresShell::Initialize() (this=0x7f089ba7e000) at /home/emilio/src/moz/gecko-2/layout/base/PresShell.cpp:1808
#14 0x00007f08c7ba775a in nsContentSink::StartLayout(bool) (this=0x7f089ba6d000, aIgnorePendingSheets=false) at /home/emilio/src/moz/gecko-2/dom/base/nsContentSink.cpp:1276
#15 0x00007f08ca1e78af in nsXMLContentSink::MaybeStartLayout(bool) (this=0x7f089ba6d000, aIgnorePendingSheets=false) at /home/emilio/src/moz/gecko-2/dom/xml/nsXMLContentSink.cpp:932

Comment 14

[Brian Grinstead [:bgrins]]

Is there a way to somehow completely ignore CE definitions / reactions from within NAC? We don't actually need any of the Custom Element behavior in this case (or presumably in any case since we don't want them to happen).

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Hmm, it's somewhat hard without tagging all the callers around or something... By the time we're creating that element we still don't know we're going to be NAC.

Comment 16

[Brian Grinstead [:bgrins]]

I didn't see anywhere we used label features, so I pushed up a version that uses description everywhere.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2e35ed6a692
Use a xul:description instead of a xul:label for tooltips;r=smaug

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f2e35ed6a692