Last Comment Bug 767996 - crash in nsSVGUtils::InvalidateBounds
: crash in nsSVGUtils::InvalidateBounds
Status: RESOLVED FIXED
[startupcrash]
: crash, regression, reproducible, topcrash
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 16 Branch
: All All
: -- critical with 1 vote (vote)
: ---
Assigned To: Jonathan Watt [:jwatt] (catching up after vacation)
:
Mentors:
: 768237 769590 (view as bug list)
Depends on:
Blocks: 738192
  Show dependency treegraph
 
Reported: 2012-06-25 08:03 PDT by Scoobidiver (away)
Modified: 2012-06-29 07:47 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
additional patch (2.39 KB, patch)
2012-06-25 08:58 PDT, Jonathan Watt [:jwatt] (catching up after vacation)
no flags Details | Diff | Splinter Review

Description Scoobidiver (away) 2012-06-25 08:03:01 PDT
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] (catching up after vacation) 2012-06-25 08:11:30 PDT
#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] (catching up after vacation) 2012-06-25 08:20:36 PDT
Pushed https://tbpl.mozilla.org/?rev=3fe96d84e281
Comment 3 Robert Longson 2012-06-25 08:28:57 PDT
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] (catching up after vacation) 2012-06-25 08:42:45 PDT
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] (catching up after vacation) 2012-06-25 08:58:17 PDT
Created attachment 636331 [details] [diff] [review]
additional patch

I think we should also land this.
Comment 6 Jonathan Watt [:jwatt] (catching up after vacation) 2012-06-25 10:51:02 PDT
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] (catching up after vacation) 2012-06-25 10:58:58 PDT
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] (catching up after vacation) 2012-06-25 11:00:12 PDT
The second push was https://hg.mozilla.org/mozilla-central/rev/5424abbda7a8
Comment 9 Tim Taubert [:ttaubert] 2012-06-25 13:27:53 PDT
(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 2012-06-25 13:59:55 PDT
Always reproducible for me when just opening http://flightaware.com/live/flight/AAL1929/history/20120624/1345Z/KSFO/KLAX
Comment 11 Scoobidiver (away) 2012-06-25 14:15:02 PDT
I can reproduce with the URL in comment 9.
Comment 12 Thomas Ahlblom 2012-06-25 14:23:00 PDT
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] (catching up after vacation) 2012-06-25 14:54:51 PDT
(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] (catching up after vacation) 2012-06-25 14:56:51 PDT
Robert, thanks for drawing my attention to the danger of an nsSVGOuterSVGFrame being passed to nsSVGUtils::InvalidateBounds - well spotted!
Comment 15 Jonathan Watt [:jwatt] (catching up after vacation) 2012-06-25 15:17:28 PDT
*** Bug 768237 has been marked as a duplicate of this bug. ***
Comment 16 Alex Keybl [:akeybl] 2012-06-26 13:18:09 PDT
Found on 16, fixed in 16 now. No need to track.
Comment 17 Scoobidiver (away) 2012-06-29 07:47:04 PDT
*** Bug 769590 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.