Move InstanceInvalidationGuard/UpdateBlocker to SVGElement from SVGElementInstance
Created attachment 245856 [details] Patch
Bot shows Windows failing but I don’t see a specific error. Maybe Brent can help?
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.
The patch built successfully without any changes against ToT. I'm not sure why the EWS complained about this file.
I am convinced that the Windows EWS has multiple false negatives. Still looking for a reviewer for this patch.
Comment on attachment 245856 [details] Patch This builds and runs tests on mac and windows, and seems like a good change. r=me.
Comment on attachment 245856 [details] Patch Clearing flags on attachment: 245856 Committed r179548: <http://trac.webkit.org/changeset/179548>
All reviewed patches have been landed. Closing bug.
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()
(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.
I guess we should roll this out so that darin can figure out what's going on?
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.
Re-opened since this is blocked by bug 141201
My guess is that we are somehow nesting image update blockers, even though the assertion darin added should catch that. Weird!
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.
(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.
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); }
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.
Maybe SVGElement::setInstanceUpdatesBlocked should call ensureSVGRareData so we never hit this case? Or perhaps call it in DEBUG mode so assertions stay correct?
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.
(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.
Committed r179695: <http://trac.webkit.org/changeset/179695>