835978 - crash in nsDisplayList::HitTest

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

< 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 .
=============================================================

Comment 1

[Timothy Nikkel (:tnikkel)]

The crash stack doesn't really provide any clues. We'll need more. Can we talk to the developer? A testcase? STR?

Comment 2

[Jonathan Cutrell]

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.

Comment 3

[Timothy Nikkel (:tnikkel)]

Perfect. I was able to reproduce right away.

Comment 4

[Jonathan Cutrell]

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.

Comment 5

[Timothy Nikkel (:tnikkel)]

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?

Comment 6

[Timothy Nikkel (:tnikkel)]

See comment 5.

Comment 7

[Timothy Nikkel (:tnikkel)]

The steps in comment 4 plus moving your mouse immediately before and after the click make it happen pretty reliably.

Comment 8

[Robert Longson [:longsonr]]

Attachment: patch

Comment 9

[Jonathan Watt [: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.

Comment 10

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=eb1b95818869

Comment 11

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bbcb088b1b8

Comment 12

[Timothy Nikkel (:tnikkel)]

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.

Comment 13

[Robert Longson [:longsonr]]

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)

Comment 14

[Robert Longson [:longsonr]]

The patch will apply to Aurora but not to Beta.

Comment 15

[Robert Kaiser]

As the original crash report is from 18, I assume that everything newer is affected as well.

Comment 16

[Timothy Nikkel (:tnikkel)]

And I got it in 16 too, and the likely regression bug has TM 15, so esr17 is affected too I guess.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: patch backported to beta (firefox 18)

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.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: patch backported to esr17 branch

...and here's the patch backported to ESR17. (Same as the beta patch, but with s/SVGContentUtils/nsSVGUtils/ basically.)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0bbcb088b1b8

Should this have a test?

Comment 20

[Robert Longson [:longsonr]]

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

Comment 21

[Robert Longson [:longsonr]]

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.

Comment 22

[Daniel Holbert [:dholbert]]

(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.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 24

[Robert Longson [:longsonr]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/26872b25c4d0

Comment 25

[Paul Silaghi, QA [:pauly]]

I reproduced this on Nightly 2013-01-29 using the STR in comment 7.
Verified fixed on FF 20b7 Win 7.

Comment 26

[Paul Silaghi, QA [:pauly]]

Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0b2