1474768 - Assertion failure: !noPrimaryFrame (Ancestors of nodes with frames to be constructed lazily should have frames), at src/layout/base/nsCSSFrameConstructor.cpp:6809

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20180710230738
SourceStamp=3edc9c3ae818490ed36b8bfc8ffdfc9e222b41db

Assertion failure: !noPrimaryFrame (Ancestors of nodes with frames to be constructed lazily should have frames), at src/layout/base/nsCSSFrameConstructor.cpp:6809

#0 nsCSSFrameConstructor::CheckBitsForLazyFrameConstruction(nsIContent*) src/layout/base/nsCSSFrameConstructor.cpp:6810:3
#1 nsCSSFrameConstructor::MaybeConstructLazily(nsCSSFrameConstructor::Operation, nsIContent*) src/layout/base/nsCSSFrameConstructor.cpp:6883:3
#2 nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, nsCSSFrameConstructor::InsertionKind) src/layout/base/nsCSSFrameConstructor.cpp:7553:9
#3 mozilla::PresShell::ContentInserted(nsIContent*) src/layout/base/PresShell.cpp:4506:22
#4 nsNodeUtils::ContentInserted(nsINode*, nsIContent*) src/dom/base/nsNodeUtils.cpp:216:3
#5 nsINode::doInsertChildAt(nsIContent*, unsigned int, bool, nsAttrAndChildArray&) src/dom/base/nsINode.cpp:1418:7
#6 nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:2281:14
#7 nsINode::ReplaceWith(mozilla::dom::Sequence<mozilla::dom::OwningNodeOrString> const&, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:1618:13
#8 mozilla::dom::Element_Binding::replaceWith(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/ElementBinding.cpp:4545:9
#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:3306:13
#10 CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/vm/Interpreter.cpp:444:15
#11 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:532:16
#12 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:583:12
#13 Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3237:18
#14 js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:424:12
#15 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:556:15
#16 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:583: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:602: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:2887: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, JS::Realm*) 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:1122:52
#23 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1329:20
#24 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:418:17
#25 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:635:16
#26 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1110:9
#27 nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1169:7
#28 nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:7057:21
#29 nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:6850: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:1309:3
#32 nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:852:14
#33 nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:741:9
#34 nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:627: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 imgRequestProxy::RemoveFromLoadGroup() src/image/imgRequestProxy.cpp:445:15
#38 imgRequestProxy::OnLoadComplete(bool) src/image/imgRequestProxy.cpp:1114:7
#39 void mozilla::image::ImageObserverNotifier<mozilla::image::ObserverTable const*>::operator()<void mozilla::image::SyncNotifyInternal<mozilla::image::ObserverTable const*>(mozilla::image::ObserverTable const* const&, bool, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)::{lambda(mozilla::image::IProgressObserver*)#7}>(void mozilla::image::SyncNotifyInternal<mozilla::image::ObserverTable const*>(mozilla::image::ObserverTable const* const&, bool, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)::{lambda(mozilla::image::IProgressObserver*)#7}) src/image/ProgressTracker.cpp:283:9
#40 void mozilla::image::SyncNotifyInternal<mozilla::image::ObserverTable const*>(mozilla::image::ObserverTable const* const&, bool, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) src/image/ProgressTracker.cpp:357:5
#41 mozilla::image::ProgressTracker::SyncNotifyProgress(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)::$_1::operator()(mozilla::image::ObserverTable const*) const src/image/ProgressTracker.cpp:378:5
#42 _ZNK7mozilla5image11CopyOnWriteINS0_13ObserverTableEE4ReadIZNS0_15ProgressTracker18SyncNotifyProgressEjRKNS_3gfx12IntRectTypedINS6_12UnknownUnitsEEEE3$_1EEDTclfp_scPKS2_LDnEEET_ src/image/CopyOnWrite.h:179:12
#43 mozilla::image::ProgressTracker::SyncNotifyProgress(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) src/image/ProgressTracker.cpp:377:14
#44 mozilla::image::RasterImage::NotifyProgress(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::Maybe<unsigned int> const&, mozilla::image::DecoderFlags, mozilla::image::SurfaceFlags) src/image/RasterImage.cpp:1710:28
#45 mozilla::image::RasterImage::NotifyForLoadEvent(unsigned int) src/image/RasterImage.cpp:980:3
#46 mozilla::image::RasterImage::NotifyDecodeComplete(mozilla::image::DecoderFinalStatus const&, mozilla::image::ImageMetadata const&, mozilla::image::DecoderTelemetry const&, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::Maybe<unsigned int> const&, mozilla::image::DecoderFlags, mozilla::image::SurfaceFlags) src/image/RasterImage.cpp:1797:7
#47 mozilla::image::IDecodingTask::NotifyDecodeComplete(mozilla::NotNull<mozilla::image::RasterImage*>, mozilla::NotNull<mozilla::image::Decoder*>)::$_2::operator()() const src/image/IDecodingTask.cpp:130:12
#48 mozilla::detail::RunnableFunction<mozilla::image::IDecodingTask::NotifyDecodeComplete(mozilla::NotNull<mozilla::image::RasterImage*>, mozilla::NotNull<mozilla::image::Decoder*>)::$_2>::Run() src/xpcom/threads/nsThreadUtils.h:552:5
#49 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1051:14
#50 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
#51 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#52 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#53 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#54 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
#55 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:920:22
#56 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:269:9
#57 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#58 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#59 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:746:34
#60 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
#61 main src/browser/app/nsBrowserApp.cpp:287:18
#62 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#63 _start (firefox+0x423724)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Ugh, we definitely end up with a very broken state. We're inserting in a Columnset frame:

  (rr) p aParent
  $10 = (mozilla::dom::HTMLSharedListElement *) 0x7f6e9ebbc900
  (rr) p aParent->GetPrimaryFrame()
  $11 = (nsColumnSetFrame *) 0x7f6e92980440
  (rr) p aParent->GetParent()
  $12 = (mozilla::dom::HTMLSpanElement *) 0x7f6e9eb1b580
  (rr) p aParent->GetParent()->GetPrimaryFrame()
  $13 = (nsIFrame *) 0x0

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So we lose track of a span which is on the overflow property of the parent inline, when we're only supposed to remove empty next-in-flows.

What's going on is that we're reflowing an inline frame which is the <data> element, which has already been split into multiple continuations.

We start pulling frames from the next continuation in:

  https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/layout/generic/nsInlineFrame.cpp#638

But we pull the <br>, and that causes an inline break after, yet we forget about the reflow being incomplete... So we start removing next continuations, which still contains the span and what not.

I think I have a fix, will push it to try.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

The patch is somewhat trivial, so I'll submit for review. I ran a bunch of tests locally and it looks green so far. Waiting on the full try though.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Don't forget about completeness status if we find a break. r=dbaron

If we find a break, we'd report a complete status, which would mean that our next continuations would get incorrectly deleted, even though there would still be frames to be reflowed in them.

See comment 3 for more context, even though there's a bit which was wrong, which is that the <span> frame wasn't in an overflow list. It was just a pre-existing child of a next continuation.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Try is https://treeherder.mozilla.org/#/jobs?repo=try&revision=d302092140519c3501db1e99985e30df41418617, fwiw.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)
> Try is
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d302092140519c3501db1e99985e30df41418617, fwiw.

Looks green modulo stuff from bug 1475511, whoops.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8993740 [details] [diff] [review]
Don't forget about completeness status if we find a break. r=dbaron

So this is relatively tricky code, so it took me a bit of time to dig into it.

So, for a start, this as a straightforward review- because it's adding a uninitialized read of isComplete, in the case of frame being non-null at the start of the loop (which might not be a thing that happens much if at all -- but which we clearly have code to handle).

However, looking a little deeper, based on reading through ReflowInlineFrame, it seems like there are two cases in ReflowInlineFrame where we could enter the if where you're adding new code without aStatus.IsIncomplete() already being true (that is, where your new code to call aStatus.SetIncomplete() might be useful):

(1)
https://searchfox.org/mozilla-central/rev/10e6320abc7fb0479032f0e95e9d61e4be194b9e/layout/generic/nsInlineFrame.cpp#730-732

(2)
https://searchfox.org/mozilla-central/rev/10e6320abc7fb0479032f0e95e9d61e4be194b9e/layout/generic/nsInlineFrame.cpp#748-758
but only if the loop finishes.

Given what you said in comment 2, it sounds like you're hitting the second case.

At first glance it looks like the loop finishing in case (2) should correspond to isComplete (which was an output from PullOneFrame) being true.  However, they don't quite correspond, because PullOneFrame's boolean aIsComplete out parameter seems to correspond exactly to whether its return value is null, whereas it seems like it would be more like what the caller (at least, *this* caller) wants if it represented whether the *next* call to PullOneFrame would return null.

Is that the case you're hitting?  If so, maybe it makes more sense to try to fix the aIsComplete output from PullOneFrame -- although that also requires looking at what the other callers want, which I haven't done yet.

I'm also curious about case (1).  I think your change is *probably* incorrect for that case, though I haven't actually investigated what sets inline line-break-before state, or what the normal protocol for handling it is.  (It's also... not at all an exclusively "inline"-related state; I'm not sure why it has the name it does.)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think an example of something that sets inline line-break-before status would be a text frame with a long unbreakable word that doesn't fit on the line, preceded by something non-text.  But it might be worth verifying that.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the separate boolean aIsComplete made more sense prior to https://github.com/mozilla/gecko-dev/commit/0a512d8393d814f3709e3e779292abd0307ad0da but has corresponded since then; to some degree removing it was probably cleanup that should have happened in that patch.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #7)
> Comment on attachment 8993740 [details] [diff] [review]
> Don't forget about completeness status if we find a break. r=dbaron
> 
> So this is relatively tricky code, so it took me a bit of time to dig into
> it.
> 
> So, for a start, this as a straightforward review- because it's adding a
> uninitialized read of isComplete, in the case of frame being non-null at the
> start of the loop (which might not be a thing that happens much if at all --
> but which we clearly have code to handle).

Pfft, you're totally right, nice catch.

> [...]
>
> Is that the case you're hitting?  If so, maybe it makes more sense to try to
> fix the aIsComplete output from PullOneFrame -- although that also requires
> looking at what the other callers want, which I haven't done yet.

I'm hitting (2), and I think most of the other callers don't care, of the output of this outparam, and that we should probably fix (2) so that it actually does what it promises instead.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Remove useless aIsComplete out-param from PullOneFrame.

*aIsComplete == !!frame.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Fix ReflowInlineFrame to properly detect whether we'll pull from a next-in-flow.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)
> Created attachment 8993853 [details] [diff] [review]
> Remove useless aIsComplete out-param from PullOneFrame.
> 
> *aIsComplete == !!frame.

Err, *aIsComplete == !frame, I mean :).

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8993853 [details] [diff] [review]
Remove useless aIsComplete out-param from PullOneFrame.

r=dbaron

You should fix the commit message to change the "!!" to just one "!" since aIsComplete is true when the returned frame is null.

If you want you could also note that it's cleanup following bug 15999. :-)

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8993856 [details] [diff] [review]
Fix ReflowInlineFrame to properly detect whether we'll pull from a next-in-flow.

>From 326544b5b07bcd273eae14d16b1cb49880e5fabb Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <[e-mail]>
>Date: Sat, 21 Jul 2018 01:09:24 +0200
>Subject: [PATCH 2/2] Fix nsInlineFrame::ReflowInlineFrame to properly detect
> whether we'll pull from a next-in-flow.

Maybe mention that the key difference here is that it's now checking the overflow list of the next-in-flows.  (I missed that while writing comment 7, too!)

(This means the patch is only changing behavior in cases where one frame gets reflowed multiple times without at least some of its later continuations being reflowed, since once the later continuations are all reflowed there won't be anything on the overflow lists.)

>+bool

Maybe add "/* static */" here, except...

>+nsInlineFrame::HasFramesToPull(nsInlineFrame* aNextInFlow)

Maybe this would be nicer if aNextInFlow was instead the |this| parameter, and it weren't static?

r=dbaron

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #15)
> Comment on attachment 8993856 [details] [diff] [review]
> Fix ReflowInlineFrame to properly detect whether we'll pull from a
> next-in-flow.
> 
> >From 326544b5b07bcd273eae14d16b1cb49880e5fabb Mon Sep 17 00:00:00 2001
> >From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <[e-mail]>
> >Date: Sat, 21 Jul 2018 01:09:24 +0200
> >Subject: [PATCH 2/2] Fix nsInlineFrame::ReflowInlineFrame to properly detect
> > whether we'll pull from a next-in-flow.
> 
> Maybe mention that the key difference here is that it's now checking the
> overflow list of the next-in-flows.  (I missed that while writing comment 7,
> too!)

Sure.

> (This means the patch is only changing behavior in cases where one frame
> gets reflowed multiple times without at least some of its later
> continuations being reflowed, since once the later continuations are all
> reflowed there won't be anything on the overflow lists.)
> 
> >+bool
> 
> Maybe add "/* static */" here, except...
> 
> >+nsInlineFrame::HasFramesToPull(nsInlineFrame* aNextInFlow)
> 
> Maybe this would be nicer if aNextInFlow was instead the |this| parameter,
> and it weren't static?

I think that's a bit harder to prove right, since PullOneFrame doesn't operate on `this`, but on irs.mNextInFlow, which isn't always this->GetNextInFlow(), and also it'd make the callers a bit more verbose, since I should add null-checks around. So I'll leave it like this for now.

> r=dbaron

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61470c6169b5
Remove useless aIsComplete out-param. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/028ea79c0322
Fix nsInlineFrame::ReflowInlineFrame to properly detect whether we'll pull from a next-in-flow. r=dbaron

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/61470c6169b5
https://hg.mozilla.org/mozilla-central/rev/028ea79c0322

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)
> > >+nsInlineFrame::HasFramesToPull(nsInlineFrame* aNextInFlow)
> > 
> > Maybe this would be nicer if aNextInFlow was instead the |this| parameter,
> > and it weren't static?
> 
> I think that's a bit harder to prove right, since PullOneFrame doesn't
> operate on `this`, but on irs.mNextInFlow, which isn't always
> this->GetNextInFlow(), and also it'd make the callers a bit more verbose,
> since I should add null-checks around. So I'll leave it like this for now.

Oh... I wasn't intending that you change what the parameter was (i.e., move getting the next-in-flow inside the function) -- it could still be something like irs.mNextInFlow->PullOneFrame().