Closed Bug 655025 Opened 12 years ago Closed 12 years ago

Crash [@ nsSVGGlyphFrame::EndsWithWhitespace ]


(Core :: SVG, defect)

Not set



Tracking Status
firefox5 --- unaffected
status2.0 --- unaffected
status1.9.2 --- unaffected
status1.9.1 --- unaffected


(Reporter: tchung, Assigned: longsonr)




(4 keywords)

Crash Data


(5 files, 1 obsolete file)

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

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

Crash stack:

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
3) go to URL, click "Startup" > leave the controls as default, and click "Update"
4) Crash @ nsSVGGlyphFrame::EndsWithWhitespace.

- no crash

- Stack trace.
Frame 	Module 	Signature [Expand] 	Source
0 	nsSVGGlyphFrame::EndsWithWhitespace 	nsTextFragment.h:206
1 	nsSVGTextFrame::SetWhitespaceHandling 	layout/svg/base/src/nsSVGTextFrame.cpp:314
2 	nsSVGTextFrame::UpdateGlyphPositioning 	layout/svg/base/src/nsSVGTextFrame.cpp:341
3 	nsSVGTextFrame::InitialUpdate 	layout/svg/base/src/nsSVGTextFrame.cpp:257
4 	nsSVGDisplayContainerFrame::InsertFrames 	layout/generic/nsIFrame.h:1054
5 	nsFrameManager::InsertFrames 	layout/base/nsFrameManager.cpp:488
6 	nsCSSFrameConstructor::AppendFrames 	layout/base/nsCSSFrameConstructor.cpp:5764
7 	nsCSSFrameConstructor::ContentAppended 	layout/base/nsCSSFrameConstructor.cpp:6706
8 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsCSSFrameConstructor.cpp:6316
9 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
10 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
11 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
12 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
13 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
14 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
15 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsChildIterator.h:175
16 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsCSSFrameConstructor.cpp:6337
17 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:4793
18 	nsDocument::FlushPendingNotifications 	nsCOMPtr.h:492
19 	nsGenericElement::GetPrimaryFrame 	nsIContent.h:893
20 	nsSVGGraphicElement::GetBBox 	content/svg/content/src/nsSVGGraphicElement.cpp:95
21 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:199
22 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:3141
23 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1610
24 	js::Interpret 	js/src/jsinterp.cpp:4666
25 	js::mjit::stubs::UncachedCallHelper 	js/src/methodjit/InvokeHelpers.cpp:381
26 	CallCompiler::update 	js/src/methodjit/MonoIC.cpp:962
27 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:1013
29 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:599
30 	js::mjit::JaegerShot 	js/src/jscntxt.h:2134
31 	js::Interpret 	js/src/jsinterp.cpp:4648
32 	js::mjit::stubs::UncachedCallHelper 	js/src/methodjit/InvokeHelpers.cpp:381
33 	CallCompiler::update 	js/src/methodjit/MonoIC.cpp:962
34 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:1013
36 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:599
37 	js::mjit::JaegerShot 	js/src/jscntxt.h:2134
38 	js::Interpret 	js/src/jsinterp.cpp:4648
39 	js::mjit::stubs::UncachedNewHelper 	js/src/methodjit/InvokeHelpers.cpp:381
40 	CallCompiler::update 	js/src/methodjit/MonoIC.cpp:956
41 	js::mjit::ic::New 	js/src/methodjit/MonoIC.cpp:1020
43 	js::mjit::ic::New 	js/src/methodjit/MonoIC.cpp:599
44 	js::mjit::JaegerShotAtSafePoint 	js/src/jscntxt.h:2134
45 	js::Interpret 	js/src/jsinterp.cpp:6327
46 	js::Invoke 	js/src/jsinterp.cpp:603
47 	js_fun_call 	js/src/jsfun.cpp:2149
48 	js::Interpret 	js/src/jsinterp.cpp:4666
49 	js::mjit::stubs::UncachedCallHelper 	js/src/methodjit/InvokeHelpers.cpp:381
50 	CallCompiler::update 	js/src/methodjit/MonoIC.cpp:962
51 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:1013
53 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:599
54 	js::mjit::JaegerShot 	js/src/jscntxt.h:2134
55 	js::Invoke 	js/src/jsinterp.cpp:600
56 	js::ExternalInvoke 	js/src/jsinterp.cpp:806
57 	JS_CallFunctionValue 	js/src/jsapi.cpp:5079
58 	nsXPCWrappedJSClass::CallMethod 	js/src/xpconnect/src/xpcwrappedjsclass.cpp:1664
59 	nsXPCWrappedJS::CallMethod 	js/src/xpconnect/src/xpcwrappedjs.cpp:587
60 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:134
62 	nsDOMEventListenerWrapper::HandleEvent 	content/events/src/nsDOMEventTargetHelper.cpp:66
63 		@0x43174f5f 	
64 	nsEventListenerManager::HandleEventSubType 	content/events/src/nsEventListenerManager.cpp:1141
65 	nsEventListenerManager::HandleEventInternal 	content/events/src/nsEventListenerManager.cpp:1236
66 	nsEventTargetChainItem::HandleEvent 	content/events/src/nsEventListenerManager.h:146
67 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:346
68 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:650
69 	nsEventDispatcher::DispatchDOMEvent 	content/events/src/nsEventDispatcher.cpp:711
70 	nsDOMEventTargetHelper::DispatchDOMEvent 	content/events/src/nsDOMEventTargetHelper.cpp:228
71 	nsXMLHttpRequest::ChangeState 	nsCOMPtr.h:492
72 	nsXMLHttpRequest::RequestCompleted 	nsTSubstring.h:593
73 	nsXMLHttpRequest::OnStopRequest 	content/base/src/nsXMLHttpRequest.cpp:1724
74 	nsCORSListenerProxy::OnStopRequest 	content/base/src/nsCrossSiteListenerProxy.cpp:618
75 	nsHTTPCompressConv::OnStopRequest 	netwerk/streamconv/converters/nsHTTPCompressConv.cpp:128
76 	mozilla::net::HttpChannelChild::OnStopRequest 	nsCOMPtr.h:663
77 	mozilla::net::HttpChannelChild::RecvOnStopRequest 	netwerk/protocol/http/HttpChannelChild.cpp:413
78 	mozilla::net::PHttpChannelChild::OnMessageReceived 	objdir/ipc/ipdl/PHttpChannelChild.cpp:583
79 	mozilla::dom::PContentChild::OnMessageReceived 	objdir/ipc/ipdl/PContentChild.cpp:1171
80 	mozilla::ipc::AsyncChannel::OnDispatchMessage 	ipc/glue/AsyncChannel.cpp:261
81 	mozilla::ipc::RPCChannel::OnMaybeDequeueOne 	ipc/glue/RPCChannel.cpp:435
82 	RunnableMethod<mozilla::ipc::RPCChannel, bool , Tuple0>::Run 	ipc/chromium/src/base/task.h:308
83 	mozilla::ipc::RPCChannel::DequeueTask::Run 	RPCChannel.h:485
84 	MessageLoop::RunTask 	ipc/chromium/src/base/
85 	MessageLoop::DeferOrRunPendingTask 	ipc/chromium/src/base/
86 	MessageLoop::DoWork 	ipc/chromium/src/base/
87 	mozilla::ipc::DoWorkRunnable::Run 	ipc/glue/MessagePump.cpp:71
88 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:618
89 	NS_ProcessNextEvent_P 	objdir/xpcom/build/nsThreadUtils.cpp:250
90 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:111
91 	mozilla::ipc::MessagePumpForChildProcess::Run 	ipc/glue/MessagePump.cpp:230
92 	MessageLoop::RunInternal 	ipc/chromium/src/base/
93 	MessageLoop::Run 	ipc/chromium/src/base/
94 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:191
95 	XRE_RunAppShell 	toolkit/xre/nsEmbedFunctions.cpp:673
96 	mozilla::ipc::MessagePumpForChildProcess::Run 	ipc/glue/MessagePump.cpp:222
97 	MessageLoop::RunInternal 	ipc/chromium/src/base/
98 	MessageLoop::Run 	ipc/chromium/src/base/
99 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp:514
103 	plugin-container 	plugin-container@0x3d43 	
104 	plugin-container 	plugin-container@0x11a4 	
105 	plugin-container 	plugin-container@0x1133 	
SImpler STR:
2. Tap Startup graph in the top header
Component: General → SVG
Product: Fennec → Core
QA Contact: general → general
Can you attach the crashing page as an attachment somehow?
Use an older nightly, do the steps that cause the crash and attach the page that crashes a newer nightly perhaps?
(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.
(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.
> 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.
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.
Flagging as possibly security-sensitive, since it looks like we may be reading memory outside the bounds of an array, which is scary.
Group: core-security
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).
Blocks: 620286
(Crash report with today's Nightly: bp-241dc463-4c33-42d9-ac7d-be4212110505 )
OS: Android → All
Hardware: ARM → All
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.
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
> 0k, 10k, 20k, 30k, 40k, 50k, 60k
These are the labels on the Y axis in the final graph, btw.
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.
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 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.
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.
Assignee: nobody → cam
Attachment #530542 - Flags: review?(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.
Attachment #530542 - Flags: review?(longsonr) → review-
Sure.  What's the behaviour we want?
Assignee: cam → nobody
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.


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.
Assignee: nobody → longsonr
Attachment #530542 - Attachment is obsolete: true
Attachment #530592 - Flags: review?(dholbert)
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 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.
Attachment #530592 - Flags: review?(dholbert) → review+
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 :-)
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.
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?
>  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.
R+ on renaming
pushed patch:

Leaving open pending comment 28. I've attached a followup to fix that.
Attachment #530679 - Flags: review?
Ah, I missed comment 31. Thanks!  I'll push the followup then.
Attachment #530679 - Flags: review? → review+
Landed followup:

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.
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Flags: in-testsuite+
Thanks for all the great work Daniel and Robert!   Needs verification on the next nightly.
Summary: Crash [@ nsSVGGlyphFrame::EndsWithWhitespace ] on mobile zippity page → Crash [@ nsSVGGlyphFrame::EndsWithWhitespace ]
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?
Yup, desktop is fixed.
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110507 Firefox/6.0a1
Blocks: 655407
Crash Signature: [@ nsSVGGlyphFrame::EndsWithWhitespace ]
Blocks: 694298
No longer blocks: 694298
Depends on: 694298
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.