1437155 - stack-overflow in [@ nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval]

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

BuildID=20180209100327
SourceStamp=d49553765a743ebbd4f08e92a93c9d811ee064c2

==101299==ERROR: AddressSanitizer: stack-overflow on address 0x7fff10d5ad18 (pc 0x0000004c132e bp 0x7fff10d5b570 sp 0x7fff10d5ad20 T0)
    #0 0x4c132d in __asan_memset /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:27:3
    #1 0x7f5379a7f572 in mozilla::dom::ExplicitChildIterator::ExplicitChildIterator(nsIContent const*, bool) /src/dom/base/ChildIterator.cpp:24:5
    #2 0x7f537e4459d7 in FlattenedChildIterator /src/dom/base/ChildIterator.h:136:7
    #3 0x7f537e4459d7 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8584
    #4 0x7f537e432a59 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /src/layout/base/nsCSSFrameConstructor.cpp:9870:7
    #5 0x7f537e4482d8 in nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval(nsIFrame*) /src/obj-firefox/dist/include/nsCOMPtr.h
    #6 0x7f537e445865 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8635:9
    #7 0x7f537e445afb in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8589:11
    #8 0x7f537e432a59 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /src/layout/base/nsCSSFrameConstructor.cpp:9870:7
    #9 0x7f537e4482d8 in nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval(nsIFrame*) /src/obj-firefox/dist/include/nsCOMPtr.h
    #10 0x7f537e445865 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8635:9
    #11 0x7f537e445afb in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8589:11
    #12 0x7f537e432a59 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /src/layout/base/nsCSSFrameConstructor.cpp:9870:7
    #13 0x7f537e4482d8 in nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval(nsIFrame*) /src/obj-firefox/dist/include/nsCOMPtr.h
    #14 0x7f537e445865 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8635:9
    #15 0x7f537e445afb in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8589:11
    #16 0x7f537e432a59 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /src/layout/base/nsCSSFrameConstructor.cpp:9870:7
    #17 0x7f537e4482d8 in nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval(nsIFrame*) /src/obj-firefox/dist/include/nsCOMPtr.h
    #18 0x7f537e445865 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8635:9
    #19 0x7f537e445afb in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8589:11
    #20 0x7f537e432a59 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /src/layout/base/nsCSSFrameConstructor.cpp:9870:7
    #21 0x7f537e4482d8 in nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval(nsIFrame*) /src/obj-firefox/dist/include/nsCOMPtr.h
    #22 0x7f537e445865 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8635:9
    #23 0x7f537e445afb in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8589:11
    #24 0x7f537e432a59 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /src/layout/base/nsCSSFrameConstructor.cpp:9870:7
    #25 0x7f537e4482d8 in nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval(nsIFrame*) /src/obj-firefox/dist/include/nsCOMPtr.h
    #26 0x7f537e445865 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8635:9
    #27 0x7f537e445afb in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8589:11
    #28 0x7f537e432a59 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /src/layout/base/nsCSSFrameConstructor.cpp:9870:7
    #29 0x7f537e4482d8 in nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval(nsIFrame*) /src/obj-firefox/dist/include/nsCOMPtr.h
    #30 0x7f537e445865 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /src/layout/base/nsCSSFrameConstructor.cpp:8635:9
    ...

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So the primary issue is that the first-letter content points to a display: contents element...

Comment 2

[Bob Clary [:bc] (inactive)]

Bughunter found this via the attached test case for both Nightly/60, Beta/59 on Windows and Linux. Also crashes opt builds with various stacks containing nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval

Comment 3

[Jet Villegas (inactive)]

[ Triage 2017/02/20: P2 ] P2 bugs may become P1's after further analysis. Please prioritize diagnosis and repair.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1437155: Avoid giving a first-letter frame to a display: contents element.

It doesn't make sense, since they have no frame themselves, and it breaks
invariants other code relies on. Use the parent frame instead.

The stack overflow happens because we give the first-letter frame to the
display: contents element, then we reframe it.

Removing a display: contents node calls ContentRemoved on all the children. One
of these children is this text-node inside the first-letter frame. Since it was
split by bidi resolution we go ahead and reframe the parent in:

  https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/layout/base/nsCSSFrameConstructor.cpp#9688

But the parent is the display: contents node, which results in infinite
recursion.

The usage of GetParent() is wrong anyway too, since it doesn't handle XBL or
Shadow DOM in any way.

Review commit: https://reviewboard.mozilla.org/r/224966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224966/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8956020 [details]
Bug 1437155: Avoid giving a first-letter frame to a display: contents element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224966/diff/1-2/

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8956020 [details]
Bug 1437155: Avoid giving a first-letter frame to a display: contents element.

https://reviewboard.mozilla.org/r/224966/#review230940

Seems reasonable to me.
I think we need to update the "So use its parent for the first-letter." comments also.

I think the "its" there refers to the text node, so this change makes that untrue.
Perhaps something like "So use the parent frame's content as the first-letter
frame's content too.".  Then again, that's a bit wordy for what is fairly obvious
code, so perhaps just remove this part of the comment? (Please keep the first part
about not using the text node itself though.)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8956020 [details]
Bug 1437155: Avoid giving a first-letter frame to a display: contents element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224966/diff/2-3/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #6)
> Comment on attachment 8956020 [details]
> Bug 1437155: Avoid giving a first-letter frame to a display: contents
> element.
> 
> https://reviewboard.mozilla.org/r/224966/#review230940
> 
> Seems reasonable to me.
> I think we need to update the "So use its parent for the first-letter."
> comments also.
> 
> I think the "its" there refers to the text node, so this change makes that
> untrue.
> Perhaps something like "So use the parent frame's content as the first-letter
> frame's content too.".  Then again, that's a bit wordy for what is fairly
> obvious
> code, so perhaps just remove this part of the comment? (Please keep the
> first part
> about not using the text node itself though.)

Sure, did that, thanks for the review!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9b65eec6830
Avoid giving a first-letter frame to a display: contents element. r=mats

Comment 10

[Mats Palmgren (inactive)]

We might want to revisit this once we implement bug 214004 though.
If I read the spec[1] correctly, this should work:

<outer><div>a</div></outer>
<style>
outer::first-letter{ text-decoration:underline; }
outer { display:contents; }


[1]
https://drafts.csswg.org/css-pseudo-4/#first-letter-pseudo
"The ::first-letter pseudo-element represents the first typographic
letter unit [CSS3TEXT] on the first formatted line of its originating
element, [...]"

We might want to make <outer> the content of the first-letter frame
in that case, because the <div> might also have a ::first-letter
rule associated with and it be weird to have two first-letter frames
associated with the same content.  We might want to make the frame
tree look something like this:
block(div)
  first-letter(outer)
    first-letter(div)
      text("a")

Comment 11

[Mats Palmgren (inactive)]

BTW, did you check if we have a similar problem for ::first-line frames?

Comment 12

[Mats Palmgren (inactive)]

The again, Chrome doesn't support the above testcase, even though it
supports both ::first-letter in descendant blocks and display:contents
as far as I know...

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #10)
> We might want to revisit this once we implement bug 214004 though.
> If I read the spec[1] correctly, this should work:
> 
> <outer><div>a</div></outer>
> <style>
> outer::first-letter{ text-decoration:underline; }
> outer { display:contents; }
> 
> 
> [1]
> https://drafts.csswg.org/css-pseudo-4/#first-letter-pseudo
> "The ::first-letter pseudo-element represents the first typographic
> letter unit [CSS3TEXT] on the first formatted line of its originating
> element, [...]"
> 
> We might want to make <outer> the content of the first-letter frame
> in that case, because the <div> might also have a ::first-letter
> rule associated with and it be weird to have two first-letter frames
> associated with the same content.  We might want to make the frame
> tree look something like this:
> block(div)
>   first-letter(outer)
>     first-letter(div)
>       text("a")

Yeah, so... ::first-line / ::first-letter and its interactions with display: contents are kind of a mess... Per spec they do should inherit from display: contents, yet nobody does that. But even without display: contents into account they're terribly incompatible and nobody does what the spec says.

I think representing these pseudos in the frame tree is is the wrong choice btw, our first-letter / first-line code makes the frame constructor very complex, and doing them on the frame tree means we need this really complex style context reparenting code, and also that pulling line frames is a perf footgun, etc etc...

I think we should just try to query the pseudos during line layout, cached somewhere to avoid re-resolving them all the time, fwiw. But that maybe just moves the complexity somewhere else.

(In reply to Mats Palmgren (:mats) from comment #11)
> BTW, did you check if we have a similar problem for ::first-line frames?

Yeah, we initialize the first-line frame with the block frame's contents, which makes sense since we only support first-line on the block itself, so I don't think it has the same issue.

Comment 14

[Mats Palmgren (inactive)]

Ah, I guess it's restricted to elements that *themselves* have block containers
in the next section:
https://drafts.csswg.org/css-pseudo-4/#application-in-css
"In CSS, the ::first-letter pseudo-element only applies to block containers."
Although it leaves open to perhaps extend that in the future:
"A future version of this specification may allow this pseudo-element to apply to more display types."

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abb61fa06489
Expect an assertion on the old style system. r=me

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a9b65eec6830
https://hg.mozilla.org/mozilla-central/rev/abb61fa06489

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix in 59.