Bug 141148 – Move InstanceInvalidationGuard/UpdateBlocker to SVGElement from SVGElementInstance

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Darin Adler]

Move InstanceInvalidationGuard/UpdateBlocker to SVGElement from SVGElementInstance

[Darin Adler]

Created attachment 245856 [details]
Patch

[Darin Adler]

Bot shows Windows failing but I don’t see a specific error. Maybe Brent can help?

[Brent Fulgham]

Yes, it looks like something in TestWebKitAPI is failing. Unfortunately, the MSVC output is so bloated with minutia that we don't see the relevant error in the 500 characters of output captured in the EWS log.

I'll apply the patch locally and see what the actual error is.

[Brent Fulgham]

The patch built successfully without any changes against ToT. I'm not sure why the EWS complained about this file.

[Darin Adler]

I am convinced that the Windows EWS has multiple false negatives.

Still looking for a reviewer for this patch.

[Brent Fulgham]

Comment on attachment 245856 [details]
Patch

This builds and runs tests on mac and windows, and seems like a good change. r=me.

[WebKit Commit Bot]

Comment on attachment 245856 [details]
Patch

Clearing flags on attachment: 245856

Committed r179548: <http://trac.webkit.org/changeset/179548>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This caused many assertions: https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20(Tests)/builds/10255

ASSERTION FAILED: m_element.instanceUpdatesBlocked()
/Volumes/Data/slave/mavericks-debug/build/Source/WebCore/svg/SVGElement.h(265) : WebCore::SVGElement::InstanceUpdateBlocker::~InstanceUpdateBlocker()
1   0x105430df0 WTFCrash
2   0x10aca752b WebCore::SVGElement::InstanceUpdateBlocker::~InstanceUpdateBlocker()
3   0x10aca42f5 WebCore::SVGElement::InstanceUpdateBlocker::~InstanceUpdateBlocker()
4   0x10acbc92e WebCore::applyCSSPropertyToTargetAndInstances(WebCore::SVGElement&, WebCore::QualifiedName const&, WTF::String const&)
5   0x10acbc76b WebCore::SVGAnimateElementBase::applyResultsToTarget()
6   0x10ab59b38 WebCore::SMILTimeContainer::updateAnimations(WebCore::SMILTime, bool)
7   0x10ab595ff WebCore::SMILTimeContainer::begin()
8   0x10acde113 WebCore::SVGDocumentExtensions::startAnimations()
9   0x1095152d8 WebCore::Document::implicitClose()
10  0x109876f3b WebCore::FrameLoader::checkCallImplicitClose()
11  0x109876bf4 WebCore::FrameLoader::checkCompleted()
12  0x1098756e8 WebCore::FrameLoader::finishedParsing()
13  0x109521851 WebCore::Document::finishedParsing()
14  0x10b02e12d WebCore::XMLDocumentParser::end()

[Brent Fulgham]

(In reply to comment #9)
> This caused many assertions:
> https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20(Tests)/
> builds/10255
> 
> ASSERTION FAILED: m_element.instanceUpdatesBlocked()
> /Volumes/Data/slave/mavericks-debug/build/Source/WebCore/svg/SVGElement.
> h(265) : WebCore::SVGElement::InstanceUpdateBlocker::~InstanceUpdateBlocker()
> 1   0x105430df0 WTFCrash
> 2   0x10aca752b

Idea: We should probably have EWS run a debug build so we can see if we've caused new assertions to be fired.

[Brent Fulgham]

I guess we should roll this out so that darin can figure out what's going on?

[Brent Fulgham]

The assertion being fired is new to this patch:

    ASSERT(m_element.instanceUpdatesBlocked()); 

Previously, the SVGInstanceElement would simply call 'm_element.setInstanceUpdatesBlocked(false)' without worrying about whether the element was in an 'updates blocked' state. When this responsibility was moved to SVGInstance, the new assertion was added causing this issue.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 141201

[Brent Fulgham]

My guess is that we are somehow nesting image update blockers, even though the assertion darin added should catch that. Weird!

[Brent Fulgham]

These assertions seem to be coming about because the InstanceUpdateBlocker is failing to mark the element as "update blocked". I added some additional assertions:

    ASSERT(!targetElement.instanceUpdatesBlocked());
    SVGElement::InstanceUpdateBlocker blocker(targetElement);
    ASSERT(targetElement.instanceUpdatesBlocked());

The first assertion is always valid, but when we hit the errors shown by the test system, the second assertion is getting triggered.

Somehow, the InstanceUpdateBlocker is NOT changing the state of the target element.

[Brent Fulgham]

(In reply to comment #15)
> These assertions seem to be coming about because the InstanceUpdateBlocker
> is failing to mark the element as "update blocked". I added some additional
> assertions:
> 
>     ASSERT(!targetElement.instanceUpdatesBlocked());
>     SVGElement::InstanceUpdateBlocker blocker(targetElement);
>     ASSERT(targetElement.instanceUpdatesBlocked());
> 
> The first assertion is always valid, but when we hit the errors shown by the
> test system, the second assertion is getting triggered.
> 
> Somehow, the InstanceUpdateBlocker is NOT changing the state of the target
> element.

FYI: This is in 'applyCSSPropertyToTargetAndInstances' in the SVGAnimateElementBase.cpp class.

[Brent Fulgham]

Ok. I think I see the problem. Apparently some SVGElements do not have m_svgRareData. When this is the case, calling 'setInstanceUpdatesBlocked' is a no-op, but later attempts to confirm that we are in the right state fail.

I think this change might be the right thing to do:

Index: svg/SVGElement.h
===================================================================
--- svg/SVGElement.h	(revision 179554)
+++ svg/SVGElement.h	(working copy)
@@ -258,11 +258,12 @@
     : m_element(element)
 {
     m_element.setInstanceUpdatesBlocked(true);
+    ASSERT(!m_element.m_svgRareData || m_element.instanceUpdatesBlocked());
 }
 
 inline SVGElement::InstanceUpdateBlocker::~InstanceUpdateBlocker()
 {
-    ASSERT(m_element.instanceUpdatesBlocked());
+    ASSERT(!m_element.m_svgRareData || m_element.instanceUpdatesBlocked());
     m_element.setInstanceUpdatesBlocked(false);
 }

[Brent Fulgham]

No. That's not complete.

We have some cases (in the debugger I'm looking at an SVGCircleElement) where the element had no rare data when it entered the scope of the InstanceUpdateBlocker. Later, it called applyCSSPropertyToTarget, which calls "ensureAnimatedSMILStyleProperties", which in turn calls ensureSVGRareData().

At this point, we now have a rare data without its m_instancesUpdatesBlocked member set properly.

Consequently, the assertion fails.

[Brent Fulgham]

Maybe SVGElement::setInstanceUpdatesBlocked should call ensureSVGRareData so we never hit this case?

Or perhaps call it in DEBUG mode so assertions stay correct?

[Darin Adler]

The assertion in ~InstanceUpdateBlocker not critical. I added it to detect a potential mistake, but we can just omit it. The right thing to do here is to land the patch without that assertion.

[Brent Fulgham]

(In reply to comment #20)
> The assertion in ~InstanceUpdateBlocker not critical. I added it to detect a
> potential mistake, but we can just omit it. The right thing to do here is to
> land the patch without that assertion.

Ok. I'm running Debug tests locally with this patch (without the assertion) and will land it shortly if everything goes well.

[Brent Fulgham]

Committed r179695: <http://trac.webkit.org/changeset/179695>