Closed Bug 1460158 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: tsmith, Assigned: mats)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase)

Attachments

(5 files)

Attached file 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)
Flags: in-testsuite?
The assertion was introduced in bug 1458028.
Blocks: 1458028
Flags: needinfo?(mats)
Keywords: regression
Priority: -- → P1
Attached file frame tree
Oh, nice, the assertion caught a bug.
Flags: needinfo?(mats)
Assignee: nobody → mats
To be clear, this is an existing bug already before the assertion was added in bug 1458028.
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?
Flags: needinfo?(xidorn+moz)
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?
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.
Flags: needinfo?(xidorn+moz)
(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).
Attachment #8975527 - Attachment mime type: text/plain → text/html
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."
(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.
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...
(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...
Speccing HTML <details> in terms of Shadow DOM in the first place
seems just plain wrong to me.
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?
Attachment #8975293 - Flags: review?(xidorn+moz) → review+
Mats, why does that seem wrong?
(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.
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.
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.
(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.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6171dc6da6
Ignore non-primary child frames when searching for the main <summary> frame.  r=xidorn
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/7d6171dc6da6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Is there a user impact here which warrants uplift consideration or can this ride the 62 train?
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.
Flags: needinfo?(mats)
You need to log in before you can comment on or make changes to this bug.