1460158 - Assertion failure: firstChild && firstChild->IsPrimaryFrame() (this is probably not the frame you were looking for), at src/layout/generic/DetailsFrame.cpp:145

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Assertion failure: firstChild && firstChild->IsPrimaryFrame() (this is probably not the frame you were looking for), at src/layout/generic/DetailsFrame.cpp:145

#0 mozilla::DetailsFrame::HasMainSummaryFrame(nsIFrame*) src/layout/generic/DetailsFrame.cpp:144:7
#1 nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval(nsIFrame*) src/layout/base/nsCSSFrameConstructor.cpp:8904:34
#2 nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) src/layout/base/nsCSSFrameConstructor.cpp:8018:9
#3 mozilla::PresShell::ContentRemoved(nsIContent*, nsIContent*) src/layout/base/PresShell.cpp:4572:22
#4 nsNodeUtils::ContentRemoved(nsINode*, nsIContent*, nsIContent*) src/dom/base/nsNodeUtils.cpp:230:3
#5 nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) src/dom/base/nsINode.cpp:1664:5
#6 mozilla::dom::FragmentOrElement::RemoveChildAt_Deprecated(unsigned int, bool) src/dom/base/FragmentOrElement.cpp:1215:5
#7 nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:1975:18
#8 mozilla::dom::NodeBinding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/NodeBinding.cpp:944:45
#9 bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3260:13
#10 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/vm/JSContext-inl.h:280:15
#11 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:467:16
#12 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:516:12
#13 Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3086:18
#14 js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:417:12
#15 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:489:15
#16 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:516:12
#17 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:535:10
#18 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2989:12
#19 mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:264:37
#20 void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
#21 mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) src/dom/events/JSEventHandler.cpp:214:12
#22 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1121:52
#23 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*) src/dom/events/EventListenerManager.cpp:1288:20
#24 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:348:17
#25 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:528:16
#26 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:961:9
#27 nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1064:7
#28 nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:7246:21
#29 nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:7039:7
#30 non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp
#31 nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1315:3
#32 nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:858:14
#33 nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:747:9
#34 nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:632:5
#35 non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp
#36 mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:629:28
#37 nsIDocument::DoUnblockOnload() src/dom/base/nsDocument.cpp:8413:18
#38 nsDocument::UnblockOnload(bool) src/dom/base/nsDocument.cpp:8335:9
#39 nsIDocument::DispatchContentLoadedEvents() src/dom/base/nsDocument.cpp:5315:3
#40 mozilla::detail::RunnableMethodImpl<nsIDocument*, void (nsIDocument::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1216:13
#41 mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
#42 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1090:14
#43 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
#44 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#45 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:326:10
#46 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299:3
#47 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27
#48 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:893:22
#49 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:269:9
#50 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:326:10
#51 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299:3
#52 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:719:34
#53 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
#54 main src/browser/app/nsBrowserApp.cpp:282:18
#55 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#56 _start (firefox+0x423444)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

The assertion was introduced in bug 1458028.

Comment 2

[Mats Palmgren (inactive)]

Attachment: frame tree

Oh, nice, the assertion caught a bug.

Comment 3

[Mats Palmgren (inactive)]

To be clear, this is an existing bug already before the assertion was added in bug 1458028.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Ignore non-primary child frames when searching for the main <summary> frame

Comment 5

[Mats Palmgren (inactive)]

Attachment: ::before/::after testcase

BTW, it appears that we create a different box tree compared to
Chrome for ::before/::after pseudos on <details> and <summary>.
Do you know if any spec(s) say anything about this?

Comment 6

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89e8d27eaf4937b24269d4930e82e5a7fe8b106e&selectedJob=178185509

Comment 7

[Mats Palmgren (inactive)]

I found https://html.spec.whatwg.org/multipage/rendering.html#the-details-and-summary-elements
which I think says that the rendering in Gecko is the correct one.
Do you know if there's a bug filed on Chrome?

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Yeah, looks like Gecko's behavior is correct on the testcase.

I think we should submit a web-platform test and file a new issue to Chrome. I'm not aware of any of that.

Comment 9

[Mats Palmgren (inactive)]

OK, I filed https://bugs.chromium.org/p/chromium/issues/detail?id=842704 and
https://bugs.chromium.org/p/chromium/issues/detail?id=842713

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: details-shadow-dom.html

(In reply to Mats Palmgren (:mats) from comment #9)
> OK, I filed https://bugs.chromium.org/p/chromium/issues/detail?id=842704 and
> https://bugs.chromium.org/p/chromium/issues/detail?id=842713

Hmm, given <details> is spec'd in terms of Shadow DOM, I'd expect the correct rendering to be Chrome's... I think the spec is really confused here.

We'd get the same rendering if we used Shadow DOM to implement <details>, see the attached test-case (requires dom.webcomponents.shadowdom.enabled=true).

Comment 11

[Mats Palmgren (inactive)]

That seems like a spec bug to me.  I don't see why Shadow DOM should be
a requirement to implement <details>.  I also think our rendering
makes more sense; the ::before/::after should render together with
the rest of the <details> children, like it does on any other block.
The spec says:
https://html.spec.whatwg.org/multipage/rendering.html#the-details-and-summary-elements
"The details element is expected to render as a block box."

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #11)
> That seems like a spec bug to me.  I don't see why Shadow DOM should be
> a requirement to implement <details>.  I also think our rendering
> makes more sense; the ::before/::after should render together with
> the rest of the <details> children, like it does on any other block.
> The spec says:
> https://html.spec.whatwg.org/multipage/rendering.html#the-details-and-
> summary-elements
> "The details element is expected to render as a block box."

According to Anne, that "descendants" bit only applies to DOM descendants, not generated content, and there's no HTML spec bug in that sense.

I can see use cases for both renderings FWIW.

Comment 13

[Mats Palmgren (inactive)]

Well, ::before/::after content is pretty much always treated
as if they were actual DOM children.  That's the essence of
the generated content spec anyway:
https://www.w3.org/TR/css-pseudo-4/#generated-content
"these pseudo-elements generate boxes as if they were immediate
children of their originating element"
...
"::before
    Represents a styleable child pseudo-element immediately before
    the originating element’s actual content. "

So, details::before should render exactly the same as this renders:

<details>
details::before<summary>SUMMARY</summary>
DETAILS
</details>

Funny that Chrome actually get that test right, but the actual
::before test wrong...

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #13)
> Funny that Chrome actually get that test right, but the actual
> ::before test wrong...

FWIW, it's not by chance: Shadow DOM doesn't move generated content into <slot>s, only DOM children. Moving generated content around that way would be kind of a pain, I'd think...

Comment 15

[Mats Palmgren (inactive)]

Speccing HTML <details> in terms of Shadow DOM in the first place
seems just plain wrong to me.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8975293 [details] [diff] [review]
Ignore non-primary child frames when searching for the main <summary> frame

Review of attachment 8975293 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/crashtests/1460158-1.html
@@ +9,5 @@
> +  a.appendChild(b);
> +}
> +</script>
> +<body onload=go()>
> +<progress id="a"></progress>

It's not clear to me why is the <progress> element necessary in the crashtests. Can we just remove it and have go() function delete the element rather than moving it?

Comment 17

[Anne (:annevk)]

Mats, why does that seem wrong?

Comment 18

[Mats Palmgren (inactive)]

(In reply to Anne (:annevk) from comment #17)
> Mats, why does that seem wrong?

It's wrong for a few reasons...
1. UAs have been shipping <details> for a while now, but some don't
   ship Shadow DOM yet.  So the spec doesn't match reality.  (It might
   even be a breaking change since I doubt it was described in terms
   of Shadow DOM when we shipped it, but I don't know for sure.)
2. I don't think specs should demand a particular implementation
   technology like this.  Detailed descriptions are good but it
   should be possible for an UA to implement <details> without
   supporting Shadow DOM.
3. Shadow DOM is still evolving and seems experimental to me.
   Mature specifications like HTML should not make normative
   references to experimental technology which still has many
   unresolved technical issues (e.g. Selection, bug 1455894)

It's fine if HTML requires a particular CSS box hierarchy, but that
can be described without referring to a particular underlying
implementation technology.

Comment 19

[Mats Palmgren (inactive)]

Fwiw, I also think the current spec isn't very good in general
since it severely limits how authors can style these elements.
For example, the "take the element's remaining descendants, if any,
and place them in a second block box container." excludes the use
of 'display:grid' (etc) to layout the <details> contents in a grid.
I don't see any reason for this limitation; it seems like a spec bug
to me.

It's another indication that the CSS box construction for these
elements haven't been thought through properly.

Comment 20

[Anne (:annevk)]

Re comment 18:

1. I don't think that matters. You could implement this however you want as long as the end result is indistinguishable.
2. That is still allowed.
3. I think we're past that point. In fact in order to make shadow trees more mature many parts of HTML had to be modified. You cannot really keep it isolated.

Re comment 19: I'm pretty sure this was considered. Being able to use shadow trees to underpin a bunch of elements rather than having custom layout code for them was in fact a motivating factor to get shadow trees done. I tried to clarify the specification here so it's a little less vague: https://github.com/whatwg/html/pull/3686.

Comment 21

[Mats Palmgren (inactive)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #16)
> It's not clear to me why is the <progress> element necessary in the
> crashtests. Can we just remove it and have go() function delete the element
> rather than moving it?

Sure, I added a test that does removeChild directly.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6171dc6da6
Ignore non-primary child frames when searching for the main <summary> frame.  r=xidorn

Comment 23

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7d6171dc6da6

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact here which warrants uplift consideration or can this ride the 62 train?

Comment 25

[Mats Palmgren (inactive)]

No, I don't think this warrants uplift.  I doubt it will affect any real
web sites, and if it does it's a minor layout glitch.