Bug 139644 – The SVGDocument of an SVGImage should not perform any additional actions when the SVGImage is being destroyed

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Radu Stavila]

In SVGImage::~SVGImage, when detaching the frame, the SVGDocument of the image performs additional actions (such as dispatching events and performing style recalc) even though it is being destroyed (the document is created by the image and destroyed together with it). This is not only useless, but can cause additional problems.

[Radu Stavila]

Created attachment 243294 [details]
Patch

[Antti Koivisto]

Comment on attachment 243294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243294&action=review

> Source/WebCore/dom/Document.h:534
> +    inline bool isBeingDestroyed() const { return m_isBeingDestroyed; }
> +    inline void setIsBeingDestroyed() { m_isBeingDestroyed = true; }

This sounds very generic yet it is only ever set for SVG images.

Can we just have isSVGImage() or similar? Other places that check for that seem to use frame()->page()->chrome().client().isSVGImageChromeClient().

[Radu Stavila]

Comment on attachment 243294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243294&action=review

>> Source/WebCore/dom/Document.h:534
>> +    inline void setIsBeingDestroyed() { m_isBeingDestroyed = true; }
> 
> This sounds very generic yet it is only ever set for SVG images.
> 
> Can we just have isSVGImage() or similar? Other places that check for that seem to use frame()->page()->chrome().client().isSVGImageChromeClient().

I can do that but since the actions that are taken when the flag is set are not SVG-specific but rather specific for any document being destroyed, isn't this more consistent?

[Radu Stavila]

Another thing would be that I would like to only affect the particular scenario of the SVGImage being destroyed, I don't want to change it for all SVGImages, in any scenario.

[Antti Koivisto]

(In reply to comment #4)
> Another thing would be that I would like to only affect the particular
> scenario of the SVGImage being destroyed, I don't want to change it for all
> SVGImages, in any scenario.

What are those scenarios and why don't you want to change them?

[Radu Stavila]

Because I don't know all the cases when closeURL might be called on SVGDocuments attached to SVGImages and I feel keeping this restricted to only the problem at hand is the safest way to avoid unforeseen efects.

[Simon Fraser (smfr)]

Comment on attachment 243294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243294&action=review

>>> Source/WebCore/dom/Document.h:534
>>> +    inline void setIsBeingDestroyed() { m_isBeingDestroyed = true; }
>> 
>> This sounds very generic yet it is only ever set for SVG images.
>> 
>> Can we just have isSVGImage() or similar? Other places that check for that seem to use frame()->page()->chrome().client().isSVGImageChromeClient().
> 
> I can do that but since the actions that are taken when the flag is set are not SVG-specific but rather specific for any document being destroyed, isn't this more consistent?

I don't like having both this and RenderObject::documentBeingDestroyed() (which calls Document::renderTreeBeingDestroyed()) which means something different.

Either they should be true at the same time, or they need names that distinguish them.

[Radu Stavila]

Created attachment 243355 [details]
Implemented reviewer feedback

[WebKit Commit Bot]

Comment on attachment 243355 [details]
Implemented reviewer feedback

Clearing flags on attachment: 243355

Committed r177441: <http://trac.webkit.org/changeset/177441>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radu Stavila]

*** Bug 139592 has been marked as a duplicate of this bug. ***