Bug 212163 – ASSERTION FAILED: (!s_current || &m_view != &s_current->m_view) in RenderTreeBuilder::RenderTreeBuilder

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jack]

<rdar://57028096>

0   com.apple.JavaScriptCore      	0x000000010b167d3e WTFCrash + 14
1   com.apple.WebCore             	0x000000011ada8d8b WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x000000011eb396df WebCore::RenderTreeBuilder::RenderTreeBuilder(WebCore::RenderView&) + 431
3   com.apple.WebCore             	0x000000011eb39a5d WebCore::RenderTreeBuilder::RenderTreeBuilder(WebCore::RenderView&) + 29
4   com.apple.WebCore             	0x000000011eb5734c WebCore::RenderTreeUpdater::RenderTreeUpdater(WebCore::Document&) + 124
5   com.apple.WebCore             	0x000000011eb5741d WebCore::RenderTreeUpdater::RenderTreeUpdater(WebCore::Document&) + 29
6   com.apple.WebCore             	0x000000011d42142e WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 1262
7   com.apple.WebCore             	0x000000011e018622 WebCore::Frame::setPageAndTextZoomFactors(float, float) + 482
8   com.apple.WebKitLegacy        	0x00000001307d325a -[WebView _setZoomMultiplier:isTextOnly:] + 202
9   com.apple.WebKitLegacy        	0x000000013075e784 +[WebFrame(WebInternal) _createFrameWithPage:frameName:frameView:ownerElement:] + 900
10  com.apple.WebKitLegacy        	0x000000013075eb7b +[WebFrame(WebInternal) _createSubframeWithOwnerElement:frameName:frameView:] + 171
11  com.apple.WebKitLegacy        	0x000000013074bf90 WebFrameLoaderClient::createFrame(WTF::URL const&, WTF::String const&, WebCore::HTMLFrameOwnerElement&, WTF::String const&) + 320
12  com.apple.WebCore             	0x000000011deb4078 WebCore::SubframeLoader::loadSubframe(WebCore::HTMLFrameOwnerElement&, WTF::URL const&, WTF::String const&, WTF::String const&) + 440
13  com.apple.WebCore             	0x000000011deb2983 WebCore::SubframeLoader::loadOrRedirectSubframe(WebCore::HTMLFrameOwnerElement&, WTF::URL const&, WTF::AtomString const&, WebCore::LockHistory, WebCore::LockBackForwardList) + 483
14  com.apple.WebCore             	0x000000011deb3416 WebCore::SubframeLoader::requestObject(WebCore::HTMLPlugInImageElement&, WTF::String const&, WTF::AtomString const&, WTF::String const&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&) + 566
15  com.apple.WebCore             	0x000000011d935480 WebCore::HTMLPlugInImageElement::requestObject(WTF::String const&, WTF::String const&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&) + 416
16  com.apple.WebCore             	0x000000011d921606 WebCore::HTMLObjectElement::updateWidget(WebCore::CreatePlugins) + 694
17  com.apple.WebCore             	0x000000011d931daf WebCore::HTMLPlugInImageElement::updateAfterStyleResolution() + 335
18  com.apple.WebCore             	0x000000011d93dd3d WebCore::HTMLPlugInImageElement::scheduleUpdateForAfterStyleResolution()::$_0::operator()() const + 29
19  com.apple.WebCore             	0x000000011d93daae WTF::Detail::CallableWrapper<WebCore::HTMLPlugInImageElement::scheduleUpdateForAfterStyleResolution()::$_0, void>::call() + 30
20  com.apple.WebCore             	0x000000011adb63e2 WTF::Function<void ()>::operator()() const + 130
21  com.apple.WebCore             	0x000000011eb948f2 WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler() + 114
22  com.apple.WebCore             	0x000000011eb94ac5 WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler() + 21
23  com.apple.WebCore             	0x000000011eb57759 WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 601
24  com.apple.WebCore             	0x000000011d421c4c WebCore::Document::updateTextRenderer(WebCore::Text&, unsigned int, unsigned int) + 332
25  com.apple.WebCore             	0x000000011d640903 WebCore::Text::updateRendererAfterContentChange(unsigned int, unsigned int) + 179
26  com.apple.WebCore             	0x000000011d3ad0f5 WebCore::CharacterData::setDataAndUpdate(WTF::String const&, unsigned int, unsigned int, unsigned int) + 261
27  com.apple.WebCore             	0x000000011d3acf0c WebCore::CharacterData::setData(WTF::String const&) + 332
28  com.apple.WebCore             	0x000000011d3adcdb WebCore::CharacterData::setNodeValue(WTF::String const&) + 43
29  com.apple.WebCore             	0x000000011d58b24c WebCore::Node::setTextContent(WTF::String const&) + 172
30  com.apple.WebCore             	0x000000011ee24017 WebCore::SVGTRefElement::updateReferencedText(WebCore::Element*) + 503
31  com.apple.WebCore             	0x000000011ee23dca WebCore::SVGTRefTargetEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) + 170
32  com.apple.WebCore             	0x000000011d51fa0d WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul>, WebCore::EventTarget::EventInvokePhase) + 925
33  com.apple.WebCore             	0x000000011d51bca4 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) + 356
34  com.apple.WebCore             	0x000000011d58ffd2 WebCore::Node::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) + 178
35  com.apple.WebCore             	0x000000011d4f5fb1 WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const + 193
36  com.apple.WebCore             	0x000000011d4f6aaf WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&) + 383
37  com.apple.WebCore             	0x000000011d4f65b7 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) + 567
38  com.apple.WebCore             	0x000000011d59002d WebCore::Node::dispatchEvent(WebCore::Event&) + 29
39  com.apple.WebCore             	0x000000011d5d8e9e WebCore::ScopedEventQueue::dispatchEvent(WebCore::Event&) const + 46
40  com.apple.WebCore             	0x000000011d5d8e21 WebCore::ScopedEventQueue::enqueueEvent(WTF::Ref<WebCore::Event, WTF::DumbPtrTraits<WebCore::Event> >&&) + 193
41  com.apple.WebCore             	0x000000011d4f6289 WebCore::EventDispatcher::dispatchScopedEvent(WebCore::Node&, WebCore::Event&) + 105
42  com.apple.WebCore             	0x000000011d58fffd WebCore::Node::dispatchScopedEvent(WebCore::Event&) + 29
43  com.apple.WebCore             	0x000000011d59018b WebCore::Node::dispatchSubtreeModifiedEvent() + 331
44  com.apple.WebCore             	0x000000011d4ea90f WebCore::Element::didAddAttribute(WebCore::QualifiedName const&, WTF::AtomString const&) + 175
45  com.apple.WebCore             	0x000000011d4ea803 WebCore::Element::addAttributeInternal(WebCore::QualifiedName const&, WTF::AtomString const&, WebCore::Element::SynchronizationOfLazyAttribute) + 195
46  com.apple.WebCore             	0x000000011d4e3711 WebCore::Element::setAttributeInternal(unsigned int, WebCore::QualifiedName const&, WTF::AtomString const&, WebCore::Element::SynchronizationOfLazyAttribute) + 129
47  com.apple.WebCore             	0x000000011d4e39ce WebCore::Element::setAttribute(WTF::AtomString const&, WTF::AtomString const&) + 398
48  com.apple.WebCore             	0x000000011b6b6237 WebCore::jsElementPrototypeFunctionSetAttributeBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSElement*, JSC::ThrowScope&) + 535
49  com.apple.WebCore             	0x000000011b631a12 long long WebCore::IDLOperation<WebCore::JSElement>::call<&(WebCore::jsElementPrototypeFunctionSetAttributeBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSElement*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) + 786
50  com.apple.WebCore             	0x000000011b6316f4 WebCore::jsElementPrototypeFunctionSetAttribute(JSC::JSGlobalObject*, JSC::CallFrame*) + 36
51  ???                           	0x0000433d02a0116b 0 + 73929316110699

[Jack]

oot cause: RenderTreeBuilder expects one instance for each render view. However, in the test case, we instantiate two RenderTreeBuilder with the same render view due to reentrancy.
1. We instantiate RenderTreeBuilder in function Document::updateTextRenderer while processing setAttribute command (frame #35).
2. Right after step 1, RenderTreeUpdater::commit is called to update the render tree (frame #34).
3. In render tree updating, HTMLPlugInImageElement::scheduleUpdateForAfterStyleResolution is called which eventually invokes Document::resolveStyle.
4. Document::resolveStyle instantiate the second RenderTreeUpdater with the same render view (frame #4).

<script>
function run() {
obj = document.createElement("object");
li.appendChild(obj);
svg.currentScale = 0.99;
obj.data = String.fromCharCode(89, 82)
ff.setAttribute("direction", "rtl");
}
</script>
<body onload=run()><li id=li><svg id=svg><font-face-uri id=ff><tref xlink:href=#ff>


  * frame #0: 0x000000012531b5f4 WebCore`WebCore::RenderTreeBuilder::RenderTreeBuilder(this=0x00007ffee64045c0, view=0x000000013db9d210) at RenderTreeBuilder.cpp:154:5
    frame #1: 0x000000012531b9dd WebCore`WebCore::RenderTreeBuilder::RenderTreeBuilder(this=0x00007ffee64045c0, view=0x000000013db9d210) at RenderTreeBuilder.cpp:153:1
    frame #2: 0x00000001253337dc WebCore`WebCore::RenderTreeUpdater::RenderTreeUpdater(this=0x00007ffee6404598, document={ origin = file://, url = about:blank, inMainFrame = 0, backForwardCacheState = NotInBackForwardCache }) at RenderTreeUpdater.cpp:84:7
    frame #3: 0x00000001253338ad WebCore`WebCore::RenderTreeUpdater::RenderTreeUpdater(this=0x00007ffee6404598, document={ origin = file://, url = about:blank, inMainFrame = 0, backForwardCacheState = NotInBackForwardCache }) at RenderTreeUpdater.cpp:85:1
    frame #4: 0x0000000123acc18e WebCore`WebCore::Document::resolveStyle(this={ origin = file://, url = about:blank, inMainFrame = 0, backForwardCacheState = NotInBackForwardCache }, type=Rebuild) at Document.cpp:2008:31
    frame #5: 0x0000000123accc30 WebCore`WebCore::Document::updateStyleIfNeeded(this={ origin = file://, url = about:blank, inMainFrame = 0, backForwardCacheState = NotInBackForwardCache }) at Document.cpp:2101:5
    frame #6: 0x0000000123ae697a WebCore`WebCore::Document::finishedParsing(this={ origin = file://, url = about:blank, inMainFrame = 0, backForwardCacheState = NotInBackForwardCache }) at Document.cpp:5861:9
    frame #7: 0x00000001241aadf8 WebCore`WebCore::HTMLConstructionSite::finishedParsing(this=0x000000013d5784c0) at HTMLConstructionSite.cpp:419:16
    frame #8: 0x00000001241f7657 WebCore`WebCore::HTMLTreeBuilder::finished(this=0x000000013d5784a0) at HTMLTreeBuilder.cpp:2843:12
    frame #9: 0x00000001241b2258 WebCore`WebCore::HTMLDocumentParser::end(this=0x000000013dfb9000) at HTMLDocumentParser.cpp:449:20
    frame #10: 0x00000001241b00d8 WebCore`WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd(this=0x000000013dfb9000) at HTMLDocumentParser.cpp:458:5
    frame #11: 0x00000001241afe07 WebCore`WebCore::HTMLDocumentParser::prepareToStopParsing(this=0x000000013dfb9000) at HTMLDocumentParser.cpp:153:5
    frame #12: 0x00000001241b22c2 WebCore`WebCore::HTMLDocumentParser::attemptToEnd(this=0x000000013dfb9000) at HTMLDocumentParser.cpp:470:5
    frame #13: 0x00000001241b2399 WebCore`WebCore::HTMLDocumentParser::finish(this=0x000000013dfb9000) at HTMLDocumentParser.cpp:498:5
    frame #14: 0x0000000124518be2 WebCore`WebCore::DocumentWriter::end(this=0x000000013dfc8d90) at DocumentWriter.cpp:288:15
    frame #15: 0x0000000124517c34 WebCore`WebCore::DocumentLoader::finishedLoading(this=0x000000013dfc8d00) at DocumentLoader.cpp:452:14
    frame #16: 0x0000000124523244 WebCore`WebCore::DocumentLoader::maybeLoadEmpty(this=0x000000013dfc8d00) at DocumentLoader.cpp:1799:5
    frame #17: 0x00000001245233d5 WebCore`WebCore::DocumentLoader::startLoadingMainResource(this=0x000000013dfc8d00) at DocumentLoader.cpp:1813:9
    frame #18: 0x00000001245757c6 WebCore`WebCore::FrameLoader::init(this=0x000000013d59c680) at FrameLoader.cpp:344:34
    frame #19: 0x0000000124763fe6 WebCore`WebCore::Frame::init(this={ origin = file://, url = about:blank, isMainFrame = 0, backForwardCacheState = NotInBackForwardCache }) at Frame.cpp:184:15
    frame #20: 0x0000000138f203fc WebKitLegacy`+[WebFrame(self=WebFrame, _cmd="_createFrameWithPage:frameName:frameView:ownerElement:", page=0x000000013dcfa000, name={ length = 0, contents = '' }, frameView=0x00007f9a1c587c70, ownerElement=0x000000013db67720) _createFrameWithPage:frameName:frameView:ownerElement:] at WebFrame.mm:318:21
    frame #21: 0x0000000138f2094b WebKitLegacy`+[WebFrame(self=WebFrame, _cmd="_createSubframeWithOwnerElement:frameName:frameView:", ownerElement=0x000000013db67720, name={ length = 0, contents = '' }, frameView=0x00007f9a1c587c70) _createSubframeWithOwnerElement:frameName:frameView:] at WebFrame.mm:342:12
    frame #22: 0x0000000138f0ce98 WebKitLegacy`WebFrameLoaderClient::createFrame(this=0x000000013d5e4f80, name={ length = 0, contents = '' }, ownerElement=0x000000013db67720) at WebFrameLoaderClient.mm:1625:37
    frame #23: 0x00000001245f1337 WebCore`WebCore::FrameLoader::SubframeLoader::loadSubframe(this=0x000000013d5f8a80, ownerElement=0x000000013db67720, url={ file:///Users/jacklee/browser2/57028099/YR }, name={ length = 0, contents = '' }, referrer={ length = 57, contents = 'file:///Users/jacklee/browser2/57028099/min-57028099.html' }) at SubframeLoader.cpp:337:44
    frame #24: 0x00000001245efc25 WebCore`WebCore::FrameLoader::SubframeLoader::loadOrRedirectSubframe(this=0x000000013d5f8a80, ownerElement=0x000000013db67720, requestURL={ file:///Users/jacklee/browser2/57028099/YR }, frameName={ length = 0, contents = '' }, lockHistory=Yes, lockBackForwardList=Yes) at SubframeLoader.cpp:309:17
    frame #25: 0x00000001245f06d6 WebCore`WebCore::FrameLoader::SubframeLoader::requestObject(this=0x000000013d5f8a80, ownerElement=0x000000013db67720, url={ length = 2, contents = 'YR' }, frameName={ length = 0, contents = '' }, mimeType={ length = 0, contents = '' }, paramNames={ size = 2, capacity = 16 }, paramValues={ size = 2, capacity = 16 }) at SubframeLoader.cpp:245:12
    frame #26: 0x0000000124026da0 WebCore`WebCore::HTMLPlugInImageElement::requestObject(this=0x000000013db67720, relativeURL={ length = 2, contents = 'YR' }, mimeType={ length = 0, contents = '' }, paramNames={ size = 2, capacity = 16 }, paramValues={ size = 2, capacity = 16 }) at HTMLPlugInImageElement.cpp:811:58
    frame #27: 0x000000012400d106 WebCore`WebCore::HTMLObjectElement::updateWidget(this=0x000000013db67720, createPlugins=No) at HTMLObjectElement.cpp:289:19
    frame #28: 0x00000001240236bf WebCore`WebCore::HTMLPlugInImageElement::updateAfterStyleResolution(this=0x000000013db67720) at HTMLPlugInImageElement.cpp:298:17
    frame #29: 0x000000012403550d WebCore`WebCore::HTMLPlugInImageElement::scheduleUpdateForAfterStyleResolution(this=0x000000013f4ee8a8)::$_0::operator()() const at HTMLPlugInImageElement.cpp:276:24
    frame #30: 0x000000012403527e WebCore`WTF::Detail::CallableWrapper<WebCore::HTMLPlugInImageElement::scheduleUpdateForAfterStyleResolution()::$_0, void>::call(this=0x000000013f4ee8a0) at Function.h:52:39
    frame #31: 0x00000001212757c2 WebCore`WTF::Function<void ()>::operator(this=0x000000013d577000)() const at Function.h:84:35
    frame #32: 0x00000001253d23a2 WebCore`WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler(this=0x00007ffee6405ed8) at StyleTreeResolver.cpp:643:17
    frame #33: 0x00000001253d2575 WebCore`WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler(this=0x00007ffee6405ed8) at StyleTreeResolver.cpp:637:1
    frame #34: 0x0000000125333bbc WebCore`WebCore::RenderTreeUpdater::commit(this=0x00007ffee6405f28, styleUpdate=unique_ptr<const WebCore::Style::Update, std::__1::default_delete<const WebCore::Style::Update> > @ 0x00007ffee6405f20) at RenderTreeUpdater.cpp:136:1
    frame #35: 0x0000000123acc90c WebCore`WebCore::Document::updateTextRenderer(this={ origin = file://, url = file:///Users/jacklee/browser2/57028099/min-57028099.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }, text=0x000000013db67220, offsetOfReplacedText=0, lengthOfReplacedText=0) at Document.cpp:2051:5
    frame #36: 0x0000000123cff1e3 WebCore`WebCore::Text::updateRendererAfterContentChange(this=0x000000013db67220, offsetOfReplacedData=0, lengthOfReplacedData=0) at Text.cpp:221:16
    frame #37: 0x0000000123a5a055 WebCore`WebCore::CharacterData::setDataAndUpdate(this=0x000000013db67220, newData={ length = 1, contents = '
' }, offsetOfReplacedData=0, oldLength=0, newLength=1) at CharacterData.cpp:198:31
    frame #38: 0x0000000123a59e6c WebCore`WebCore::CharacterData::setData(this=0x000000013db67220, data={ length = 1, contents = '
' }) at CharacterData.cpp:65:5
    frame #39: 0x0000000123a5ac3b WebCore`WebCore::CharacterData::setNodeValue(this=0x000000013db67220, nodeValue={ length = 1, contents = '
' }) at CharacterData.cpp:187:5
    frame #40: 0x0000000123c4a8cc WebCore`WebCore::Node::setTextContent(this=0x000000013db67220, text={ length = 1, contents = '
' }) at Node.cpp:1575:16
    frame #41: 0x000000012568b4a7 WebCore`WebCore::SVGTRefElement::updateReferencedText(this=0x000000013db66f80, target=0x000000013db66ea0) at SVGTRefElement.cpp:150:29
    frame #42: 0x000000012568b25a WebCore`WebCore::SVGTRefTargetEventListener::handleEvent(this=0x000000013d51ba50, (null)={ origin = file://, url = file:///Users/jacklee/browser2/57028099/min-57028099.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }, event=0x000000013db67920) at SVGTRefElement.cpp:119:23
    frame #43: 0x0000000123bdd097 WebCore`WebCore::EventTarget::innerInvokeEventListeners(this=0x000000013db66ea0, event=0x000000013db67920, listeners={ size = 1, capacity = 1 }, phase=Bubbling) at EventTarget.cpp:335:40
    frame #44: 0x0000000123bd9370 WebCore`WebCore::EventTarget::fireEventListeners(this=0x000000013db66ea0, event=0x000000013db67920, phase=Bubbling) at EventTarget.cpp:267:9
    frame #45: 0x0000000123c4f6d2 WebCore`WebCore::Node::handleLocalEvents(this=0x000000013db66ea0, event=0x000000013db67920, phase=Bubbling) at Node.cpp:2371:5
    frame #46: 0x0000000123bc7ab1 WebCore`WebCore::EventContext::handleLocalEvents(this=0x000000013f4cca78, event=0x000000013db67920, phase=Bubbling) const at EventContext.cpp:55:17
    frame #47: 0x0000000123bc857f WebCore`WebCore::dispatchEventInDOM(event=0x000000013db67920, path=0x00007ffee64065a8) at EventDispatcher.cpp:100:22
    frame #48: 0x0000000123bc80b7 WebCore`WebCore::EventDispatcher::dispatchEvent(node=0x000000013db66ea0, event=0x000000013db67920) at EventDispatcher.cpp:154:9
    frame #49: 0x0000000123c4f72d WebCore`WebCore::Node::dispatchEvent(this=0x000000013db66ea0, event=0x000000013db67920) at Node.cpp:2381:5
    frame #50: 0x0000000123c9854e WebCore`WebCore::ScopedEventQueue::dispatchEvent(this=0x0000000127c709e8, event=0x000000013db67920) const at ScopedEventQueue.cpp:57:37
    frame #51: 0x0000000123c984d1 WebCore`WebCore::ScopedEventQueue::enqueueEvent(this=0x0000000127c709e8, event=0x00007ffee6406780) at ScopedEventQueue.cpp:52:9
    frame #52: 0x0000000123bc7d89 WebCore`WebCore::EventDispatcher::dispatchScopedEvent(node=0x000000013db66ea0, event=0x000000013db67920) at EventDispatcher.cpp:52:35
    frame #53: 0x0000000123c4f6fd WebCore`WebCore::Node::dispatchScopedEvent(this=0x000000013db66ea0, event=0x000000013db67920) at Node.cpp:2376:5
    frame #54: 0x0000000123c4f88b WebCore`WebCore::Node::dispatchSubtreeModifiedEvent(this=0x000000013db66ea0) at Node.cpp:2397:5
    frame #55: 0x0000000123b8dccf WebCore`WebCore::Element::didAddAttribute(this=0x000000013db66ea0, name=0x00007ffee64069b8, value={ length = 3, contents = 'rtl' }) at Element.cpp:4100:5
    frame #56: 0x0000000123b8dbc0 WebCore`WebCore::Element::addAttributeInternal(this=0x000000013db66ea0, name=0x00007ffee64069b8, value={ length = 3, contents = 'rtl' }, inSynchronizationOfLazyAttribute=NotInSynchronizationOfLazyAttribute) at Element.cpp:2874:5
    frame #57: 0x0000000123b86975 WebCore`WebCore::Element::setAttributeInternal(this=0x000000013db66ea0, index=4294967295, name=0x00007ffee64069b8, newValue={ length = 3, contents = 'rtl' }, inSynchronizationOfLazyAttribute=NotInSynchronizationOfLazyAttribute) at Element.cpp:1711:9
    frame #58: 0x0000000123b86c1e WebCore`WebCore::Element::setAttribute(this=0x000000013db66ea0, qualifiedName={ length = 9, contents = 'direction' }, value={ length = 3, contents = 'rtl' }) at Element.cpp:1678:5
    frame #59: 0x0000000121cbcb10 WebCore`WebCore::jsElementPrototypeFunctionSetAttributeBody(lexicalGlobalObject=0x000000013dff8468, callFrame=0x00007ffee6406c30, castedThis=0x000000013d547b08, throwScope=0x00007ffee6406ba8) at JSElement.cpp:3795:63
    frame #60: 0x0000000121c30412 WebCore`long long WebCore::IDLOperation<WebCore::JSElement>::call<&(lexicalGlobalObject=0x000000013dff8468, callFrame=0x00007ffee6406c30, operationName="setAttribute")), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) at JSDOMOperation.h:53:16
    frame #61: 0x0000000121c300f4 WebCore`WebCore::jsElementPrototypeFunctionSetAttribute(lexicalGlobalObject=0x000000013dff8468, callFrame=0x00007ffee6406c30) at JSElement.cpp:3801:12
    frame #62: 0x00005a7589601178

[Jack]

This bug probably has security implications since if this assertion is disabled we would hit the following assertion in FrameViewLayoutContext::layout():

    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!frame().document()->inRenderTreeUpdate());

On an old webkit version that does not have either assertion, would it be a concern?

[Jack]

Created attachment 399885 [details]
Patch

[Jack]

Created attachment 399887 [details]
Patch

[Jack]

The patches are for discussions and demonstrating the issue.

To avoid reentrancy, added check for inRenderTreeUpdate() and bail out. In the first patch the whole Document::resolveStyle. In second and third patches we still run part of Document::resolveStyle.

[Jack]

Created attachment 399888 [details]
Patch

[Geoffrey Garen]

I don't completely agree that this is re-entrency. We only invoke this nested plug-in code at the very end of RenderTreeUpdater::commit. RenderTreeUpdater::commit's PostResolutionCallbackDisabler is designed to delay this work until we it will not re-enter.

Is this a release crash or a debug assertion failure?

Can you explain what about re-entrency caused a bug here?

Does moving the PostResolutionCallbackDisabler into the callers of RenderTreeUpdater::commit fix the bug?

[Jack]

Sorry I should have made it more clear. It was referring to RenderTreeBuilder constructor. The design is non-reentrant since it checks for existing RenderTreeBuilder and crashes.

And thanks for the suggestion. Moving PostResolutionCallbackDisabler out of commit works. And there are only two callers so it should be easy to make the change.

I am uploading a quick hack to see if it passes EWS. If it does, I will separate the function as it relies on destructors to clear m_inRenderTreeUpdate in document and s_current in RenderTreeBuilder.  

(In reply to Geoffrey Garen from comment #7)
> I don't completely agree that this is re-entrency. We only invoke this
> nested plug-in code at the very end of RenderTreeUpdater::commit.
> RenderTreeUpdater::commit's PostResolutionCallbackDisabler is designed to
> delay this work until we it will not re-enter.
> 
> Is this a release crash or a debug assertion failure?
> 
> Can you explain what about re-entrency caused a bug here?
> 
> Does moving the PostResolutionCallbackDisabler into the callers of
> RenderTreeUpdater::commit fix the bug?

[Jack]

Created attachment 399973 [details]
Patch

[Jack]

This is a release crash in RenderTreeBuilder constructor:

RELEASE_ASSERT(!s_current || &m_view != &s_current->m_view);

(In reply to Geoffrey Garen from comment #7)
> Is this a release crash or a debug assertion failure?

[Geoffrey Garen]

Comment on attachment 399973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399973&action=review

> Source/WebCore/dom/Document.cpp:2011
> +            SetForScope<bool>* inRenderTreeUpdate = new SetForScope<bool>(m_inRenderTreeUpdate, true);
> +            RenderTreeUpdater* updater = new RenderTreeUpdater(*this);
> +            updater->commit(WTFMove(styleUpdate));
> +            delete updater;
> +            delete inRenderTreeUpdate;
> +            Style::PostResolutionCallbackDisabler callbackDisabler(*this);

This is the right idea but the wrong approach. We try very hard to avoid manual delete in WebKit. Also, you declared the PostResolutionCallbackDisabler too late, so it has no effect anymore during the call to commit().

Here's a suggestion:

1. Change the  RenderTreeUpdater constructor to require a const PostResolutionCallbackDisabler& argument. (The purpose of this argument is to communicate, at compile time, the requirement that our caller must have this object in scope already. Since we won't actually use this argument, you don't need to give it a parameter name, just a type.)

2. Make it build.

This change uses C++ scope rules to naturally enforce the idiom that the PostResolutionCallbackDisabler must run its destructor after the RenderTreeUpdater runs its destructor.

[Jack]

Thanks for the explanation. Now I have a better idea how PostResolutionCallbackDisabler works.

Yeah, it was a quick hack. I planed to create a function in document to call updater->commit so it doesn't require manual delete. And it can be easily added if there will be more callers.

(In reply to Geoffrey Garen from comment #11)
> Comment on attachment 399973 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399973&action=review
> 
> > Source/WebCore/dom/Document.cpp:2011
> > +            SetForScope<bool>* inRenderTreeUpdate = new SetForScope<bool>(m_inRenderTreeUpdate, true);
> > +            RenderTreeUpdater* updater = new RenderTreeUpdater(*this);
> > +            updater->commit(WTFMove(styleUpdate));
> > +            delete updater;
> > +            delete inRenderTreeUpdate;
> > +            Style::PostResolutionCallbackDisabler callbackDisabler(*this);
> 
> This is the right idea but the wrong approach. We try very hard to avoid
> manual delete in WebKit. Also, you declared the
> PostResolutionCallbackDisabler too late, so it has no effect anymore during
> the call to commit().
> 
> Here's a suggestion:
> 
> 1. Change the  RenderTreeUpdater constructor to require a const
> PostResolutionCallbackDisabler& argument. (The purpose of this argument is
> to communicate, at compile time, the requirement that our caller must have
> this object in scope already. Since we won't actually use this argument, you
> don't need to give it a parameter name, just a type.)
> 
> 2. Make it build.
> 
> This change uses C++ scope rules to naturally enforce the idiom that the
> PostResolutionCallbackDisabler must run its destructor after the
> RenderTreeUpdater runs its destructor.

[Jack]

Geoff, in the last patch I also experiment with:
    SetForScope<bool> inRenderTreeUpdate(m_inRenderTreeUpdate, true);
    delete inRenderTreeUpdate;"

If inRenderTreeUpdate is not destructed before ~PostResolutionCallbackDisabler(), the following RELEASE_ASSERT_WITH_SECURITY_IMPLICATION triggers in DumpRenderTree.

void FrameViewLayoutContext::layout()
{
    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!frame().document()->inRenderTreeUpdate());
...
}

  * frame #0: 0x0000000111ca1133 WebCore`WTFCrashWithInfo((null)=191, (null)="./page/FrameViewLayoutContext.cpp", (null)="void WebCore::FrameViewLayoutContext::layout()", (null)=28) at Assertions.h:671:5 [opt]
    frame #1: 0x000000011312d57e WebCore`WebCore::FrameViewLayoutContext::layout(this=<unavailable>) at FrameViewLayoutContext.cpp:191:5 [opt]
    frame #2: 0x00000001131146ea WebCore`WebCore::Frame::setPageAndTextZoomFactors(this={ origin = file://, url = file:///Users/jacklee/browser/min-57028099.html, isMainFrame = 1, backForwardCacheState = NotInBackForwardCache }, pageZoomFactor=<unavailable>, textZoomFactor=<unavailable>) at Frame.cpp:965:35 [opt]
    frame #3: 0x000000011636ae4f WebKitLegacy`+[WebFrame(self=<unavailable>, _cmd=<unavailable>, page=<unavailable>, name=<unavailable>, frameView=<unavailable>, ownerElement=0x0000000119dd6ec0) _createFrameWithPage:frameName:frameView:ownerElement:] at WebFrame.mm:320:5 [opt]
    frame #4: 0x000000011636b0b7 WebKitLegacy`+[WebFrame(self=<unavailable>, _cmd=<unavailable>, ownerElement=<unavailable>, name=<unavailable>, frameView=<unavailable>) _createSubframeWithOwnerElement:frameName:frameView:] at WebFrame.mm:342:12 [opt]
    frame #5: 0x0000000116364ba0 WebKitLegacy`WebFrameLoaderClient::createFrame(this=0x0000000119ee5e60, name={ length = 0, contents = '' }, ownerElement=0x0000000119dd6ec0) at WebFrameLoaderClient.mm:1625:37 [opt]
    frame #6: 0x000000011306d5b4 WebCore`WebCore::FrameLoader::SubframeLoader::loadSubframe(this=0x0000000119ef9230, ownerElement=0x0000000119dd6ec0, url={ file:///Users/jacklee/browser/YR }, name={ length = 0, contents = '' }, referrer={ length = 47, contents = 'file:///Users/jacklee/browser/min-57028099.html' }) at SubframeLoader.cpp:337:44 [opt]
    frame #7: 0x000000011306c218 WebCore`WebCore::FrameLoader::SubframeLoader::loadOrRedirectSubframe(this=0x0000000119ef9230, ownerElement=0x0000000119dd6ec0, requestURL=<unavailable>, frameName={ length = 0, contents = '' }, lockHistory=<unavailable>, lockBackForwardList=Yes) at SubframeLoader.cpp:309:17 [opt]
    frame #8: 0x000000011306c9c0 WebCore`WebCore::FrameLoader::SubframeLoader::requestObject(this=0x0000000119ef9230, ownerElement=0x0000000119dd6ec0, url={ length = 2, contents = 'YR' }, frameName=<unavailable>, mimeType={ length = 0, contents = '' }, paramNames=<unavailable>, paramValues={ size = 2, capacity = 16 }) at SubframeLoader.cpp:245:12 [opt]
    frame #9: 0x0000000112e3951b WebCore`WebCore::HTMLPlugInImageElement::requestObject(this=0x0000000119dd6ec0, relativeURL={ length = 2, contents = 'YR' }, mimeType={ length = 0, contents = '' }, paramNames={ size = 2, capacity = 16 }, paramValues={ size = 2, capacity = 16 }) at HTMLPlugInImageElement.cpp:811:58 [opt]
    frame #10: 0x0000000112e2b7e2 WebCore`WebCore::HTMLObjectElement::updateWidget(this=0x0000000119dd6ec0, createPlugins=No) at HTMLObjectElement.cpp:289:19 [opt]
    frame #11: 0x0000000112e36c5d WebCore`WebCore::HTMLPlugInImageElement::updateAfterStyleResolution(this=0x0000000119dd6ec0) at HTMLPlugInImageElement.cpp:298:17 [opt]
    frame #12: 0x0000000113637971 WebCore`WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler() [inlined] WTF::Function<void ()>::operator(this=<unavailable>)() const at Function.h:84:35 [opt]
    frame #13: 0x0000000113637967 WebCore`WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler(this=<unavailable>) at StyleTreeResolver.cpp:643 [opt]
    frame #14: 0x0000000112bcff41 WebCore`WebCore::Document::updateTextRenderer(WebCore::Text&, unsigned int, unsigned int) [inlined] WebCore::updateRenderTree(document={ origin = file://, url = file:///Users/jacklee/browser/min-57028099.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }, styleUpdate=unique_ptr<const WebCore::Style::Update, std::__1::default_delete<const WebCore::Style::Update> > @ scalar) at Document.cpp:1931:1 [opt]
    frame #15: 0x0000000112bcfec4 WebCore`WebCore::Document::updateTextRenderer(this={ origin = file://, url = file:///Users/jacklee/browser/min-57028099.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }, text=<unavailable>, offsetOfReplacedText=<unavailable>, lengthOfReplacedText=<unavailable>) at Document.cpp:2054 [opt]

[Jack]

Created attachment 400006 [details]
Patch

[Jack]

In this patch, the order of the following local variables need to be preserved so the destructors are called in the right order to prevent issues mentioned in comment #13.

    Style::PostResolutionCallbackDisabler callbackDisabler(document);
    SetForScope<bool> inRenderTreeUpdate(document.getInRenderTreeUpdate(), true);

Is there a better way to preserve the order?

(In reply to Jack from comment #14)
> Created attachment 400006 [details]
> Patch

[Geoffrey Garen]

(In reply to Jack from comment #15)
> In this patch, the order of the following local variables need to be
> preserved so the destructors are called in the right order to prevent issues
> mentioned in comment #13.
> 
>     Style::PostResolutionCallbackDisabler callbackDisabler(document);
>     SetForScope<bool> inRenderTreeUpdate(document.getInRenderTreeUpdate(),
> true);
> 
> Is there a better way to preserve the order?

For better or worse, in C++, construction order is the best way. 

At least by putting this in a helper function, you've ensured that all callers will do it right.

[Geoffrey Garen]

Comment on attachment 400006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400006&action=review

r=me, with one style change

> Source/WebCore/dom/Document.h:1318
> +    bool& getInRenderTreeUpdate() { return m_inRenderTreeUpdate; }

I think it would be slightly nicer to remove this accessor and make updateRenderTree a Document member function instead. It's best to avoid making pointers to data members public because doing so makes it harder to reason about the places that can change those data members.

[Jack]

Created attachment 400027 [details]
Patch for landing

[EWS]

Patch 400027 does not build

[Jack]

Created attachment 400059 [details]
Patch

[Geoffrey Garen]

Comment on attachment 400059 [details]
Patch

r=me

[Jack]

Changed to member function and added regression test.
 
(In reply to Jack from comment #20)
> Created attachment 400059 [details]
> Patch

[Jack]

Created attachment 400067 [details]
Patch for landing

[Jack]

No sure why EWS cannot find "StyleUpdate.h", somehow it builds on local machine.

Include "StyleResolver.h" instead.

(In reply to Jack from comment #23)
> Created attachment 400067 [details]
> Patch for landing

[EWS]

Patch 400067 does not build

[Jack]

Created attachment 400107 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 400107 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Jack]

Created attachment 400108 [details]
Patch for landing

[EWS]

Committed r262095: <https://trac.webkit.org/changeset/262095>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400108 [details].

[Jack]

Reopening to attach new patch.

[Jack]

Created attachment 400111 [details]
Patch

[Jack]

Win EWS didn't finish. Run tests again to make sure.
(In reply to Jack from comment #31)
> Created attachment 400111 [details]
> Patch

[Jack]

Created attachment 400112 [details]
Patch

[Jack]

EWS fails the same test on earlier changes, for example:
https://ews-build.webkit.org/#/builders/10/builds/18819

(In reply to Jack from comment #33)
> Created attachment 400112 [details]
> Patch

[Antti Koivisto]

Comment on attachment 400108 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=400108&action=review

> Source/WebCore/dom/Document.cpp:1932
> +    // NOTE: Preserve the order of definitions below so the destructors are called in proper sequence.
> +    Style::PostResolutionCallbackDisabler callbackDisabler(*this);
> +    SetForScope<bool> inRenderTreeUpdate(m_inRenderTreeUpdate, true);
> +    RenderTreeUpdater updater(*this, callbackDisabler);
> +    // End of ordered definitions
> +
> +    updater.commit(WTFMove(styleUpdate));

Instead of comments, maybe make the scoping clear by adding a { } block after PostResolutionCallbackDisabler?

[Jack]

Thanks, Antti. Good idea!
(In reply to Antti Koivisto from comment #35)
> Comment on attachment 400108 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400108&action=review
> 
> > Source/WebCore/dom/Document.cpp:1932
> > +    // NOTE: Preserve the order of definitions below so the destructors are called in proper sequence.
> > +    Style::PostResolutionCallbackDisabler callbackDisabler(*this);
> > +    SetForScope<bool> inRenderTreeUpdate(m_inRenderTreeUpdate, true);
> > +    RenderTreeUpdater updater(*this, callbackDisabler);
> > +    // End of ordered definitions
> > +
> > +    updater.commit(WTFMove(styleUpdate));
> 
> Instead of comments, maybe make the scoping clear by adding a { } block
> after PostResolutionCallbackDisabler?

[Jack]

Created attachment 400128 [details]
Patch

[EWS]

Committed r262103: <https://trac.webkit.org/changeset/262103>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400128 [details].