| Summary: | Lonely stop crashes | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Philip Rogers <pdr> | ||||||
| Component: | SVG | Assignee: | Stephen Chenney <schenney> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, darin, d-r, esprehn+autocc, fmalita, glenn, kondapallykalyan, rwlbuis, schenney, zimmermann | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | 86941 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Philip Rogers
2012-05-31 06:06:46 PDT
Hi Philip, Have you tried with Niko's change to not create a renderer for stops? (In reply to comment #1) > Hi Philip, > > Have you tried with Niko's change to not create a renderer for stops? I have now and it does fix it. I'll mark this as depending on wkbug.com/86941 This is also a top crasher in Chrome, on pages containing the Meebo bar, that has the following stack. The only real potential cause is a null renderer or null style. I will post a fix in an effort to get this crash addressed, while waiting for Bug 86941 and 87373 to be fixed. Thread 0 *CRASHED* ( EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE @ 0x00000004 ) 0x7b6bc17c [Google Chrome Framework] - ../../WTF/wtf/RefPtr.h:58] WebCore::SVGStopElement::stopColorIncludingOpacity 0x7b6831ac [Google Chrome Framework] - SVGGradientElement.cpp:150 WebCore::SVGGradientElement::buildStops 0x7b68cedd [Google Chrome Framework] - SVGLinearGradientElement.cpp:150 WebCore::SVGLinearGradientElement::collectGradientAttributes 0x7b6066c6 [Google Chrome Framework] - RenderSVGResourceLinearGradient.cpp:45 WebCore::RenderSVGResourceLinearGradient::collectGradientAttributes 0x7b6059ad [Google Chrome Framework] - RenderSVGResourceGradient.cpp:131 WebCore::RenderSVGResourceGradient::applyResource 0x7b605db0 [Google Chrome Framework] + 0x01e78db0] non-virtual thunk to WebCore::RenderSVGResourceGradient::applyResource(WebCore::RenderObject*, WebCore::RenderStyle*, WebCore::GraphicsContext*&, unsigned short) 0x7b60e27b [Google Chrome Framework] - RenderSVGShape.cpp:254 WebCore::RenderSVGShape::fillShape 0x7b60e4a0 [Google Chrome Framework] - RenderSVGShape.cpp:291 WebCore::RenderSVGShape::fillAndStrokePath 0x7b60e857 [Google Chrome Framework] - RenderSVGShape.cpp:339 WebCore::RenderSVGShape::paint Argh, this is an entirely different issue to the Chrome crasher. So forget I ever mentioned it. It's worth pointing out that a <stop/> inside an SVG like this is illegal according to the SVG content model. Created attachment 209678 [details]
Patch
(In reply to comment #5) > Created an attachment (id=209678) [details] > Patch I think this fix is too local and may hide real security bugs in the future. RenderSVGGradientStop is one of the few SVG renderers to inherit from RenderObject instead of RenderBlock or RenderSVGBoxModelObject, and it's missing the overrides we have in those "normal" base classes. As an example, we don't appear to do the proper SVGResourcesCache::clientDestroyed call in willBeDestroyed(). Niko tried and failed to remove RenderSVGGradientStop. Reviving that fix is one solution; making RenderSVGGradientStop inherit from RenderSVGBoxModelObject is another. Comment on attachment 209678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209678&action=review > Source/WebCore/rendering/svg/RenderSVGGradientStop.h:50 > + virtual bool nodeAtFloatPoint(const HitTestRequest&, HitTestResult&, const FloatPoint&, HitTestAction) OVERRIDE FINAL { return false; } No need for FINAL on the function when it’s in a FINAL class. Seems fine to revert this and do other testing to find the “real fix” too, but seems no harm to doing this. (In reply to comment #8) > Seems fine to revert this and do other testing to find the “real fix” too, but seems no harm to doing this. I talked with Philip on IRC. As was noted this fixes the problem, though long-term the current situation is not ideal. Removing RenderSVGGradientStop sounds like the ideal solution, but it is AFAIK still blocked by problems with interaction with CSS transitions. We'll probably have to decide between removal of RenderSVGGradientStop or inheriting from RenderSVGBoxModelObject (adds some bloat though) soon, for now I'll commit this and close the bug so whatever we choose this problem is already fixed and we have the regression test to check that it remains fixed. (In reply to comment #6) > (In reply to comment #5) > > Created an attachment (id=209678) [details] [details] > > Patch > > I think this fix is too local and may hide real security bugs in the future. RenderSVGGradientStop is one of the few SVG renderers to inherit from RenderObject instead of RenderBlock or RenderSVGBoxModelObject, and it's missing the overrides we have in those "normal" base classes. As an example, we don't appear to do the proper SVGResourcesCache::clientDestroyed call in willBeDestroyed(). That may be the wrong example ; AFAIK stops are not added to SVGResourcesCache, so there is no need to call SVGResourcesCache::clientDestroyed for them. Committed r154644: <http://trac.webkit.org/changeset/154644> |