Prevent destruction of self in SMILTimeContainer with 'discard'

Prevent destruction of self in SMILTimeContainer with 'discard'

If discarding the <svg> root an animation update could end up destroying
the SVGSVGElement owning the SMILTimeContainer, leading to use-after-free.
Make sure to keep an additional reference to the owning SVGSVGElement
across affected callsites.

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

[kouhei (in TOK), 2014-03-12 00:30:29]

https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
Source/core/svg/animation/SMILTimeContainer.cpp:337: DiscardScope
discardScope(m_ownerSVGElement);
Can we put this inside updateAnimationsAndScheduleFrameIfNeeded?

[fs, 2014-03-12 07:32:13]

https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
Source/core/svg/animation/SMILTimeContainer.cpp:337: DiscardScope
discardScope(m_ownerSVGElement);
On 2014/03/12 00:30:30, kouhei wrote:
> Can we put this inside updateAnimationsAndScheduleFrameIfNeeded?

No (unfortunately), because on the next line following it here is an access to
|this|.

[pdr., 2014-03-13 07:07:23]

On 2014/03/12 07:32:13, fs wrote:
>
https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
> File Source/core/svg/animation/SMILTimeContainer.cpp (right):
> 
>
https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
> Source/core/svg/animation/SMILTimeContainer.cpp:337: DiscardScope
> discardScope(m_ownerSVGElement);
> On 2014/03/12 00:30:30, kouhei wrote:
> > Can we put this inside updateAnimationsAndScheduleFrameIfNeeded?
> 
> No (unfortunately), because on the next line following it here is an access to
> |this|.

Kouhei is now an expert in this area and may have some ideas (esp. with how
Oilpan might help)


I'm a little worried about protecting the callers to these functions as well.
For example,
SVGImage::startAnimation->SVGSVGElement::setCurrentTime->SMILTimeContainer::setElapsed.
Do we need to move the DiscardScope up any further?

I'm also worried about accidentally regressing this in a future refactor of
SMILTimeContainer. Could the DiscardScope be renamed AnimationUpdater (or
similar) and have that protector object be the one to actually call
updateAnimationsAndScheduleFrameIfNeeded? This way, it's not possible to call
updateAnimationsAndScheduleFrameIfNeeded without creating this protector object.

[kouhei (in TOK), 2014-03-13 07:30:07]

On 2014/03/13 07:07:23, pdr wrote:
> On 2014/03/12 07:32:13, fs wrote:
> >
>
https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
> > File Source/core/svg/animation/SMILTimeContainer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
> > Source/core/svg/animation/SMILTimeContainer.cpp:337: DiscardScope
> > discardScope(m_ownerSVGElement);
> > On 2014/03/12 00:30:30, kouhei wrote:
> > > Can we put this inside updateAnimationsAndScheduleFrameIfNeeded?
> > 
> > No (unfortunately), because on the next line following it here is an access
to
> > |this|.
> 
> Kouhei is now an expert in this area and may have some ideas (esp. with how
> Oilpan might help)

Actually oilpan would keep any object referenced from the current stack alive,
so we can get rid of all protect scope in future.

[fs, 2014-03-13 10:06:43]

On 2014/03/13 07:07:23, pdr wrote:
> On 2014/03/12 07:32:13, fs wrote:
> >
>
https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
> > File Source/core/svg/animation/SMILTimeContainer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
> > Source/core/svg/animation/SMILTimeContainer.cpp:337: DiscardScope
> > discardScope(m_ownerSVGElement);
> > On 2014/03/12 00:30:30, kouhei wrote:
> > > Can we put this inside updateAnimationsAndScheduleFrameIfNeeded?
> > 
> > No (unfortunately), because on the next line following it here is an access
to
> > |this|.
> 
> Kouhei is now an expert in this area and may have some ideas (esp. with how
> Oilpan might help)

Yeah (as kouhei already said), this is another situation where Oilpan would
help...

> I'm a little worried about protecting the callers to these functions as well.
> For example,
>
SVGImage::startAnimation->SVGSVGElement::setCurrentTime->SMILTimeContainer::setElapsed.
> Do we need to move the DiscardScope up any further?
> 
> I'm also worried about accidentally regressing this in a future refactor of
> SMILTimeContainer. Could the DiscardScope be renamed AnimationUpdater (or
> similar) and have that protector object be the one to actually call
> updateAnimationsAndScheduleFrameIfNeeded? This way, it's not possible to call
> updateAnimationsAndScheduleFrameIfNeeded without creating this protector
object.

I'm not particularly fond of this either, and it's just reasonably non-intrusive
fix for the (quickly) addressing the issue. It could certainly make sense to let
a helper object handle this (and other things like wake-up scheduling etc.).
That would probably require some fairly non-trivial refactoring (which had half
planned already, so I'll try to throw this in the mix too...).

[pdr., 2014-03-13 18:42:50]

On 2014/03/13 10:06:43, fs wrote:
> On 2014/03/13 07:07:23, pdr wrote:
> > On 2014/03/12 07:32:13, fs wrote:
> > >
> >
>
https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
> > > File Source/core/svg/animation/SMILTimeContainer.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/195233003/diff/1/Source/core/svg/animation/SM...
> > > Source/core/svg/animation/SMILTimeContainer.cpp:337: DiscardScope
> > > discardScope(m_ownerSVGElement);
> > > On 2014/03/12 00:30:30, kouhei wrote:
> > > > Can we put this inside updateAnimationsAndScheduleFrameIfNeeded?
> > > 
> > > No (unfortunately), because on the next line following it here is an
access
> to
> > > |this|.
> > 
> > Kouhei is now an expert in this area and may have some ideas (esp. with how
> > Oilpan might help)
> 
> Yeah (as kouhei already said), this is another situation where Oilpan would
> help...
> 
> > I'm a little worried about protecting the callers to these functions as
well.
> > For example,
> >
>
SVGImage::startAnimation->SVGSVGElement::setCurrentTime->SMILTimeContainer::setElapsed.
> > Do we need to move the DiscardScope up any further?
> > 
> > I'm also worried about accidentally regressing this in a future refactor of
> > SMILTimeContainer. Could the DiscardScope be renamed AnimationUpdater (or
> > similar) and have that protector object be the one to actually call
> > updateAnimationsAndScheduleFrameIfNeeded? This way, it's not possible to
call
> > updateAnimationsAndScheduleFrameIfNeeded without creating this protector
> object.
> 
> I'm not particularly fond of this either, and it's just reasonably
non-intrusive
> fix for the (quickly) addressing the issue. It could certainly make sense to
let
> a helper object handle this (and other things like wake-up scheduling etc.).
> That would probably require some fairly non-trivial refactoring (which had
half
> planned already, so I'll try to throw this in the mix too...).

That sounds reasonable to me. Would you like to land this as-is? If so, LGTM.

[kouhei (in TOK), 2014-03-14 01:20:09]

lgtm

[fs, 2014-03-14 11:38:39]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-03-14 11:38:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/195233003/1

[commit-bot: I haz the power, 2014-03-14 11:38:59]

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

[commit-bot: I haz the power, 2014-03-14 11:39:00]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[fs, 2014-03-14 12:09:21]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-03-14 12:09:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/195233003/1

[commit-bot: I haz the power, 2014-03-14 12:09:52]

Change committed as 169239