1421807 - crash at null in [@ mozilla::TextNodeCorrespondenceRecorder::TraverseAndRecord]

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

==19887==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fd7df5a0a84 bp 0x7fff01d66990 sp 0x7fff01d66930 T0)
==19887==The signal is caused by a READ memory access.
==19887==Hint: address points to the zero page.
    #0 0x7fd7df5a0a83 in mozilla::TextNodeCorrespondenceRecorder::TraverseAndRecord(nsIFrame*) /src/layout/svg/SVGTextFrame.cpp:1486:49
    #1 0x7fd7df5a06c6 in mozilla::TextNodeCorrespondenceRecorder::TraverseAndRecord(nsIFrame*) /src/layout/svg/SVGTextFrame.cpp:1459:7
    #2 0x7fd7df5a06c6 in mozilla::TextNodeCorrespondenceRecorder::TraverseAndRecord(nsIFrame*) /src/layout/svg/SVGTextFrame.cpp:1459:7
    #3 0x7fd7df5a0336 in mozilla::TextNodeCorrespondenceRecorder::Record(SVGTextFrame*) /src/layout/svg/SVGTextFrame.cpp:1415:3
    #4 0x7fd7df5a0121 in mozilla::TextNodeCorrespondenceRecorder::RecordCorrespondence(SVGTextFrame*) /src/layout/svg/SVGTextFrame.cpp:1400:14
    #5 0x7fd7df5b2f20 in MaybeReflowAnonymousBlockChild /src/layout/svg/SVGTextFrame.cpp:5475:5
    #6 0x7fd7df5b2f20 in SVGTextFrame::ReflowSVG() /src/layout/svg/SVGTextFrame.cpp:3790
    #7 0x7fd7df5b4539 in nsSVGDisplayContainerFrame::ReflowSVG() /src/layout/svg/nsSVGContainerFrame.cpp:350:17
    #8 0x7fd7df622393 in nsSVGOuterSVGFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /src/layout/svg/nsSVGOuterSVGFrame.cpp:455:14
    #9 0x7fd7df36eaaf in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /src/layout/generic/nsLineLayout.cpp:922:13
    #10 0x7fd7df1b3119 in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /src/layout/generic/nsBlockFrame.cpp:4173:15
    #11 0x7fd7df1b1ad7 in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /src/layout/generic/nsBlockFrame.cpp:3969:5
    #12 0x7fd7df1a8e77 in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /src/layout/generic/nsBlockFrame.cpp:3843:9
    #13 0x7fd7df1a2100 in nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /src/layout/generic/nsBlockFrame.cpp:2827:5
    #14 0x7fd7df197eaa in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /src/layout/generic/nsBlockFrame.cpp:2363:7
    #15 0x7fd7df18fc65 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /src/layout/generic/nsBlockFrame.cpp:1236:3
    #16 0x7fd7df1af0a7 in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, nsReflowStatus&, mozilla::BlockReflowInput&) /src/layout/generic/nsBlockReflowContext.cpp:306:11
    #17 0x7fd7df1a42bb in nsBlockFrame::ReflowBlockFrame(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /src/layout/generic/nsBlockFrame.cpp:3474:11
    #18 0x7fd7df1a2255 in nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /src/layout/generic/nsBlockFrame.cpp:2824:5
    #19 0x7fd7df197eaa in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /src/layout/generic/nsBlockFrame.cpp:2363:7
    #20 0x7fd7df18fc65 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /src/layout/generic/nsBlockFrame.cpp:1236:3
    #21 0x7fd7df1f02a6 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /src/layout/generic/nsContainerFrame.cpp:934:14
    #22 0x7fd7df1eea4b in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /src/layout/generic/nsCanvasFrame.cpp:757:5
    #23 0x7fd7df1f02a6 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /src/layout/generic/nsContainerFrame.cpp:934:14
    #24 0x7fd7df2c4637 in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) /src/layout/generic/nsGfxScrollFrame.cpp:552:3
    #25 0x7fd7df2c5839 in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) /src/layout/generic/nsGfxScrollFrame.cpp:664:3
    #26 0x7fd7df2c99e6 in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /src/layout/generic/nsGfxScrollFrame.cpp:1041:3
    #27 0x7fd7df174dee in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /src/layout/generic/nsContainerFrame.cpp:978:14
    #28 0x7fd7df1738c9 in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /src/layout/generic/ViewportFrame.cpp:336:7
    #29 0x7fd7def417d0 in mozilla::PresShell::DoReflow(nsIFrame*, bool) /src/layout/base/PresShell.cpp:9025:11
    #30 0x7fd7def58a43 in mozilla::PresShell::ProcessReflowCommands(bool) /src/layout/base/PresShell.cpp:9198:24
    #31 0x7fd7def57804 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /src/layout/base/PresShell.cpp:4267:11
    #32 0x7fd7deeb8114 in FlushPendingNotifications /src/obj-firefox/dist/include/nsIPresShell.h:580:5
    #33 0x7fd7deeb8114 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:1901
    #34 0x7fd7deec731f in TickDriver /src/layout/base/nsRefreshDriver.cpp:336:13
    #35 0x7fd7deec731f in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /src/layout/base/nsRefreshDriver.cpp:306
    #36 0x7fd7deec6ed4 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:327:5
    #37 0x7fd7deec975e in RunRefreshDrivers /src/layout/base/nsRefreshDriver.cpp:769:5
    #38 0x7fd7deec975e in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:682
    #39 0x7fd7deec4ce7 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() /src/layout/base/nsRefreshDriver.cpp:528:20
    #40 0x7fd7d752c17e in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1033:14
    #41 0x7fd7d7547f00 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:508:10
    #42 0x7fd7d83b975a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
    #43 0x7fd7d83109f9 in RunInternal /src/ipc/chromium/src/base/message_loop.cc:326:10
    #44 0x7fd7d83109f9 in RunHandler /src/ipc/chromium/src/base/message_loop.cc:319
    #45 0x7fd7d83109f9 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299
    #46 0x7fd7de7419ba in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:157:27
    #47 0x7fd7e2c58dfb in nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
    #48 0x7fd7e2e71668 in XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4649:22
    #49 0x7fd7e2e7449e in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4811:8
    #50 0x7fd7e2e75914 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4903:21
    #51 0x4ee80b in do_main /src/browser/app/nsBrowserApp.cpp:231:22
    #52 0x4ee80b in main /src/browser/app/nsBrowserApp.cpp:304
    #53 0x7fd7f5fa082f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #54 0x41e078 in _start (/home/user/workspace/browsers/m-c-1511776031-asan-opt/firefox+0x41e078)

Comment 1

[Tyson Smith [:tsmith]]

Attachment: prefs.js

Comment 2

[Cameron McCormack (:heycam)]

Tyson, now that we disallow shadow trees on SVG elements now (and the function has been renamed from createShadowRoot to attachShadow), this test should no longer crash.  Can you confirm?

Comment 3

[Tyson Smith [:tsmith]]

Attachment: testcase.html

I can still trigger the crash with this testcase.

Tested with m-c:
BuildID=20180419223008
SourceStamp=5d73549d363f2a52854ff43fabac9c6fd05b90b0

Comment 4

[Cameron McCormack (:heycam)]

Thanks Tyson.

I just found bug 1452039, which pointed me to https://drafts.csswg.org/css-display/#unbox-svg, which describes how various elements should behave wrt display:contents.  The case here is an HTML <dl> element that is a child of an SVG <text>.  In a <text>, the normal behavior of any element other than SVG <textPath>, <tspan>, and <a> would be to skip the element entirely.  I think it makes sense for the <dl> element to be skipped, just as if it were display:inline, although it's a bit hard to tell if this case is covered by the spec.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #4)
> Thanks Tyson.
> 
> I just found bug 1452039, which pointed me to
> https://drafts.csswg.org/css-display/#unbox-svg, which describes how various
> elements should behave wrt display:contents.  The case here is an HTML <dl>
> element that is a child of an SVG <text>.  In a <text>, the normal behavior
> of any element other than SVG <textPath>, <tspan>, and <a> would be to skip
> the element entirely.  I think it makes sense for the <dl> element to be
> skipped, just as if it were display:inline, although it's a bit hard to tell
> if this case is covered by the spec.

Comment 3 doesn't crash on my machine, am I missing something? Maybe fixed by bug 1453702, which touched a lot of that code?

Comment 6

[Cameron McCormack (:heycam)]

Hmm, yes it also doesn't crash on my machine, but I do run into the "expected a TextNodeCorrespondenceProperty on nsTextFrame used for SVG text" (non-fatal) assertion still.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Alright, I'll try to take a look as soon as I have a build :)

Comment 8

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Looks like I removed a newline at the end of the testcase which broke it.

I repro'd with m-c:
BuildID=20180420091115
SourceStamp=cc0d7de218cb0c260c8ba0cf6637845ad2222f49

Comment 9

[Emilio Cobos Álvarez (:emilio)]

TextCorrespondenceRecorder makes too many assumptions about the DOM shape, all to just count a couple characters... :(

Cam, how important is that code? Is there a simpler way to find that out? It really wants a lot of the insertion-point-computation code that nsCSSFrameConstructor has, plus walking over flattened tree siblings.

I'll land the wallpaper for now but it'd be nice for our code to be a bit more Shadow DOM friendly... :P

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1421807: Wallpaper SVG text code which makes too many assumptions about the DOM.

Second test-case is because I initially made this code work with
display: contents. But then realised that display: contents meant allowing
Shadow DOM in there, which I don't really want to deal with right now.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca67a1ab5b2
Add an assertion that should help with catching more instances of this bug. r=me

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Pulsebot from comment #11)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca67a1ab5b2
> Add an assertion that should help with catching more instances of this bug.
> r=me

Err, that landed with the wrong bug number, sigh.

Comment 13

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/05 to 09/06) from comment #9)
> TextCorrespondenceRecorder makes too many assumptions about the DOM shape,
> all to just count a couple characters... :(
> 
> Cam, how important is that code? Is there a simpler way to find that out? It
> really wants a lot of the insertion-point-computation code that
> nsCSSFrameConstructor has, plus walking over flattened tree siblings.

Yes, I agree that the code to handle SVG text positioning attributes is quite complicated, and I wish we didn't have to do it, or could do something simpler.  I think it's fine for now to just punt on display:contents in SVG text, but I suppose we do need to teach TextCorrespondenceRecorder what it means to traverse through frames for children of a display:contents, and I guess it shouldn't be too hard.

Ultimately, SVGTextFrame needs to know which text characters in the DOM tree are addressable by x="", y="", dx="", dy="", rotate="", and that is dependent on the information in the frames (to see which characters are trimmed white space, etc., which are inhibited due to display:none, ...).  Content relies on this unfortunately.

For now, given shadow trees aren't allowed to be attached to SVG elements, I think we don't need to worry about flattened tree sibling walking.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8970307 [details]
Bug 1421807: Wallpaper SVG text code which makes too many assumptions about the DOM.

https://reviewboard.mozilla.org/r/239102/#review244858

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #13)
> (In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/05 to 09/06)
> from comment #9)
> > TextCorrespondenceRecorder makes too many assumptions about the DOM shape,
> > all to just count a couple characters... :(
> > 
> > Cam, how important is that code? Is there a simpler way to find that out? It
> > really wants a lot of the insertion-point-computation code that
> > nsCSSFrameConstructor has, plus walking over flattened tree siblings.
> 
> Yes, I agree that the code to handle SVG text positioning attributes is
> quite complicated, and I wish we didn't have to do it, or could do something
> simpler.  I think it's fine for now to just punt on display:contents in SVG
> text, but I suppose we do need to teach TextCorrespondenceRecorder what it
> means to traverse through frames for children of a display:contents, and I
> guess it shouldn't be too hard.

Sure, I did teach it that, I had a patch ready. But allowing display: contents elements there means allowing, e.g., and <html:div> element, which _can_ be a shadow root, causing badness.

Thus this patch instead.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26b85ad25329
Wallpaper SVG text code which makes too many assumptions about the DOM. r=heycam

Comment 17

[Cameron McCormack (:heycam)]

For that underspecified case (display:contents on non-SVG-text-content-element inside SVG <text>) I think it would be fine to just make that behave like display:none, for now.

But I don't think making display:contents on SVG <tspan> etc. is urgent at the moment so I am fine with landing this wallpapering.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1af032ceb563
Adjust test expectations for test that no longer asserts. r=me

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/4ca67a1ab5b2

Comment 20

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/26b85ad25329
https://hg.mozilla.org/mozilla-central/rev/1af032ceb563