Closed Bug 835978 Opened 7 years ago Closed 7 years ago

crash in nsDisplayList::HitTest

Categories

(Core :: SVG, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla21
Tracking Status
firefox18 --- affected
firefox19 --- affected
firefox20 --- verified
firefox21 --- verified
firefox-esr17 --- wontfix

People

(Reporter: kbrosnan, Assigned: longsonr)

References

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(3 files)

< jonathancutrell> Hey folks - I'm a front end developer, and I'm looking for a bit of insight into why Firefox is crashing on an app I'm developing.


This bug was filed from the Socorro interface and is 
report bp-ad0abc57-a8f1-49bc-9225-9f11e2130129 .
=============================================================
Component: Layout: HTML Frames → Layout: View Rendering
The crash stack doesn't really provide any clues. We'll need more. Can we talk to the developer? A testcase? STR?
Hey folks - I am the developer.

Basically, this error occurs randomly while using the interface at http://believecampaign.org ; in particular, when interacting with the modals. The "close" action on any given modal seems to be the most common trigger for the problem.

A few notes: the application relies heavily on Polymaps, which is an SVG-based map plugin. We're loading in a map and displaying it as the full width background, and overlaying SVG circles as "anchors" for the messages that show on top of those anchors.

Hopefully this is helpful/insightful.
Perfect. I was able to reproduce right away.
So an interesting addition - it seems that enabling/disabling a JS pushHistory call significantly reduces the occurrence of this bug, but doesn't  completely eradicate it. To reproduce the bug almost without variation, visit http://believecampaign.org/beliefs/52 directly, and click the black "curtain".

The frequency of occurrence of this specific scenario is 100% with pushHistory, and MUCH less without, but, as I mentioned before, still not eradicated.
We can flush while we are hittesting, destroying a frame that's on our list of potential hits. Here is the stack.

#34 0x0000000101dfaf0a in nsDocument::FlushPendingNotifications (this=0x10f245000, aType=Flush_Layout) at /Users/tim/ff/src/content/base/src/nsDocument.cpp:6723
#35 0x0000000102bb7861 in SVGContentUtils::GetCTM (aElement=0x149dce8c0, aScreenCTM=true) at /Users/tim/ff/src/content/svg/content/src/SVGContentUtils.cpp:252
#36 0x0000000102aa2caf in nsSVGUtils::GetStrokeTransform (aFrame=0x149f1c968) at /Users/tim/ff/src/layout/svg/nsSVGUtils.cpp:1366
#37 0x0000000102aa3b26 in nsSVGUtils::SetupCairoStrokeGeometry (aFrame=0x149f1c968, aContext=0x10f056380, aObjectPaint=0x0) at /Users/tim/ff/src/layout/svg/nsSVGUtils.cpp:1662
#38 0x0000000102aa3c68 in nsSVGUtils::SetupCairoStrokeHitGeometry (aFrame=0x149f1c968, aContext=0x10f056380, aObjectPaint=0x0) at /Users/tim/ff/src/layout/svg/nsSVGUtils.cpp:1765
#39 0x0000000102a94335 in nsSVGPathGeometryFrame::GetFrameForPoint (this=0x149f1c968, aPoint=@0x7fff5fbf8070) at /Users/tim/ff/src/layout/svg/nsSVGPathGeometryFrame.cpp:255
#40 0x0000000102a9352d in nsDisplaySVGPathGeometry::HitTest (this=0x11d2c2ae0, aBuilder=0x7fff5fbfbe10, aRect=@0x7fff5fbf8520, aState=0x7fff5fbfba80, aOutFrames=0x7fff5fbf8170) at /Users/tim/ff/src/layout/svg/nsSVGPathGeometryFrame.cpp:81

Can we not flush here?
Component: Layout: View Rendering → SVG
OS: Mac OS X → All
The steps in comment 4 plus moving your mouse immediately before and after the click make it happen pretty reliably.
Keywords: reproducible
Assignee: nobody → longsonr
Attached patch patchSplinter Review
Attachment #708085 - Flags: review?(jwatt)
Comment on attachment 708085 [details] [diff] [review]
patch

Hmm, yeah, it does seem like a bad idea for nsSVGUtils::GetStrokeTransform to flush. r=me.
Attachment #708085 - Flags: review?(jwatt) → review+
Keywords: regression
Blocks: 528332
Awesome, thanks guys!

This happens all the way back to Firefox 16 and even further back, so if the patch is safe enough we should get it uplifted appropriately.
It's pretty safe, there are 3 callers of SVGContentUtils::GetCTM, the patch moves the flush call from SVGContentUtils::GetCTM to the other two callers, thereby removing it from the nsSVGUtils::GetStrokeTransform path

It will need adjusting for branches though as the location of the SVGContentUtils::GetCTM callers have moved (the same functions are there but in another file)
The patch will apply to Aurora but not to Beta.
As the original crash report is from 18, I assume that everything newer is affected as well.
And I got it in 16 too, and the likely regression bug has TM 15, so esr17 is affected too I guess.
Backported the patch to beta, w/ Robert's help, since he didn't have a beta branch handy.  Basically, the trunk patch's changes to SVGLocatableElement need to be applied to the corresponding methods in nsSVGGraphicElement and nsSVGSVGElement.
Attachment #708262 - Flags: feedback?
Attachment #708262 - Flags: feedback? → feedback?(longsonr)
...and here's the patch backported to ESR17. (Same as the beta patch, but with s/SVGContentUtils/nsSVGUtils/ basically.)
Attachment #708270 - Flags: feedback?(longsonr)
Attachment #708262 - Attachment description: patch backported to beta branch → patch backported to beta (firefox 18)
Attachment #708262 - Flags: feedback?(longsonr) → feedback+
Attachment #708270 - Flags: feedback?(longsonr) → feedback+
https://hg.mozilla.org/mozilla-central/rev/0bbcb088b1b8

Should this have a test?
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment on attachment 708085 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 528332
User impact if declined: crashes occasionally with SVG pages that use non-scaling-stroke and some other DOM manipulation
Testing completed (on m-c, etc.): patch landed, no automated test though. Does not crash on example site any more.
Risk to taking this patch (and alternatives if risky): Low risk, just moves some code to callers so that one of the callers does not flush.
String or UUID changes made by this patch: none
Attachment #708085 - Flags: approval-mozilla-aurora?
Attachment #708262 - Flags: approval-mozilla-beta?
Comment on attachment 708270 [details] [diff] [review]
patch backported to esr17 branch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: crash on some SVG sites that use non-scaling-stroke
Fix Landed on Version: 21
Risk to taking this patch (and alternatives if risky): Low Risk.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #708270 - Flags: approval-mozilla-esr17?
(In reply to Ryan VanderMeulen from comment #19)
> Should this have a test?

Ideally, yes. The best we've got right now, though, is a URL and some interactive STR.
 --> adding 'testcase-wanted', in case anyone can make a reduced testcase that crashes in un-patched builds.
Keywords: testcase-wanted
Comment on attachment 708085 [details] [diff] [review]
patch

We'll take this on Aurora, but it's too late for beta 19 now and this has been around since 15 so there's no rush push out the fix.  Also it does not meet ESR landing criteria.
Attachment #708085 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #708262 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #708270 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17-
I reproduced this on Nightly 2013-01-29 using the STR in comment 7.
Verified fixed on FF 20b7 Win 7.
Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0b2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.