Enable SVGImage to dispatch events in the destructor.

Enable SVGImage to dispatch events in the destructor.

The destructor of SVGImage can dispatch harmless events
that can be called in EventDispatchForbiddenScope scope.

BUG=378524
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188801

[reni, 2014-12-09 19:09:49]

rhodovan.u-szeged@partner.samsung.com changed reviewers:
+ esprehn@chromium.org

[pdr., 2014-12-09 19:47:25]

On 2014/12/09 at 19:09:49, rhodovan.u-szeged wrote:
> 

I am skeptical that these would be harmless. What events are we creating here?

[esprehn, 2014-12-18 06:57:30]

On 2014/12/09 at 19:47:25, pdr wrote:
> On 2014/12/09 at 19:09:49, rhodovan.u-szeged wrote:
> > 
> 
> I am skeptical that these would be harmless. What events are we creating here?

The bug shows that this is the unload event firing from the SVGImage's document,
against the SVGImage's window object that can't actually have listeners. I think
this is the right fix.

The code has changed since that bug was filed but the way we get there is:

Page::willBeDestroyed() calls mainFrame->detach(), LocalFrame::detach calls
m_loader.closeURL(), which calls m_frame->document()->dispatchUnloadEvents();

You could probably also change that function to:

if (m_frame->document() && !SVGImage::isInSVGImage(m_frame->document()))
  m_frame->document()->dispatchUnloadEvents();

https://codereview.chromium.org/787143002/diff/1/Source/core/svg/graphics/SVG...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/787143002/diff/1/Source/core/svg/graphics/SVG...
Source/core/svg/graphics/SVGImage.cpp:68: // to run so we can call it in
EventDispatchForbiddenScope scope.
I would be more specific and call out that unload events will fire.

[reni, 2014-12-18 18:39:32]

@esprehn, @pdr: thanks for the comments and reviews.

[reni, 2015-01-07 14:18:18]

Could someone take a look at this, please?

[esprehn, 2015-01-09 00:55:06]

https://codereview.chromium.org/787143002/diff/20001/Source/core/svg/graphics...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/787143002/diff/20001/Source/core/svg/graphics...
Source/core/svg/graphics/SVGImage.cpp:69:
EventDispatchForbiddenScope::AllowUserAgentEvents allowEvents;
Why do you still need this if you have the SVGImage::isInSVGImage check in
closeURL()?

[esprehn, 2015-01-21 09:48:28]

lgtm

[esprehn, 2015-01-21 09:48:31]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2015-01-21 09:48:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/787143002/40001

[commit-bot: I haz the power, 2015-01-21 11:45:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-21 11:45:53]

Failed to commit the patch.
svn: E155010: Commit failed (details follow):
svn: E155010:
'/b/infra_internal/commit_queue/workdir/blink/LayoutTests/platform/linux/fast/masking'
is scheduled for addition, but is missing

[reni, 2015-01-22 09:46:31]

The CQ bit was checked by rhodovan.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2015-01-22 09:46:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/787143002/40001

[commit-bot: I haz the power, 2015-01-22 09:47:11]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188801

[reni, 2015-01-22 09:47:29]

On 2015/01/21 09:48:28, esprehn wrote:
> lgtm

Thank you :)

[sof, 2015-01-22 15:06:48]

Thanks for fixing this, but doesn't it do the opposite of what the description
says? i.e., it doesn't dispatch unload events for SVGImages.