Bug 139644

Summary: The SVGDocument of an SVGImage should not perform any additional actions when the SVGImage is being destroyed
Product: WebKit Reporter: Radu Stavila <stavila>
Component: CSSAssignee: Radu Stavila <stavila>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, d-r, esprehn+autocc, fmalita, gyuyoung.kim, japhet, kangil.han, koivisto, mihnea, pdr, schenney, sergio, simon.fraser
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 139294, 139592    
Attachments:
Description Flags
Patch
simon.fraser: review-
Implemented reviewer feedback none

Radu Stavila
Reported 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.
Attachments
Patch (4.13 KB, patch)
2014-12-15 07:04 PST, Radu Stavila 		simon.fraser: review-
DetailsFormatted DiffDiff
Implemented reviewer feedback (2.52 KB, patch)
2014-12-16 02:13 PST, Radu Stavila 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radu Stavila
Comment 1 2014-12-15 07:04:27 PST
Created attachment 243294 [details] Patch
Antti Koivisto
Comment 2 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().
Radu Stavila
Comment 3 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?
Radu Stavila
Comment 4 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.
Antti Koivisto
Comment 5 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?
Radu Stavila
Comment 6 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.
Simon Fraser (smfr)
Comment 7 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.
Radu Stavila
Comment 8 2014-12-16 02:13:13 PST
Created attachment 243355 [details] Implemented reviewer feedback
WebKit Commit Bot
Comment 9 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>
WebKit Commit Bot
Comment 10 2014-12-17 02:13:57 PST
All reviewed patches have been landed. Closing bug.
Radu Stavila
Comment 11 2014-12-18 03:57:05 PST
*** Bug 139592 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.