Bug 181513 – RenderTreeUpdater::current() returns null_ptr when mutation is done through Document::resolveStyle.

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

[alan]

which is incorrect but will deal with that later.
rdar://problem/36367085

[alan]

<rdar://problem/36367085>

[alan]

Created attachment 331013 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 331013 [details]
Patch

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

> Source/WebCore/style/StyleTreeResolver.cpp:587
> +        if (!RenderTreeBuilder::current())

It's really confusing that you check RenderTreeBuilder global state then assign to a member variable. Does std::make_unique<RenderTreeBuilder>()have side effects?

[alan]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 331013 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331013&action=review
> 
> > Source/WebCore/style/StyleTreeResolver.cpp:587
> > +        if (!RenderTreeBuilder::current())
> 
> It's really confusing that you check RenderTreeBuilder global state then
> assign to a member variable. Does std::make_unique<RenderTreeBuilder>()have
> side effects?
It ensures that the subsequent RenderTreeBuilder::current() calls return a valid object within the current scope. RenderTreeBuilder c'tor sets s_current = this;

[Simon Fraser (smfr)]

Comment on attachment 331013 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        It can be reverted soon after the incorrect mutations are taken care of.

Maybe add a comment/file a bug so we don't forget that.

[EWS Watchlist]

Comment on attachment 331013 [details]
Patch

Attachment 331013 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6029030

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 331017 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[alan]

Interesting stacktrace

1   com.apple.WebCore             	0x0000000533cc084d WebCore::RenderTreeBuilder::RenderTreeBuilder(WebCore::RenderView&) + 3197 (RenderTreeBuilder.cpp:120)
2   com.apple.WebCore             	0x0000000533cc089d WebCore::RenderTreeBuilder::RenderTreeBuilder(WebCore::RenderView&) + 29 (RenderTreeBuilder.cpp:123)
3   com.apple.WebCore             	0x0000000533ce18df WebCore::RenderTreeUpdater::RenderTreeUpdater(WebCore::Document&) + 463 (RenderTreeUpdater.cpp:87)
4   com.apple.WebCore             	0x0000000533ce194d WebCore::RenderTreeUpdater::RenderTreeUpdater(WebCore::Document&) + 29 (RenderTreeUpdater.cpp:87)
5   com.apple.WebCore             	0x0000000532943d62 WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 1586
6   com.apple.WebCore             	0x00000005329457b3 WebCore::Document::updateStyleIfNeeded() + 467 (Document.cpp:1968)
7   com.apple.WebCore             	0x0000000532952798 WebCore::Document::setFocusedElement(WebCore::Element*, WebCore::FocusDirection, WebCore::Document::FocusRemovalEventsMode) + 2520 (Document.cpp:3976)
8   com.apple.WebCore             	0x00000005331bbfa4 WebCore::FocusController::setFocusedElement(WebCore::Element*, WebCore::Frame&, WebCore::FocusDirection) + 964 (FocusController.cpp:836)
9   com.apple.WebCore             	0x00000005329ee6ec WebCore::Element::focus(bool, WebCore::FocusDirection) + 428 (Element.cpp:2419)
10  com.apple.WebCore             	0x0000000532c8d5b8 WebCore::HTMLFormControlElement::didAttachRenderers()::$_2::operator()() const + 40 (HTMLFormControlElement.cpp:248)
11  com.apple.WebCore             	0x0000000532c8d509 WTF::Function<void ()>::CallableWrapper<WebCore::HTMLFormControlElement::didAttachRenderers()::$_2>::call() + 25 (Function.h:101)
12  com.apple.WebCore             	0x0000000530be8acb WTF::Function<void ()>::operator()() const + 139 (Function.h:56)
13  com.apple.WebCore             	0x0000000533d0a3ae WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler() + 1134 (StyleTreeResolver.cpp:591)
14  com.apple.WebCore             	0x0000000533d0a535 WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler() + 21 (StyleTreeResolver.cpp:599)
15  com.apple.WebCore             	0x0000000532944325 WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 3061 (Document.cpp:1889)
16  com.apple.WebCore             	0x00000005329457b3 WebCore::Document::updateStyleIfNeeded() + 467 (Document.cpp:1968)
17  com.apple.WebCore             	0x000000053294005d WebCore::Document::updateLayout() + 333 (Document.cpp:1988)

post style resolution callback triggers style resolution :)

[alan]

Alternatively it could be fixed right at the ::current() callsites

diff --git a/Source/WebCore/rendering/RenderButton.cpp b/Source/WebCore/rendering/RenderButton.cpp
index 46d60a402d3..118d3eca008 100644
--- a/Source/WebCore/rendering/RenderButton.cpp
+++ b/Source/WebCore/rendering/RenderButton.cpp
@@ -116,7 +116,10 @@ void RenderButton::setText(const String& str)
     if (!m_buttonText) {
         auto newButtonText = createRenderer<RenderTextFragment>(document(), str);
         m_buttonText = makeWeakPtr(*newButtonText);
-        RenderTreeBuilder::current()->insertChild(*this, WTFMove(newButtonText));
+        if (RenderTreeBuilder::current())
+            RenderTreeBuilder::current()->insertChild(*this, WTFMove(newButtonText));
+        else
+            RenderTreeBuilder(*document().renderView()).insertChild(*this, WTFMove(newButtonText));
         return;
     }
 
diff --git a/Source/WebCore/rendering/RenderMenuList.cpp b/Source/WebCore/rendering/RenderMenuList.cpp
index c33113306aa..35534fabbf7 100644
--- a/Source/WebCore/rendering/RenderMenuList.cpp
+++ b/Source/WebCore/rendering/RenderMenuList.cpp
@@ -285,7 +285,10 @@ void RenderMenuList::setText(const String& s)
     else {
         auto newButtonText = createRenderer<RenderText>(document(), textToUse);
         m_buttonText = makeWeakPtr(*newButtonText);
-        RenderTreeBuilder::current()->insertChild(*this, WTFMove(newButtonText));
+        if (RenderTreeBuilder::current())
+            RenderTreeBuilder::current()->insertChild(*this, WTFMove(newButtonText));
+        else
+            RenderTreeBuilder(*document().renderView()).insertChild(*this, WTFMove(newButtonText));
     }

[alan]

Created attachment 331083 [details]
Patch

[Antti Koivisto]

Comment on attachment 331083 [details]
Patch

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

> Source/WebCore/rendering/RenderButton.cpp:122
> +        if (RenderTreeBuilder::current())
> +            RenderTreeBuilder::current()->insertChild(*this, WTFMove(newButtonText));
> +        else
> +            RenderTreeBuilder(*document().renderView()).insertChild(*this, WTFMove(newButtonText));

Could perhaps use a FIXME

[alan]

Created attachment 331086 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 331086 [details]
Patch

Clearing flags on attachment: 331086

Committed r226797: <https://trac.webkit.org/changeset/226797>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.