767996 - crash in nsSVGUtils::InvalidateBounds

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Scoobidiver (away)]

It first appeared in 16.0a1/20120625 and is currently #1 top crasher in today's build with around 60 crashes an hour!
The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cb2904476d14&tochange=c659521a7a9a
It's likely a regression from bug 738192.

One comment says: "Google Maps"

Signature 	nsSVGUtils::InvalidateBounds(nsIFrame*, bool, nsRect const*, unsigned int) More Reports Search
UUID	11b0bab2-65b4-4bc6-819e-c04cf2120625
Date Processed	2012-06-25 14:49:53
Uptime	11
Last Crash	1.1 weeks before submission
Install Age	10 seconds since version was first installed.
Install Time	2012-06-25 14:50:01
Product	Firefox
Version	16.0a1
Build ID	20120625030537
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 23 stepping 10
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x4
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x2e32, AdapterSubsysID: 0dc1105b, AdapterDriverVersion: 8.15.10.1749
D3D10 Layers? D3D10 Layers- 
EMCheckCompatibility	False
Adapter Vendor ID	0x8086
Adapter Device ID	0x2e32
Total Virtual Memory	2147352576
Available Virtual Memory	1686474752
System Memory Use Percentage	62
Available Page File	2642030592
Available Physical Memory	786591744

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsSVGUtils::InvalidateBounds 	layout/svg/base/src/nsSVGUtils.cpp:706
1 	xul.dll 	DoApplyRenderingChangeToTree 	layout/base/nsCSSFrameConstructor.cpp:7589
2 	xul.dll 	ApplyRenderingChangeToTree 	layout/base/nsCSSFrameConstructor.cpp:7649
3 	xul.dll 	nsCSSFrameConstructor::ProcessRestyledFrames 	layout/base/nsCSSFrameConstructor.cpp:7894
4 	xul.dll 	mozilla::css::RestyleTracker::ProcessOneRestyle 	layout/base/RestyleTracker.cpp:131
5 	xul.dll 	mozilla::css::RestyleTracker::DoProcessRestyles 	layout/base/RestyleTracker.cpp:238
6 	xul.dll 	nsCSSFrameConstructor::ProcessPendingRestyles 	layout/base/nsCSSFrameConstructor.cpp:11631
7 	xul.dll 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:3813
8 	xul.dll 	PresShell::HandlePostedReflowCallbacks 	layout/base/nsPresShell.cpp:3692
9 	xul.dll 	PresShell::ProcessReflowCommands 	layout/base/nsPresShell.cpp:7536
10 	xul.dll 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:3852
11 	xul.dll 	nsDocument::FlushPendingNotifications 	content/base/src/nsDocument.cpp:6286
12 	xul.dll 	nsBoxObject::GetPresShell 	layout/xul/base/src/nsBoxObject.cpp:147
13 	xul.dll 	nsBoxObject::GetFrame 	layout/xul/base/src/nsBoxObject.cpp:114
14 	xul.dll 	nsBoxObject::GetScreenPosition 	layout/xul/base/src/nsBoxObject.cpp:222
15 	xul.dll 	nsBoxObject::GetScreenX 	layout/xul/base/src/nsBoxObject.cpp:272
16 	xul.dll 	nsIBoxObject_GetScreenX 	obj-firefox/js/xpconnect/src/dom_quickstubs.cpp:21538
17 	mozjs.dll 	js::GetPropertyOperation 	js/src/jsinterpinlines.h:231
18 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2333
19 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:322
20 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:354
21 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5499
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsSVGUtils%3A%3AInvalidateBounds%28nsIFrame*%2C+bool%2C+nsRect+const*%2C+unsigned+int%29
https://crash-stats.mozilla.com/report/list?signature=nsSVGUtils%3A%3AInvalidateBounds

Comment 1

[Jonathan Watt [:jwatt]]

#1 top crash? And people claim SVG isn't popular. :)

Almost certainly this is because at the end of UpdateBounds the new code dereferences aFrame without checking whether it is null. That's easy to fix. The question is, how is it possible for us to get there with null aFrame? It shouldn't be possible for SVG frames to be created without a parent nsSVGOuterSVGFrame, so this must be something to do with frame teardown, I'd guess.

Anyway, I'll push a null check to m-c to get this to stop crashing.

Comment 2

[Jonathan Watt [:jwatt]]

Pushed https://tbpl.mozilla.org/?rev=3fe96d84e281

Comment 3

[Robert Longson [:longsonr]]

Let's say nsSVGUtils::InvalidateBounds is called with aFrame as an outer svg frame (but not the root frame). We might then advance to our parent unconditionally here

653   } else {
654     invalidArea = aFrame->GetVisualOverflowRect();
655     // GetVisualOverflowRect() already includes filter effects and transforms,
656     // so advance to our parent before the loop below:
657     invalidArea += aFrame->GetPosition();
658     aFrame = aFrame->GetParent();
659   }

And then we're no longer playing in SVG world so we may well not get aFrame->GetStateBits() & NS_STATE_IS_OUTER_SVG

If I'm right, I think you need a better patch than the null check.

Comment 4

[Jonathan Watt [:jwatt]]

nsSVGUtils::InvalidateBounds should never be passed an nsSVGOuterSVGFrame and I don't think it is, although it does look like the logic there could do with some hardening.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: additional patch

I think we should also land this.

Comment 6

[Jonathan Watt [:jwatt]]

The testcase in bug 768087 proves you're right, Robert, we are getting into nsSVGUtils::InvalidateBounds via DoApplyRenderingChangeToTree where the frame passed in is an nsSVGOuterSVGFrame. So we definitely want this patch.

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 636331 [details] [diff] [review]
additional patch

Given the number of crashes, I went ahead and pushed this r=me to m-c so it doesn't miss the next nightly. Without a testcase to reproduce the crash I'm not sure this is the cause rather than the lack of the null check added earlier, but better to have this asap rather than wait.

Comment 8

[Jonathan Watt [:jwatt]]

The second push was https://hg.mozilla.org/mozilla-central/rev/5424abbda7a8

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #7)
> Given the number of crashes, I went ahead and pushed this r=me to m-c so it
> doesn't miss the next nightly. Without a testcase to reproduce the crash I'm
> not sure this is the cause rather than the lack of the null check added
> earlier, but better to have this asap rather than wait.

Trunk (from this morning) is constantly crashing for me when opening the link below. Often only after clicking "Get Directions":

http://bit.ly/LM6yus

I imported the second patch and built trunk, no crashes so far. I did not apply the patch containing the null check. Maybe someone can debug and create test case from it?

Comment 10

[Mathieu Marquer]

Always reproducible for me when just opening http://flightaware.com/live/flight/AAL1929/history/20120624/1345Z/KSFO/KLAX

Comment 11

[Scoobidiver (away)]

I can reproduce with the URL in comment 9.

Comment 12

[Thomas Ahlblom]

Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/16.0 Firefox/16.0
http://hg.mozilla.org/mozilla-central/rev/c659521a7a9a

Crashes on http://marinetraffic.com/ais/ when running Firefox under Linux.

Comment 13

[Jonathan Watt [:jwatt]]

(In reply to Tim Taubert [:ttaubert] from comment #9)
> I imported the second patch and built trunk, no crashes so far. I did not
> apply the patch containing the null check. Maybe someone can debug and
> create test case from it?

I can't reproduce with any of the three links given above using a build with the patches I landed, so closing as fixed. It would still be good to get a testcase checked in.

Comment 14

[Jonathan Watt [:jwatt]]

Robert, thanks for drawing my attention to the danger of an nsSVGOuterSVGFrame being passed to nsSVGUtils::InvalidateBounds - well spotted!

Comment 16

[Alex Keybl [:akeybl]]

Found on 16, fixed in 16 now. No need to track.