[svg] TearOffs dynamically hold on to the current contextElement().

[svg] TearOffs dynamically hold on to the current contextElement().

Prior to this patch, TearOffs held on to contextElement() resolved at creation time. This was on based on an assumption that contextElement() never change. However, this assumption is wrong, as animatedProperty and its contextElement() can be dynamically assigned if an existing (detached) TearOff is made an item of SVG*ListProperty afterwards.

This patch fixes the issue by dynamically holding on to the current contextElement() by using SetReference inside resolveWrapperReachability callback.

BUG=317097
BUG=318577
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161752

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

[kouhei (in TOK), 2013-11-11 06:48:58]

haraken, pdr: Would you take a look? Thanks.

[kouhei (in TOK), 2013-11-11 06:49:28]

https://codereview.chromium.org/64683006/diff/1/LayoutTests/svg/dom/SVGTransf...
File LayoutTests/svg/dom/SVGTransformTearOff-contextElement-crash-expected.txt
(right):

https://codereview.chromium.org/64683006/diff/1/LayoutTests/svg/dom/SVGTransf...
LayoutTests/svg/dom/SVGTransformTearOff-contextElement-crash-expected.txt:1:
CONSOLE MESSAGE: line 15: 0
Sorry I'll remove this

[kouhei (in TOK), 2013-11-11 06:55:56]

https://codereview.chromium.org/64683006/diff/1/LayoutTests/svg/dom/SVGTransf...
File LayoutTests/svg/dom/SVGTransformTearOff-contextElement-crash-expected.txt
(right):

https://codereview.chromium.org/64683006/diff/1/LayoutTests/svg/dom/SVGTransf...
LayoutTests/svg/dom/SVGTransformTearOff-contextElement-crash-expected.txt:1:
CONSOLE MESSAGE: line 15: 0
On 2013/11/11 06:49:28, kouhei wrote:
> Sorry I'll remove this

Done.

[haraken, 2013-11-11 07:47:06]

Given this is a PO bug, let me stamp LGTM.

I'd be happy if pdr takes a close look later.

[commit-bot: I haz the power, 2013-11-11 07:51:24]

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

[kouhei (in TOK), 2013-11-11 07:51:33]

On 2013/11/11 07:47:06, haraken wrote:
> Given this is a PO bug, let me stamp LGTM.
> 
> I'd be happy if pdr takes a close look later.

Thanks for quick review!

[commit-bot: I haz the power, 2013-11-11 08:57:24]

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

[commit-bot: I haz the power, 2013-11-11 11:00:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-11 11:20:49]

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

[commit-bot: I haz the power, 2013-11-11 11:42:50]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-11 13:08:10]

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

[commit-bot: I haz the power, 2013-11-11 13:29:20]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-11 15:21:45]

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

[commit-bot: I haz the power, 2013-11-11 15:47:23]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-11 17:45:53]

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

[commit-bot: I haz the power, 2013-11-11 18:08:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-12 01:48:02]

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

[commit-bot: I haz the power, 2013-11-12 04:02:48]

Change committed as 161752

[kouhei (in TOK), 2013-11-13 05:54:11]

haraken: PTAL. added gc hack as we discussed in chat.

[haraken, 2013-11-13 06:08:59]

https://codereview.chromium.org/64683006/diff/520001/Source/core/svg/properti...
File Source/core/svg/properties/SVGAnimatedProperty.cpp (right):

https://codereview.chromium.org/64683006/diff/520001/Source/core/svg/properti...
Source/core/svg/properties/SVGAnimatedProperty.cpp:35:
contextElement->setContextElement();

Isn't there any chance where we want to unset a context element?

[kouhei (in TOK), 2013-11-13 06:11:29]

On 2013/11/13 06:08:59, haraken wrote:
>
https://codereview.chromium.org/64683006/diff/520001/Source/core/svg/properti...
> File Source/core/svg/properties/SVGAnimatedProperty.cpp (right):
> 
>
https://codereview.chromium.org/64683006/diff/520001/Source/core/svg/properti...
> Source/core/svg/properties/SVGAnimatedProperty.cpp:35:
> contextElement->setContextElement();
> 
> Isn't there any chance where we want to unset a context element?

contextElement is only cleared at dtor.

[haraken, 2013-11-13 07:03:51]

On 2013/11/13 06:11:29, kouhei wrote:
> On 2013/11/13 06:08:59, haraken wrote:
> >
>
https://codereview.chromium.org/64683006/diff/520001/Source/core/svg/properti...
> > File Source/core/svg/properties/SVGAnimatedProperty.cpp (right):
> > 
> >
>
https://codereview.chromium.org/64683006/diff/520001/Source/core/svg/properti...
> > Source/core/svg/properties/SVGAnimatedProperty.cpp:35:
> > contextElement->setContextElement();
> > 
> > Isn't there any chance where we want to unset a context element?
> 
> contextElement is only cleared at dtor.

ok. LGTM.

I agree that this fix is ugly, but this is the best short-term hack we were able
to come up with in our offline discussion. In long-term, we will clean up the GC
interaction using Oilpan.

[commit-bot: I haz the power, 2013-11-13 07:06:24]

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

[commit-bot: I haz the power, 2013-11-13 09:10:05]

Change committed as 161895