SVG: Moving animating <svg> to other iframe should not crash.

SVG: Moving animating <svg> to other iframe should not crash.

Moving SVGSVGElement with its SMILTimeContainer already started caused crash before this patch.
|SVGDocumentExtentions::startAnimations()| calls begin() against all SMILTimeContainers in the document, but the SMILTimeContainer for <svg> moved from other document may be already started.

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

[kouhei (in TOK), 2014-05-19 03:48:24]

Would you take a look?

[pdr., 2014-05-19 04:17:17]

On 2014/05/19 03:48:24, kouhei wrote:
> Would you take a look?

LGTM, but can you please change the assert in SMILTimeContainer::begin() to be a
release assert?

ASSERT_WITH_SECURITY_IMPLICATION(!m_beginTime);

[kouhei (in TOK), 2014-05-19 04:20:49]

On 2014/05/19 04:17:17, pdr wrote:
> On 2014/05/19 03:48:24, kouhei wrote:
> > Would you take a look?
> 
> LGTM, but can you please change the assert in SMILTimeContainer::begin() to be
a
> release assert?
> 
> ASSERT_WITH_SECURITY_IMPLICATION(!m_beginTime);

I'm happy to change it to release assert, but can it be RELEASE_ASSERT()? I
don't see any possible security issue if the assert is violated.

[pdr., 2014-05-19 04:27:28]

On 2014/05/19 04:20:49, kouhei wrote:
> On 2014/05/19 04:17:17, pdr wrote:
> > On 2014/05/19 03:48:24, kouhei wrote:
> > > Would you take a look?
> > 
> > LGTM, but can you please change the assert in SMILTimeContainer::begin() to
be
> a
> > release assert?
> > 
> > ASSERT_WITH_SECURITY_IMPLICATION(!m_beginTime);
> 
> I'm happy to change it to release assert, but can it be RELEASE_ASSERT()? I
> don't see any possible security issue if the assert is violated.

SGTM. Why is the bug marked as security?


I have one additional concern/question though... In the testcase we have one
document (A) which calls begin on a time container with a different document
(B). Is there any danger here if the documents are from different origins?

[kouhei (in TOK), 2014-05-19 04:48:02]

On 2014/05/19 04:27:28, pdr wrote:
> On 2014/05/19 04:20:49, kouhei wrote:
> > On 2014/05/19 04:17:17, pdr wrote:
> > > On 2014/05/19 03:48:24, kouhei wrote:
> > > > Would you take a look?
> > > 
> > > LGTM, but can you please change the assert in SMILTimeContainer::begin()
to
> be
> > a
> > > release assert?
> > > 
> > > ASSERT_WITH_SECURITY_IMPLICATION(!m_beginTime);
> > 
> > I'm happy to change it to release assert, but can it be RELEASE_ASSERT()? I
> > don't see any possible security issue if the assert is violated.
> 
> SGTM. Why is the bug marked as security?
Sorry the security bug is not directly related to this patch, but the repro case
on the bug fails many asserts so I'm fixing it one by one.

> I have one additional concern/question though... In the testcase we have one
> document (A) which calls begin on a time container with a different document
> (B). Is there any danger here if the documents are from different origins?

I don't think it would be a problem. If the documents are from different
origins, we can't reference the document to move anyway.

[kouhei (in TOK), 2014-05-20 01:43:23]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-05-20 01:43:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/290353002/20001

[commit-bot: I haz the power, 2014-05-20 02:13:11]

Change committed as 174338