Closed Bug 1437155 Opened 2 years ago Closed 2 years ago

stack-overflow in [@ nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval]

Categories

(Core :: Layout, defect, P2)

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Attachments

(2 files)

Attached file 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
    ...
Flags: in-testsuite?
Flags: needinfo?(emilio)
So the primary issue is that the first-letter content points to a display: contents element...
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
[ Triage 2017/02/20: P2 ] P2 bugs may become P1's after further analysis. Please prioritize diagnosis and repair.
Priority: -- → P2
Assignee: nobody → emilio
Flags: needinfo?(emilio)
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.)
Attachment #8956020 - Flags: review?(mats) → review+
(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!
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a9b65eec6830
Avoid giving a first-letter frame to a display: contents element. r=mats
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")
BTW, did you check if we have a similar problem for ::first-line frames?
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...
(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.
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."
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/abb61fa06489
Expect an assertion on the old style system. r=me
https://hg.mozilla.org/mozilla-central/rev/a9b65eec6830
https://hg.mozilla.org/mozilla-central/rev/abb61fa06489
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Too late to fix in 59.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.