Bug 139644 - The SVGDocument of an SVGImage should not perform any additional actions when the SVGImage is being destroyed
Summary: The SVGDocument of an SVGImage should not perform any additional actions when...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Radu Stavila
URL:
Keywords: AdobeTracked
: 139592 (view as bug list)
Depends on:
Blocks: 139294 139592
  Show dependency treegraph
 
Reported: 2014-12-15 05:24 PST by Radu Stavila
Modified: 2014-12-18 03:57 PST (History)
13 users (show)

See Also:


Attachments
Patch (4.13 KB, patch)
2014-12-15 07:04 PST, Radu Stavila
simon.fraser: review-
Details | Formatted Diff | Diff
Implemented reviewer feedback (2.52 KB, patch)
2014-12-16 02:13 PST, Radu Stavila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Radu Stavila 2014-12-15 05:24:54 PST
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.
Comment 1 Radu Stavila 2014-12-15 07:04:27 PST
Created attachment 243294 [details]
Patch
Comment 2 Antti Koivisto 2014-12-15 07:19:28 PST
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().
Comment 3 Radu Stavila 2014-12-15 07:22:02 PST
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?
Comment 4 Radu Stavila 2014-12-15 07:47:27 PST
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.
Comment 5 Antti Koivisto 2014-12-15 08:02:39 PST
(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?
Comment 6 Radu Stavila 2014-12-15 08:06:12 PST
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.
Comment 7 Simon Fraser (smfr) 2014-12-15 13:46:10 PST
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.
Comment 8 Radu Stavila 2014-12-16 02:13:13 PST
Created attachment 243355 [details]
Implemented reviewer feedback
Comment 9 WebKit Commit Bot 2014-12-17 02:13:47 PST
Comment on attachment 243355 [details]
Implemented reviewer feedback

Clearing flags on attachment: 243355

Committed r177441: <http://trac.webkit.org/changeset/177441>
Comment 10 WebKit Commit Bot 2014-12-17 02:13:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radu Stavila 2014-12-18 03:57:05 PST
*** Bug 139592 has been marked as a duplicate of this bug. ***