Closed Bug 1419762 Opened 2 years ago Closed 2 years ago

AddressSanitizer: use-after-poison [@ Hdr] with READ of size 8

Categories

(Core :: Layout: Block and Inline, defect, P2, critical)

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 - fixed
firefox59 - fixed

People

(Reporter: jkratzer, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage])

Attachments

(4 files)

Found while fuzzing mozilla-central rev 5378dcb45044.  I am in the process of reducing the testcase and will attach shortly.

==15820==ERROR: AddressSanitizer: use-after-poison on address 0x6250034b1628 at pc 0x7f60ef349513 bp 0x7ffca7946860 sp 0x7ffca7946858
READ of size 8 at 0x6250034b1628 thread T0 (file:// Content)
    #0 0x7f60ef349512 in Hdr /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:527:32
    #1 0x7f60ef349512 in Elements /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:1038
    #2 0x7f60ef349512 in IndexOf<const mozilla::FramePropertyDescriptorUntyped *, mozilla::FrameProperties::PropertyComparator> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:1187
    #3 0x7f60ef349512 in mozilla::FrameProperties::DeleteInternal(mozilla::FramePropertyDescriptorUntyped const*, nsIFrame const*) /builds/worker/workspace/build/src/layout/base/FrameProperties.h:485
    #4 0x7f60ef3ebf9c in Delete<nsContainerFrame> /builds/worker/workspace/build/src/layout/base/FrameProperties.h:266:5
    #5 0x7f60ef3ebf9c in DeleteProperty<nsContainerFrame> /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:3611
    #6 0x7f60ef3ebf9c in nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:233
    #7 0x7f60ef5c000c in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsLineBox.cpp:402:14
    #8 0x7f60ef3eb2e7 in nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:331:3
    #9 0x7f60ef5c000c in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsLineBox.cpp:402:14
    #10 0x7f60ef3eb2e7 in nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:331:3
    #11 0x7f60ef3ebe4d in DestroyFramesFrom /builds/worker/workspace/build/src/layout/generic/nsFrameList.cpp:59:12
    #12 0x7f60ef3ebe4d in nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:223
    #13 0x7f60ef3ebe4d in DestroyFramesFrom /builds/worker/workspace/build/src/layout/generic/nsFrameList.cpp:59:12
    #14 0x7f60ef3ebe4d in nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:223
    #15 0x7f60ef449199 in nsCanvasFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:160:21
    #16 0x7f60ef3ebe4d in DestroyFramesFrom /builds/worker/workspace/build/src/layout/generic/nsFrameList.cpp:59:12
    #17 0x7f60ef3ebe4d in nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:223
    #18 0x7f60ef4601d5 in Destroy /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:695:5
    #19 0x7f60ef4601d5 in nsContainerFrame::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:171
    #20 0x7f60ef297c78 in RemoveFrame /builds/worker/workspace/build/src/layout/base/nsFrameManager.cpp:535:18
    #21 0x7f60ef297c78 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:8696
    #22 0x7f60ef283644 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:9896:7
    #23 0x7f60ef1ad75a in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:1523:25
    #24 0x7f60ef22570a in mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1159:9
    #25 0x7f60ef1e3846 in ProcessPendingRestyles /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1235:3
    #26 0x7f60ef1e3846 in ProcessPendingRestyles /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RestyleManagerInlines.h:44
    #27 0x7f60ef1e3846 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4226
    #28 0x7f60eafeb710 in FlushPendingNotifications /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:571:5
    #29 0x7f60eafeb710 in nsDocument::FlushPendingNotifications(mozilla::FlushType, mozilla::FlushTarget) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:8566
    #30 0x7f60eadb2bf7 in GetPrimaryFrame /builds/worker/workspace/build/src/dom/base/Element.cpp:2451:10
    #31 0x7f60eadb2bf7 in mozilla::dom::Element::GetBoundingClientRect() /builds/worker/workspace/build/src/dom/base/Element.cpp:1081
    #32 0x7f60ec6447dc in mozilla::dom::ElementBinding::getBoundingClientRect(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/ElementBinding.cpp:2660:59
    #33 0x7f60ecb69660 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3040:13
    #34 0x7f60f30226c0 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #35 0x7f60f30226c0 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
    #36 0x7f60f300dbdb in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
    #37 0x7f60f300dbdb in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3098
    #38 0x7f60f2ff5c9a in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #39 0x7f60f30255e0 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:706:15
    #40 0x7f60f30777d5 in EvalKernel(JSContext*, JS::Handle<JS::Value>, EvalType, js::AbstractFramePtr, JS::Handle<JSObject*>, unsigned char*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/builtin/Eval.cpp:323:12
    #41 0x7f60f3075f33 in js::IndirectEval(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/builtin/Eval.cpp:416:12
    #42 0x1e5d1f4cc0c5  (<unknown module>)

0x6250034b1628 is located 7464 bytes inside of 8192-byte region [0x6250034af900,0x6250034b1900)
allocated by thread T0 (file:// Content) here:
    #0 0x4bbc8c in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3
    #1 0x7f60e814ded3 in AllocateChunk /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:193:15
    #2 0x7f60e814ded3 in InternalAllocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:228
    #3 0x7f60e814ded3 in Allocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:75
    #4 0x7f60e814ded3 in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:80
    #5 0x7f60ef6ca6af in AllocateByFrameID /builds/worker/workspace/build/src/layout/base/nsPresArena.h:39:12
    #6 0x7f60ef6ca6af in AllocateFrame /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:205
    #7 0x7f60ef6ca6af in operator new /builds/worker/workspace/build/src/layout/forms/nsHTMLButtonControlFrame.cpp:28
    #8 0x7f60ef6ca6af in NS_NewHTMLButtonControlFrame(nsIPresShell*, nsStyleContext*) /builds/worker/workspace/build/src/layout/forms/nsHTMLButtonControlFrame.cpp:25
    #9 0x7f60ef2767ae in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4011:7
    #10 0x7f60ef282a2b in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6342:3
    #11 0x7f60ef261806 in ConstructFramesFromItemList /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10879:5
    #12 0x7f60ef261806 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11193
    #13 0x7f60ef278152 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4189:9
    #14 0x7f60ef282a2b in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6342:3
    #15 0x7f60ef261806 in ConstructFramesFromItemList /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10879:5
    #16 0x7f60ef261806 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11193
    #17 0x7f60ef26b54e in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsContainerFrame*, nsStyleContext*, nsContainerFrame**, nsFrameItems&, nsIFrame*, PendingBinding*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:12133:3
    #18 0x7f60ef273a54 in nsCSSFrameConstructor::ConstructNonScrollableBlockWithConstructor(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&, nsBlockFrame* (*)(nsIPresShell*, nsStyleContext*)) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5094:3
    #19 0x7f60ef27b5c7 in nsCSSFrameConstructor::ConstructNonScrollableBlock(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5058:10
    #20 0x7f60ef2768b8 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4005:7
    #21 0x7f60ef282a2b in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6342:3
    #22 0x7f60ef261806 in ConstructFramesFromItemList /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10879:5
    #23 0x7f60ef261806 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11193
    #24 0x7f60ef26b54e in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsContainerFrame*, nsStyleContext*, nsContainerFrame**, nsFrameItems&, nsIFrame*, PendingBinding*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:12133:3
    #25 0x7f60ef267541 in nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:2757:5
    #26 0x7f60ef28a9f3 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, nsCSSFrameConstructor::InsertionKind, TreeMatchContext*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:8026:9
    #27 0x7f60ef2892c2 in nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, nsCSSFrameConstructor::InsertionKind) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:7899:3
    #28 0x7f60ef1c83f0 in mozilla::PresShell::Initialize(int, int) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:1782:26
    #29 0x7f60eaf36020 in nsContentSink::StartLayout(bool) /builds/worker/workspace/build/src/dom/base/nsContentSink.cpp:1288:26
    #30 0x7f60eafeb4a0 in nsDocument::FlushPendingNotifications(mozilla::FlushType, mozilla::FlushTarget) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:8537:13
    #31 0x7f60eacd5ded in FlushPendingNotifications /builds/worker/workspace/build/src/dom/base/nsGlobalWindowInner.cpp:6450:11
    #32 0x7f60eacd5ded in nsGlobalWindowInner::ScrollByPages(int, mozilla::dom::ScrollOptions const&) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowInner.cpp:3549
    #33 0x7f60ec2cf029 in mozilla::dom::WindowBinding::scrollByPages(JSContext*, JS::Handle<JSObject*>, nsGlobalWindowInner*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/WindowBinding.cpp:4317:9
    #34 0x7f60ec2c4105 in mozilla::dom::WindowBinding::genericMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/WindowBinding.cpp:15333:13
    #35 0x7f60f30226c0 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #36 0x7f60f30226c0 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
    #37 0x7f60f300dbdb in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
    #38 0x7f60f300dbdb in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3098
    #39 0x7f60f2ff5c9a in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #40 0x7f60f30255e0 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:706:15
    #41 0x7f60f30777d5 in EvalKernel(JSContext*, JS::Handle<JS::Value>, EvalType, js::AbstractFramePtr, JS::Handle<JSObject*>, unsigned char*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/builtin/Eval.cpp:323:12

SUMMARY: AddressSanitizer: use-after-poison /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:527:32 in Hdr
Shadow bytes around the buggy address:
  0x0c4a8068e270: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a8068e280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a8068e290: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a8068e2a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a8068e2b0: 00 00 00 00 00 00 00 00 00 00 f7 f7 f7 f7 f7 f7
=>0x0c4a8068e2c0: f7 f7 f7 f7 f7[f7]f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c4a8068e2d0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c4a8068e2e0: f7 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a8068e2f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a8068e300: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c4a8068e310: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==15820==ABORTING
Flags: in-testsuite?
Group: core-security → layout-core-security
> I am in the process of reducing the testcase and will attach shortly.

When you've attached the testcase please change the 'testcase-wanted' keyword to 'testcase'
Attached file trigger.html
Bisected to:

Start: 40df5dd35fdb7ce3652fe4448ac8961c075c928e (20171107215729)
End: 3e95c596ad5bc0c0b99608ed26380994973e7665 (20171108104655)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=40df5dd35fdb7ce3652fe4448ac8961c075c928e&tochange=3e95c596ad5bc0c0b99608ed26380994973e7665
I can't reproduce it in my local m-c ASAN build on Linux.
Are there any preferences needed?
Flags: needinfo?(jkratzer)
Looking at the stack this is likely mitigated by framepoisoning, which would make it not a sec-high bug. In that case I'd expect to see a non-ASAN crash though.
Attached file prefs.js
Flags: needinfo?(jkratzer)
Thanks, I can reproduce it with that prefs.js file.

We crash in nsContainerFrame::DestroyFrom:
(rr) list
225       // If we have any IB split siblings, clear their references to us.
226       if (HasAnyStateBits(NS_FRAME_PART_OF_IBSPLIT)) {
227         // Delete previous sibling's reference to me.
228         nsIFrame* prevSib = GetProperty(nsIFrame::IBSplitPrevSibling());
229         if (prevSib) {
230           NS_WARNING_ASSERTION(
231             this == prevSib->GetProperty(nsIFrame::IBSplitSibling()),
232             "IB sibling chain is inconsistent");
233           prevSib->DeleteProperty(nsIFrame::IBSplitSibling());
234         }

accessing |prevSib| which is a deleted frame (so this should be mitigated by frame-poisoning AFAICT).

There are two assertions leading up to the crash:
ASSERTION: aParentFrame is not last?: 'nextSibling || !IsFramePartOfIBSplit(aParentFrame) || (IsInlineFrame(aParentFrame) && !GetIBSplitSibling(aParentFrame) && !aParentFrame->GetNextContinuation()) || aIsRecursiveCall', layout/base/nsCSSFrameConstructor.cpp, line 6649
ASSERTION: line participants must not be containers: '!aFrame->IsFrameOfType(nsIFrame::eLineParticipant) || isInline || aFrame->IsBrFrame() || aFrame->IsFrameOfType(nsIFrame::eMathML)', layout/generic/nsFrame.cpp, line 622
Component: Layout: HTML Frames → Layout: Block and Inline
Attached file frame tree
This is the frame tree we're trying to destroy.
It appears we have two frames with IBSplitPrevSibling=625000353558 (red),
which seems wrong.
(In reply to Mats Palmgren (:mats) from comment #8)
> Created attachment 8931137 [details]
> frame tree
> 
> This is the frame tree we're trying to destroy.
> It appears we have two frames with IBSplitPrevSibling=625000353558 (red),
> which seems wrong.

Yeah, that looks fishy... From what I can tell, the underlying issue is that we mess up the insertion point stuff as follows:

We arrive at https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/layout/base/nsCSSFrameConstructor.cpp#7048, there's no previous nor next sibling available, so we don't walk past the trailing inline, which is empty.

We manage to get a previous sibling a couple lines below via FindAppendPrevSibling (which is a function that doesn't make much sense to me, but anyway).

When we arrive to AppendFramesToParent, we find out that we're not the last frame in the IB split chain (that'd be the trailing inline that we just avoided to walk through)...

I still haven't figured out why we don't need to peek the trailing inline when choosing a parent frame that is part of an IB split sibling...
Doing this fixes the crash as expected... Now let's figure out why that's wrong and what's supposed to happen...

         aInsertion->mParentFrame =
-          GetLastIBSplitSibling(aInsertion->mParentFrame, false);
+          GetLastIBSplitSibling(aInsertion->mParentFrame, true);
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Doing this fixes the crash as expected... Now let's figure out why that's
> wrong and what's supposed to happen...
> 
>          aInsertion->mParentFrame =
> -          GetLastIBSplitSibling(aInsertion->mParentFrame, false);
> +          GetLastIBSplitSibling(aInsertion->mParentFrame, true);

I think that change is actually correct. The only reason we wouldn't want to append to the trailing inline is when there's ::after content. And the code above ensures there isn't. Assuming this is green on try, I'll submit a patch.

See the comment in[1], for example.

[1]: https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/layout/base/nsCSSFrameConstructor.cpp#6565
ni? myself so I don't forget about cycling back to this.

Bug 1413619 is what ensures that there's no ::after content.
Flags: needinfo?(emilio)
The only reason not to do that is when there's after content in there. We know
that there isn't really any ::after content, since it would've been handled by
FindNextSibling, so we know we're performing a real append.
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Flags: needinfo?(emilio)
Attachment #8931174 - Flags: review?(mats)
Priority: -- → P2
Blocks: 1418503
Same regression range as bug 1418503.
Blocks: 1415152
Has Regression Range: --- → yes
First, the code in ContentAppended and here should match, no?

Second, it's not clear to me how this gives the right behavior.  If I have:

  <span><div></div></span>

and I append another <div> to the <span>, it should got into the block part of the {ib} split.  Ideally without reframing anything in the process.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> First, the code in ContentAppended and here should match, no?
> 
> Second, it's not clear to me how this gives the right behavior.  If I have:
> 
>   <span><div></div></span>
> 
> and I append another <div> to the <span>, it should got into the block part
> of the {ib} split.  Ideally without reframing anything in the process.

That doesn't match at all what AdjustParentFrameForAfterContent does when it finds no ::after content, which this function used to call:

  https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/layout/base/nsCSSFrameConstructor.cpp#6559

  // We might be in a situation where the last part of the {ib} split was
  // empty.  Since we have no ::after pseudo-element, we do in fact want to be
  // appending to that last part, so advance to it if needed.  Note that here
  // aParentFrame is the result of a GetLastIBSplitSibling call, so must be
  // either the last or next to last ib-split sibling.

Since bug 1413619, there's no ::after, because that's handled by FindNextSibling, but bug 1415152 removed that call which avoided that ib split adjustment.

Am I missing something Boris?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> First, the code in ContentAppended and here should match, no?
> 
> Second, it's not clear to me how this gives the right behavior.  If I have:
> 
>   <span><div></div></span>
> 
> and I append another <div> to the <span>, it should got into the block part
> of the {ib} split.  Ideally without reframing anything in the process.

Also note that the case you're describing doesn't hit this code path. This code path is only when you're inserting and there's neither previous or next sibling. In this case we'd find the <div> as the previous sibling, and use the div frame->GetParent()->GetContentInsertionFrame() as the insertion point, which would be the block part of the IB split iiuc.
Track 58-/59- for now. Feel free to nominate if it's sec-high/critical.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #17)
> This code path is only when you're inserting and there's neither previous or next
> sibling.

I'm nit-picking, but that isn't entirely accurate.  In this case the previous sibling
(<input>) does in fact have a frame, it's just that it's rejected by IsValidSibling
because it has display:table-column-group.

For the record, this is the content tree for the testcase after the node we're
trying to create a frame for (<optgroup>) has been inserted:

isindex@0x7f95e3346940 state=[100020000000] flags=[00104000] primaryframe=0x7f95d692acc0 refcount=7<
  input@0x7f95ff75bb60 state=[100040400408] flags=[00100104] primaryframe=0x7f95d692ad50 refcount=5<>
  optgroup@0x7f95e3346c10 state=[100020000008] flags=[00100008] primaryframe=(nil) refcount=3<>
>

The relevant part of the frame tree (edited for brevity):

line 7f95d692b688: state=inline <
  Inline(isindex)(1)@7f95d692acc0 next=7f95d692b458 IBSplitSibling=7f95d692b458 [state=0089100000008200] [content=7f95e3346940]
>
line 7f95d692b6d8: state=block <
  Block(isindex)(1)@7f95d692b458 next=7f95d692b5a8 IBSplitSibling=7f95d692b5a8 IBSplitPrevSibling=7f95d692acc0 [state=000b020000109000] [content=7f95e3346940] <
    line 7f95d692b508: state=inline <
      nsTextControlFrame@7f95d692ad50 [state=000b020000004220] [content=7f95ff75bb60] <
        ...
      >
    >
  >
>
line 7f95d692b728: count=1 state=inline <
  Inline(isindex)(1)@7f95d692b5a8 IBSplitPrevSibling=7f95d692b458 [state=0009100000008200] [content=7f95e3346940]<>
>

It's interesting to note that the line inside the anon block says "state=inline",
so it treats nsTextControlFrame as an inline even though it's what triggered
the ib-split in the first place...
Comment on attachment 8931174 [details] [diff] [review]
Return the inline continuation of an IB split when appending.

I agree with Boris that it's odd that ContentAppended pass |false|.
Fwiw, I investigated that a bit and found that three tests fail
if we pass 'true' there:
layout/reftests/ib-split/insert-into-split-inline-12.html
layout/reftests/ib-split/insert-into-split-inline-15.html
layout/reftests/ib-split/insert-into-split-inline-16b.html 

However, together with the fix in bug 1419964 we do pass all tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09351acbb8bbca1973f334f20e85e184ed9ef164

File a follow-up bug on investigating/fixing that?
Or if you want to land that bug first and do it here...
Attachment #8931174 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #20)
> Comment on attachment 8931174 [details] [diff] [review]
> Return the inline continuation of an IB split when appending.
> 
> I agree with Boris that it's odd that ContentAppended pass |false|.
> Fwiw, I investigated that a bit and found that three tests fail
> if we pass 'true' there:
> layout/reftests/ib-split/insert-into-split-inline-12.html
> layout/reftests/ib-split/insert-into-split-inline-15.html
> layout/reftests/ib-split/insert-into-split-inline-16b.html 
> 
> However, together with the fix in bug 1419964 we do pass all tests:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=09351acbb8bbca1973f334f20e85e184ed9ef164
> 
> File a follow-up bug on investigating/fixing that?
> Or if you want to land that bug first and do it here...

That makes sense to me, I think...

All those tests have ::after content, which means that even though we go through ContentAppended (because it's inserting the latest node), it's not a real append, in the sense that the ::after frame is there.

The tricky part is that AdjustFrameForAfterContent _also_ walked to the ib-split trailing inline when there was no ::after, which was an oversight on my side when writing bug 1415152, and it's why it regresses these.

Bug 1419964 also fixes it, and we can start walking to the last inline directly, instead of in:

    if (nsContainerFrame* trailingInline = GetIBSplitSibling(parentFrame)) {
      parentFrame = trailingInline;
    }

I had already a patch cleaning that stuff up, but I was waiting for this two to land... Will send that today.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #21)
> Bug 1419964 also fixes it, and we can start walking to the last inline
> directly, instead of in:

"Fixes it" is not the right word. I meant that also does it, _but_ we can start...
Great.  I wonder if we should also try to share more code between
ContentAppended and the "isAppend" case in ContentRangeInserted...
For the record, the testcase triggers this assertion in
nsCSSFrameConstructor::AppendFramesToParent before crashing:
 ###!!! ASSERTION: aParentFrame is not last?: 'nextSibling || !IsFramePartOfIBSplit(aParentFrame) || (IsInlineFrame(aParentFrame) && !GetIBSplitSibling(aParentFrame) && !aParentFrame->GetNextContinuation()) || aIsRecursiveCall', layout/base/nsCSSFrameConstructor.cpp, line 6649
I wanted to wait for Boris' ni?, but he told me that it was fine to push it and he'll go through it anyway, so I pushed both this and bug 1419964 since I want to do the cleanup afterwards.

https://hg.mozilla.org/integration/mozilla-inbound/rev/333214e07fd463dc1c7db00875bb6dff98eb0cdf
Blocks: 1424094
https://hg.mozilla.org/mozilla-central/rev/333214e07fd4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Duplicate of this bug: 1418503
Comment on attachment 8931174 [details] [diff] [review]
Return the inline continuation of an IB split when appending.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1415152
[User impact if declined]: security bug, potentially mitigated by frame poisoning.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: none, though it'd be nice to also uplift bug 1424094 and bug 1419964, though those could benefit of a bit of baking time.
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: One liner, brings back most of the behavior pre bug 1415152.
[String changes made/needed]: none
Attachment #8931174 - Flags: approval-mozilla-beta?
OK, I finally got a chance to sit down and sort through this code.  Hurray for plane flights.

Brief summary for posterity, or in case I ever have to look at this stuff again and remember to check this bug:

1) Most importantly, the patch in this bug is correct.

2) Getting into a situation where were have no prevsibling or nextsibling _and_ our parent is ib-split takes  some work: if it _actually_ has no kids, it wouldn't get split!  What that means is that it needs kids which will test false for IsValidSibling for us.  And the parent needs to be an inline frame to start with.  The only ways to get IsValidSibling to return false are to have a parent fieldset or xul menu (both of which are not inline frames) or to have a sibling that has a table-internal display type.  And in the latter case the only way to have an inline frame parent is to have the sibling not have a table-internal _frame_ type.  Hence the <input style="display: table-column-group"> in this testcase and the <iframe style="display: table-column-group"> in the testcase for bug 1418503.  It's enough, really, to be inserting into the DOM before such a sibling, to trigger this situation

3) Once we have gotten into that situation, we will treat the insert as an append.  This will produce incorrect layout, because we really should insert; fundamentally IsValidSibling is lying here; I filed bug 1424656 on that.

4) The reason the patch for this bug is correct is that once we're doing an append (in the sense that we set isAppend to true in nsCSSFrameConstructor::ContentRangeInserted, by setting *aIsAppend true in GetInsertionPrevSibling, then we will take the AppendFramesToParent path to actually append the newly-constructed frames to the parent.  And that path very much assumes that the new frames are parented to the trailing inline of the ib split.  If that trailing inline is empty and the new frames start with a sequence of non-inlines, it will go ahead and pull those non-inlines back into the last block of the ib-split.  That addresses my second concern from comment 15.

5) What addresses my first concern from comment 15 is that ContentAppended is still calling AdjustAppendParentForAfterContent, which this code used to call as well.  That ends up resetting the parent to the trailing inline of the ib split if there is no ::after content...  I'm not super-happy about the complexity involved in having the multiple different codepaths here.  :(  Looks like you made them match in bug 1419964, though, right?
Flags: needinfo?(bzbarsky)
Comment on attachment 8931174 [details] [diff] [review]
Return the inline continuation of an IB split when appending.

fix regression in beta58
Attachment #8931174 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: layout-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.