[SVG] Defer recursive detachAnimatedProperty call to avoid touching invalidated iterator

[SVG] Defer recursive detachAnimatedProperty call to avoid touching invalidated iterator.

This is a workaround patch for detachAnimatedProperty recursively being called for SVGSVGElement. This patch postpones the SVGAnimatedProperty dtor being called until we finish touching the HashMap, so it wont touch invalidated HashMap iterator.

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

[kouhei (in TOK), 2014-01-14 03:10:37]

pdr: Would you take a look?

This is not a fundamental solution, but I couldn't find the root cause. The
function in question is going to be removed anyway, so workaround patch.

[pdr., 2014-01-14 04:33:24]

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

https://codereview.chromium.org/133273005/diff/20001/Source/core/svg/properti...
Source/core/svg/properties/SVGAnimatedProperty.cpp:59: // Workaround for
http://crbug.com/333156 :
Please remove the workaround comment, as I think we're actually fixing the bug
even though it's local.

https://codereview.chromium.org/133273005/diff/20001/Source/core/svg/properti...
Source/core/svg/properties/SVGAnimatedProperty.cpp:62:
Vector<RefPtr<SVGAnimatedProperty> > deferredPropertyDestruct;
I would also change this comment slightly to just say "There are cases where..."
because we certainly have such a case :)

https://codereview.chromium.org/133273005/diff/20001/Source/core/svg/properti...
Source/core/svg/properties/SVGAnimatedProperty.cpp:64:
deferredPropertyDestruct.append(cache->get(*it));
I'm okay with a temporary workaround but I think we need to fully understand the
crash or we risk hiding a larger issue. For example, SVGPolyElement has a custom
points attribute that may affect the new property implementation.

Can you explain the chain of events that's leading to this bug?

I see that we have a call to detachAnimatedPropertiesForElement from an
SVGPolyElement dtor, then a SVGSVGElement dtor, and then another call to
detachAnimatedPropertiesForElement.

Is the bug that we are destructing 'this' SVGAnimatedProperty from the nested
detachAnimatedPropertiesForElement?

Would the following work or is the Vector required?
for (...) {
    RefPtr<SVGAniamtedProperty> protect = cache->get(*it);
    cache->remove(*it);
}

[kouhei (in TOK), 2014-01-14 04:45:07]

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

https://codereview.chromium.org/133273005/diff/20001/Source/core/svg/properti...
Source/core/svg/properties/SVGAnimatedProperty.cpp:64:
deferredPropertyDestruct.append(cache->get(*it));
On 2014/01/14 04:33:25, pdr wrote:
> I'm okay with a temporary workaround but I think we need to fully understand
the
> crash or we risk hiding a larger issue. For example, SVGPolyElement has a
custom
> points attribute that may affect the new property implementation.
>
> Can you explain the chain of events that's leading to this bug?
> 
> I see that we have a call to detachAnimatedPropertiesForElement from an
> SVGPolyElement dtor, then a SVGSVGElement dtor, and then another call to
> detachAnimatedPropertiesForElement.
> 
> Is the bug that we are destructing 'this' SVGAnimatedProperty from the nested
> detachAnimatedPropertiesForElement?
No. The problem is that SVGAnimatedProperty is somehow holding a reference to
SVGSVGElement. Vector<RefPtr<FrameView> > appearing in the stacktrace indicates
layout() may be related to this, but I'm not sure.
- detachAnimatedPropertiesForElement for SVGPolyElement destructs the property
holding the reference
  - Note that this is inside HashMap::remove().
- ~SVGSVGElement is removeLastRef-ed
- ~SVGSVGElement also calls detachAnimatedPropertiesForElement for itself.
- detachAnimatedPropertiesForElement calls HashMap::remove(), but this inside
another HashMap::remove() being called at detachAnimatedPropertisForElement for
SVGPolyElement. This invalidates the iterator used inside the first remove().
 
> Would the following work or is the Vector required?
> for (...) {
>     RefPtr<SVGAniamtedProperty> protect = cache->get(*it);
>     cache->remove(*it);
> }
Yes. That is more clever.

[pdr., 2014-01-14 04:51:25]

On 2014/01/14 04:45:07, kouhei wrote:
>
https://codereview.chromium.org/133273005/diff/20001/Source/core/svg/properti...
> File Source/core/svg/properties/SVGAnimatedProperty.cpp (right):
> 
>
https://codereview.chromium.org/133273005/diff/20001/Source/core/svg/properti...
> Source/core/svg/properties/SVGAnimatedProperty.cpp:64:
> deferredPropertyDestruct.append(cache->get(*it));
> On 2014/01/14 04:33:25, pdr wrote:
> > I'm okay with a temporary workaround but I think we need to fully understand
> the
> > crash or we risk hiding a larger issue. For example, SVGPolyElement has a
> custom
> > points attribute that may affect the new property implementation.
> >
> > Can you explain the chain of events that's leading to this bug?
> > 
> > I see that we have a call to detachAnimatedPropertiesForElement from an
> > SVGPolyElement dtor, then a SVGSVGElement dtor, and then another call to
> > detachAnimatedPropertiesForElement.
> > 
> > Is the bug that we are destructing 'this' SVGAnimatedProperty from the
nested
> > detachAnimatedPropertiesForElement?
> No. The problem is that SVGAnimatedProperty is somehow holding a reference to
> SVGSVGElement. Vector<RefPtr<FrameView> > appearing in the stacktrace
indicates
> layout() may be related to this, but I'm not sure.
> - detachAnimatedPropertiesForElement for SVGPolyElement destructs the property
> holding the reference
>   - Note that this is inside HashMap::remove().
> - ~SVGSVGElement is removeLastRef-ed
> - ~SVGSVGElement also calls detachAnimatedPropertiesForElement for itself.
> - detachAnimatedPropertiesForElement calls HashMap::remove(), but this inside
> another HashMap::remove() being called at detachAnimatedPropertisForElement
for
> SVGPolyElement. This invalidates the iterator used inside the first remove().
>  
> > Would the following work or is the Vector required?
> > for (...) {
> >     RefPtr<SVGAniamtedProperty> protect = cache->get(*it);
> >     cache->remove(*it);
> > }
> Yes. That is more clever.

Ah, thank you. I understand now.

LGTM with the more local protect refptr :)

[kouhei (in TOK), 2014-01-14 04:51:26]

pdr: PTAL

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

https://codereview.chromium.org/133273005/diff/20001/Source/core/svg/properti...
Source/core/svg/properties/SVGAnimatedProperty.cpp:59: // Workaround for
http://crbug.com/333156 :
On 2014/01/14 04:33:25, pdr wrote:
> Please remove the workaround comment, as I think we're actually fixing the bug
> even though it's local.

Done.

https://codereview.chromium.org/133273005/diff/20001/Source/core/svg/properti...
Source/core/svg/properties/SVGAnimatedProperty.cpp:62:
Vector<RefPtr<SVGAnimatedProperty> > deferredPropertyDestruct;
On 2014/01/14 04:33:25, pdr wrote:
> I would also change this comment slightly to just say "There are cases
where..."
> because we certainly have such a case :)

Done.

https://codereview.chromium.org/133273005/diff/20001/Source/core/svg/properti...
Source/core/svg/properties/SVGAnimatedProperty.cpp:64:
deferredPropertyDestruct.append(cache->get(*it));
On 2014/01/14 04:45:07, kouhei wrote:
> On 2014/01/14 04:33:25, pdr wrote:
> > I'm okay with a temporary workaround but I think we need to fully understand
> the
> > crash or we risk hiding a larger issue. For example, SVGPolyElement has a
> custom
> > points attribute that may affect the new property implementation.
> >
> > Can you explain the chain of events that's leading to this bug?
> > 
> > I see that we have a call to detachAnimatedPropertiesForElement from an
> > SVGPolyElement dtor, then a SVGSVGElement dtor, and then another call to
> > detachAnimatedPropertiesForElement.
> > 
> > Is the bug that we are destructing 'this' SVGAnimatedProperty from the
nested
> > detachAnimatedPropertiesForElement?
> No. The problem is that SVGAnimatedProperty is somehow holding a reference to
> SVGSVGElement. Vector<RefPtr<FrameView> > appearing in the stacktrace
indicates
> layout() may be related to this, but I'm not sure.
> - detachAnimatedPropertiesForElement for SVGPolyElement destructs the property
> holding the reference
>   - Note that this is inside HashMap::remove().
> - ~SVGSVGElement is removeLastRef-ed
> - ~SVGSVGElement also calls detachAnimatedPropertiesForElement for itself.
> - detachAnimatedPropertiesForElement calls HashMap::remove(), but this inside
> another HashMap::remove() being called at detachAnimatedPropertisForElement
for
> SVGPolyElement. This invalidates the iterator used inside the first remove().
>  
> > Would the following work or is the Vector required?
> > for (...) {
> >     RefPtr<SVGAniamtedProperty> protect = cache->get(*it);
> >     cache->remove(*it);
> > }
> Yes. That is more clever.

Done.

[kouhei (in TOK), 2014-01-14 04:51:54]

Thanks for review!

[commit-bot: I haz the power, 2014-01-14 04:58:53]

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

[commit-bot: I haz the power, 2014-01-14 08:57:05]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2014-01-14 12:59:56]

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

[commit-bot: I haz the power, 2014-01-14 15:49:32]

Change committed as 165070