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.
Created attachment 243294 [details] Patch
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 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?
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.
(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?
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 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.
Created attachment 243355 [details] Implemented reviewer feedback
Comment on attachment 243355 [details] Implemented reviewer feedback Clearing flags on attachment: 243355 Committed r177441: <http://trac.webkit.org/changeset/177441>
All reviewed patches have been landed. Closing bug.
*** Bug 139592 has been marked as a duplicate of this bug. ***