655025 - Crash [@ nsSVGGlyphFrame::EndsWithWhitespace ]

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Tony Chung [:tchung]]

I can reproducibly get this crash on this URL.  Tested on Nexus S and Motorola Xoom.  

Note: according to aaron, this reproduces even if javascript.options.methodjit.chrome is disabled.   if its a layout crash, feel free to reroute bug.

Crash stack: https://crash-stats.mozilla.com/report/index/ead409b0-e9ca-4f70-b014-2d9be2110505

Repro:
1) install Android Nightly trunk: Mozilla/5.0 (Android; Linux armv71; rv:6.0a1) Gecko/20110505 Firefox/6.0a1 Fennec/6.0a1
2) install Test harness extension from http://zippityserver.appspot.com/
3) go to URL, click "Startup" > leave the controls as default, and click "Update"
4) Crash @ nsSVGGlyphFrame::EndsWithWhitespace.

Expected:
- no crash

Actual:
- Stack trace.
Frame 	Module 	Signature [Expand] 	Source
0 	libxul.so 	nsSVGGlyphFrame::EndsWithWhitespace 	nsTextFragment.h:206
1 	libxul.so 	nsSVGTextFrame::SetWhitespaceHandling 	layout/svg/base/src/nsSVGTextFrame.cpp:314
2 	libxul.so 	nsSVGTextFrame::UpdateGlyphPositioning 	layout/svg/base/src/nsSVGTextFrame.cpp:341
3 	libxul.so 	nsSVGTextFrame::InitialUpdate 	layout/svg/base/src/nsSVGTextFrame.cpp:257
4 	libxul.so 	nsSVGDisplayContainerFrame::InsertFrames 	layout/generic/nsIFrame.h:1054
5 	libxul.so 	nsFrameManager::InsertFrames 	layout/base/nsFrameManager.cpp:488
6 	libxul.so 	nsCSSFrameConstructor::AppendFrames 	layout/base/nsCSSFrameConstructor.cpp:5764
7 	libxul.so 	nsCSSFrameConstructor::ContentAppended 	layout/base/nsCSSFrameConstructor.cpp:6706
8 	libxul.so 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsCSSFrameConstructor.cpp:6316
9 	libxul.so 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
10 	libxul.so 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
11 	libxul.so 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
12 	libxul.so 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
13 	libxul.so 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
14 	libxul.so 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
15 	libxul.so 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
16 	libxul.so 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsCSSFrameConstructor.cpp:6337
17 	libxul.so 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:4793
18 	libxul.so 	nsDocument::FlushPendingNotifications 	nsCOMPtr.h:492
19 	libxul.so 	nsGenericElement::GetPrimaryFrame 	nsIContent.h:893
20 	libxul.so 	nsSVGGraphicElement::GetBBox 	content/svg/content/src/nsSVGGraphicElement.cpp:95
21 	libxul.so 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:199
22 	libxul.so 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:3141
23 	libxul.so 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1610
24 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:4666
25 	libxul.so 	js::mjit::stubs::UncachedCallHelper 	js/src/methodjit/InvokeHelpers.cpp:381
26 	libxul.so 	CallCompiler::update 	js/src/methodjit/MonoIC.cpp:962
27 	libxul.so 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:1013
28 	libxul.so 	libxul.so@0xb9f802 	
29 	libxul.so 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:599
30 	libxul.so 	js::mjit::JaegerShot 	js/src/jscntxt.h:2134
31 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:4648
32 	libxul.so 	js::mjit::stubs::UncachedCallHelper 	js/src/methodjit/InvokeHelpers.cpp:381
33 	libxul.so 	CallCompiler::update 	js/src/methodjit/MonoIC.cpp:962
34 	libxul.so 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:1013
35 	libxul.so 	libxul.so@0xb9f802 	
36 	libxul.so 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:599
37 	libxul.so 	js::mjit::JaegerShot 	js/src/jscntxt.h:2134
38 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:4648
39 	libxul.so 	js::mjit::stubs::UncachedNewHelper 	js/src/methodjit/InvokeHelpers.cpp:381
40 	libxul.so 	CallCompiler::update 	js/src/methodjit/MonoIC.cpp:956
41 	libxul.so 	js::mjit::ic::New 	js/src/methodjit/MonoIC.cpp:1020
42 	libxul.so 	libxul.so@0xb9f802 	
43 	libxul.so 	js::mjit::ic::New 	js/src/methodjit/MonoIC.cpp:599
44 	libxul.so 	js::mjit::JaegerShotAtSafePoint 	js/src/jscntxt.h:2134
45 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:6327
46 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:603
47 	libxul.so 	js_fun_call 	js/src/jsfun.cpp:2149
48 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:4666
49 	libxul.so 	js::mjit::stubs::UncachedCallHelper 	js/src/methodjit/InvokeHelpers.cpp:381
50 	libxul.so 	CallCompiler::update 	js/src/methodjit/MonoIC.cpp:962
51 	libxul.so 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:1013
52 	libxul.so 	libxul.so@0xb9f802 	
53 	libxul.so 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:599
54 	libxul.so 	js::mjit::JaegerShot 	js/src/jscntxt.h:2134
55 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:600
56 	libxul.so 	js::ExternalInvoke 	js/src/jsinterp.cpp:806
57 	libxul.so 	JS_CallFunctionValue 	js/src/jsapi.cpp:5079
58 	libxul.so 	nsXPCWrappedJSClass::CallMethod 	js/src/xpconnect/src/xpcwrappedjsclass.cpp:1664
59 	libxul.so 	nsXPCWrappedJS::CallMethod 	js/src/xpconnect/src/xpcwrappedjs.cpp:587
60 	libxul.so 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:134
61 	libxul.so 	libxul.so@0x94f8a4 	
62 	libxul.so 	nsDOMEventListenerWrapper::HandleEvent 	content/events/src/nsDOMEventTargetHelper.cpp:66
63 		@0x43174f5f 	
64 	libxul.so 	nsEventListenerManager::HandleEventSubType 	content/events/src/nsEventListenerManager.cpp:1141
65 	libxul.so 	nsEventListenerManager::HandleEventInternal 	content/events/src/nsEventListenerManager.cpp:1236
66 	libxul.so 	nsEventTargetChainItem::HandleEvent 	content/events/src/nsEventListenerManager.h:146
67 	libxul.so 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:346
68 	libxul.so 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:650
69 	libxul.so 	nsEventDispatcher::DispatchDOMEvent 	content/events/src/nsEventDispatcher.cpp:711
70 	libxul.so 	nsDOMEventTargetHelper::DispatchDOMEvent 	content/events/src/nsDOMEventTargetHelper.cpp:228
71 	libxul.so 	nsXMLHttpRequest::ChangeState 	nsCOMPtr.h:492
72 	libxul.so 	nsXMLHttpRequest::RequestCompleted 	nsTSubstring.h:593
73 	libxul.so 	nsXMLHttpRequest::OnStopRequest 	content/base/src/nsXMLHttpRequest.cpp:1724
74 	libxul.so 	nsCORSListenerProxy::OnStopRequest 	content/base/src/nsCrossSiteListenerProxy.cpp:618
75 	libxul.so 	nsHTTPCompressConv::OnStopRequest 	netwerk/streamconv/converters/nsHTTPCompressConv.cpp:128
76 	libxul.so 	mozilla::net::HttpChannelChild::OnStopRequest 	nsCOMPtr.h:663
77 	libxul.so 	mozilla::net::HttpChannelChild::RecvOnStopRequest 	netwerk/protocol/http/HttpChannelChild.cpp:413
78 	libxul.so 	mozilla::net::PHttpChannelChild::OnMessageReceived 	objdir/ipc/ipdl/PHttpChannelChild.cpp:583
79 	libxul.so 	mozilla::dom::PContentChild::OnMessageReceived 	objdir/ipc/ipdl/PContentChild.cpp:1171
80 	libxul.so 	mozilla::ipc::AsyncChannel::OnDispatchMessage 	ipc/glue/AsyncChannel.cpp:261
81 	libxul.so 	mozilla::ipc::RPCChannel::OnMaybeDequeueOne 	ipc/glue/RPCChannel.cpp:435
82 	libxul.so 	RunnableMethod<mozilla::ipc::RPCChannel, bool , Tuple0>::Run 	ipc/chromium/src/base/task.h:308
83 	libxul.so 	mozilla::ipc::RPCChannel::DequeueTask::Run 	RPCChannel.h:485
84 	libxul.so 	MessageLoop::RunTask 	ipc/chromium/src/base/message_loop.cc:343
85 	libxul.so 	MessageLoop::DeferOrRunPendingTask 	ipc/chromium/src/base/message_loop.cc:353
86 	libxul.so 	MessageLoop::DoWork 	ipc/chromium/src/base/message_loop.cc:450
87 	libxul.so 	mozilla::ipc::DoWorkRunnable::Run 	ipc/glue/MessagePump.cpp:71
88 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:618
89 	libxul.so 	NS_ProcessNextEvent_P 	objdir/xpcom/build/nsThreadUtils.cpp:250
90 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:111
91 	libxul.so 	mozilla::ipc::MessagePumpForChildProcess::Run 	ipc/glue/MessagePump.cpp:230
92 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:219
93 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:511
94 	libxul.so 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:191
95 	libxul.so 	XRE_RunAppShell 	toolkit/xre/nsEmbedFunctions.cpp:673
96 	libxul.so 	mozilla::ipc::MessagePumpForChildProcess::Run 	ipc/glue/MessagePump.cpp:222
97 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:219
98 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:511
99 	libxul.so 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp:514
100 	libmozutils.so 	libmozutils.so@0x558a 	
101 	libmozutils.so 	libmozutils.so@0x8f16 	
102 	libmozutils.so 	libmozutils.so@0x8f18 	
103 	plugin-container 	plugin-container@0x3d43 	
104 	plugin-container 	plugin-container@0x11a4 	
105 	plugin-container 	plugin-container@0x1133 	
106 	libc.so 	libc.so@0x14db0

Comment 1

[Aaron Train [:aaronmt]]

SImpler STR:
1. http://zippityserver.appspot.com
2. Tap Startup graph in the top header

Comment 2

[Robert Longson [:longsonr]]

Can you attach the crashing page as an attachment somehow?

Comment 3

[Robert Longson [:longsonr]]

Use an older nightly, do the steps that cause the crash and attach the page that crashes a newer nightly perhaps?

Comment 4

[Tony Chung [:tchung]]

(In reply to comment #3)
> Use an older nightly, do the steps that cause the crash and attach the page
> that crashes a newer nightly perhaps?

it's the page of the URL i added up top.  is view sourcing it not enough?

I dont have time to track down a regression range as i'm traveling this week.  But if someone else can help find a range, please comment.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> it's the page of the URL i added up top.

I can confirm this crash on my Nexus S, if I view that URL and click the "update" button at the bottom.

Comment 6

[Daniel Holbert [:dholbert]]

> is view sourcing it not enough?

No - the SVG graph is dynamically generated (and it pulls in external js/css files), so view-source isn't enough.

> do the steps that cause the crash and attach the page
> that crashes a newer nightly perhaps?

You can't save pages in Fennec. I tried saving the page in Firefox and viewing it from fennec, but that wouldn't crash.

I'm guessing the crash has to do with the process of dynamically generating the graph -- and the static page I saved has an already-generated graph, so it doesn't crash.

Comment 7

[Daniel Holbert [:dholbert]]

I'm going to generate a TryServer build with some debugging printfs around the crashing region of code, and then I'll give that build a spin and see what happens.

Comment 8

[Daniel Holbert [:dholbert]]

Flagging as possibly security-sensitive, since it looks like we may be reading memory outside the bounds of an array, which is scary.

Comment 9

[Daniel Holbert [:dholbert]]

Ah, I can reproduce with Desktop firefox, too -- it's just that it's a new crash in today's nightly, so I didn't hit it until I upgraded.

We're crashing because we hit EndsWithWhitespace with text->GetLength() == 0, and then we examine text->CharAt(text->GetLength() - 1).

Comment 10

[Daniel Holbert [:dholbert]]

(Crash report with today's Nightly: bp-241dc463-4c33-42d9-ac7d-be4212110505 )

Comment 11

[Daniel Holbert [:dholbert]]

When we make the faulty call to EndsWithWhitespace, our frame tree looks like this:
> SVGG(g)(7)@0x7fa5ff7472a8 next=0x7fa5ff1f2258 {0,0,0,0} [content=0x7fa6054b3eb0] [sc=0x7fa5ffe903d0]<
>   SVGText(text)(0)@0x7fa5ff7478c0 {0,0,0,0} [content=0x7fa600505500] [sc=0x7fa5ff747820]<
>     SVGTSpan(tspan)(0)@0x7fa5ff747a10 {0,0,0,0} [content=0x7fa5ff702500] [sc=0x7fa5ff747970]<
>       SVGGlyph(0)@0x7fa5ff747b58 {0,0,0,0} [content=0x7fa60958fc80] [sc=0x7fa5ff747ab8]
>       SVGGlyph(1)@0x7fa5ff749998 {0,0,0,0} [content=0x7fa609689300] [sc=0x7fa5ff747ab8]
>     >
>   >
> >
EndsWithWhitespace is called on the second nsSVGGlyphFrame there.
And this is all inside of a GetBBox() call on the SVGText frame's element.

Comment 12

[Daniel Holbert [:dholbert]]

So the STR I'm using are:
 1. Load the page in this bug's URL field
 2. Click "Update" at the bottom.

We seem to hit this issue on the order of 10 times each time I do that. (Right now I've added code to make EndsWithWhitespace return PR_FALSE for zero-length text nodes, so that we can keep going).

Each time we hit this, we're in a situation like Comment 11, with a pair of  nsSVGGlyphFrame -- the first with a nonzero-length text fragment, and the second with a zero-length text fragment.  (The fragment is zero-length from the time the second nsSVGGlyphFrame is created, too -- I added code to Init() to check that.)

The nonzero-length nsSVGGlyphFrames in each pair have to the following text fragments (which I'm comma-separating) 0k, 10k, 20k, 30k, 40k, 50k, 60k

Comment 13

[Daniel Holbert [:dholbert]]

> 0k, 10k, 20k, 30k, 40k, 50k, 60k
These are the labels on the Y axis in the final graph, btw.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1 (crashes nightlies after 2011-05-05)

Ok, here's a standalone reduced testcase.

To make this standalone, I've directly copied in the required highcharts.js and jquery.js libraries.  Those make up the bulk of the file.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: strawman patch (plus some failing assertions)

So here's a strawman patch that just returns makes nsSVGGlyphFrame::EndsWithWhitespace return false for empty strings.  It also asserts whenever such a call is made, as well as when the nsSVGGlyphFrame is Init()ialized.

This fixes the crash, but I doubt it's what we actually want -- it seems odd that we'd create a nsSVGGlyphFrame for the empty string (though we do, for some reason, as the Init() assertion-failure shows).

Now that we've got a standalone testcase (and we've determined it's not a mobile-specific crash), I'm hoping that longsonr can dig a little deeper when he gets a chance.

Alternately, jwatt mentioned on IRC that this might be a good bug to get heycam familiar with our SVG text impl, since he's going to be working on that soon.  

So for now, I leave this to one of them. :)

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 530404 [details] [diff] [review]
strawman patch (plus some failing assertions)

> nsSVGGlyphFrame::GetCharacterPositions(nsTArray<CharacterPosition>* aCharacterPositions,
>                                        float aMetricsScale)
> {
>   PRUint32 strLength = mTextRun->GetLength();
>-  NS_ASSERTION(strLength > 0, "no text");
>+  NS_ASSERTION(strLength > 0, "no text"); // XXXdholbert Does this always hold?

Note: over IRC, longsonr pointed out the above (existing) assertion to me, as a partial reason for why his newly-added EndsWithWhitespace() function assumes that we have a nonzero-length text.

However, note that the above assertion deals with mTextRun (a gfxTextRun that seems to be lazily set up in EnsureTextRun), whereas nsSVGGlyphFrame::EndsWithWhitespace() deals with mContent->GetText().

From looking at the EnsureTextRun impl, it looks like the first thing we do is to call GetCharacterData() (effectively pulling in mContent->GetText(), and if that gets the empty string, then we bail out of EnsureTextRun.

So AFAICT, the "no text" assertion in GetCharacterPositions is indeed valid, but it has little-to-no bearing on whether or not  mContent->GetText() would be empty.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: reduced testcase 2 (crashes nightlies after 2011-05-05)

Comment 18

[Cameron McCormack (:heycam)]

Simply creating an empty text node with document.createTextNode("") and appending it to an SVG <text> element will also cause the crash.  Given this, and the fact that roc says it's normal for CSS text frames to exist for zero length Text nodes, I think removing the assertions from Daniel's strawman patch and leaving the early return is probably the right approach.

Comment 19

[Cameron McCormack (:heycam)]

Attachment: dholbert's patch minus assertions plus crashtests

Comment 20

[Robert Longson [:longsonr]]

Comment on attachment 530542 [details] [diff] [review]
dholbert's patch minus assertions plus crashtests

That won't do the right thing functionally, though it will fix the crash. I think I should take this.

Comment 21

[Cameron McCormack (:heycam)]

Sure.  What's the behaviour we want?

Comment 22

[Robert Longson [:longsonr]]

We want not to call this method at all when the text has no length. That means either 

a) adjusting getnextglyphfragment so that it skips 0 length text nodes.

or

b) deleting the frame when the text length becomes 0

b) is best though maybe harder and could be a follow up. a) should be easy.

Comment 23

[Robert Longson [:longsonr]]

Attachment: hg changeset patch

Comment 24

[Robert Longson [:longsonr]]

Having read through all the comments above including Roc's statment about frames existing with 0 length text, it seems that b) would not work. Good job I chose a)

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 530592 [details] [diff] [review]
hg changeset patch

Looks good to me!

This could probably stand to have a tryserver sanity-check before checkin.

Comment 26

[Robert Longson [:longsonr]]

Do you want to do one, it's about home time for me and I'm going to need someone to check this in for me too. Pretty please :-)

Comment 27

[Daniel Holbert [:dholbert]]

I can take care of it. I'll do the try push (sans-crashtests*) and (assuming that passes) then the m-c push.

*to avoid revealing PoC testcases to anyone looking to exploit a security-sensitive bug. Not a huge deal in this case, since the bug only affects nightly builds and only 2 affected nightlies have been released, but probably still good to be cautious.

Comment 28

[Daniel Holbert [:dholbert]]

Just noticed that the patch introduces this build warning:

> nsFrame.h:272:18: warning: ‘virtual PRBool nsFrame::IsEmpty()’ was hidden
> nsSVGGlyphFrame.h:165:95: warning:   by ‘virtual PRBool nsSVGGlyphFrame::IsEmpty() const’

Perhaps we could call the new method "IsTextEmpty()" instead?

Comment 29

[Steven Michaud [:smichaud] (Retired)]

For what it's worth, this has also become a topcrasher on Mac trunk.  I don't know if it's reproducible there:

https://crash-stats.mozilla.com/query/query?product=Firefox&version=ALL%3AALL&range_value=1&range_unit=weeks&date=05%2F06%2F2011+10%3A22%3A23&query_search=signature&query_type=contains&query=nsSVGGlyphFrame%3A%3AEndsWithWhitespace&reason=&build_id=&process_type=any&hang_type=any&do_query=1

Comment 30

[Daniel Holbert [:dholbert]]

>  I don't know if it's reproducible there:
Yes, it should be reproducible in all trunk builds, AFAIK.

Right now I'm planning to land the patch as-is today, and post a followup here with r?longsonr to rename the method & address comment 28.

Comment 31

[Robert Longson [:longsonr]]

R+ on renaming

Comment 32

[Daniel Holbert [:dholbert]]

Attachment: followup: s/IsEmpty/IsTextEmpty/

pushed patch: http://hg.mozilla.org/mozilla-central/rev/cfd13e3ed2cf

Leaving open pending comment 28. I've attached a followup to fix that.

Comment 33

[Daniel Holbert [:dholbert]]

Ah, I missed comment 31. Thanks!  I'll push the followup then.

Comment 34

[Daniel Holbert [:dholbert]]

Landed followup: http://hg.mozilla.org/mozilla-central/rev/0723af2574f1

Assuming the fix sticks, I imagine we can un-hide this bug in a few days, seeing as it only affected 2 Nightly builds and no other releases.

Comment 35

[Tony Chung [:tchung]]

Thanks for all the great work Daniel and Robert!   Needs verification on the next nightly.

Comment 37

[Tony Chung [:tchung]]

Verified fix on Fennec trunk: Mozilla/5.0 (Android; Linux armv71; rv:6.0a1) Gecko/20110507 Firefox/6.0a1 Fennec/6.0a1.  

Can someone please check if desktop is fixed also?

Comment 38

[Daniel Holbert [:dholbert]]

Yup, desktop is fixed.
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110507 Firefox/6.0a1