Bug 212714 – Release Assert @ WebCore::RenderTreeBuilder::RenderTreeBuilder

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

[Pinki Gyanchandani]

Release Assert @ WebCore::RenderTreeBuilder::RenderTreeBuilder

[Pinki Gyanchandani]

Created attachment 400966 [details]
Patch

[Geoffrey Garen]

Comment on attachment 400966 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Widget Removal in middle of building a Render Tree causes side effects, leading to Release Assert. Moved the scope for suspension of widgets

Removal => removal

in middle => in the middle

> Source/WebCore/ChangeLog:9
> +        update to RenderTreeBuilder instead of having it at RenderTreeUpdater. 

at RenderTreeUpdater => in RenderTreeUpdater

> Source/WebCore/ChangeLog:11
> +        Also made sure that the WidgetHierarchyUpdatesSuspensionScope::moveWidgets(), should handle all widgets scheduled for move. 

WidgetHierarchyUpdatesSuspensionScope::moveWidgets(), => WidgetHierarchyUpdatesSuspensionScope::moveWidgets()

scheduled for move. => scheduled to move, including new widgets scheduled during moveWidgets().

> Source/WebCore/rendering/updating/RenderTreeBuilder.h:66
> +    WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;

WebKit style puts data members together at the end of the class definition, and prefixes data members with "m_". 

I would put this declaration right above "RenderView& m_view" and call it "WidgetHierarchyUpdatesSuspensionScope m_widgetHierarchyUpdatesSuspensionScope".

[Pinki Gyanchandani]

Created attachment 400972 [details]
Patch

[Pinki Gyanchandani]

rdar://problem/63494588

[Geoffrey Garen]

Comment on attachment 400972 [details]
Patch

r=me

[EWS]

Committed r262524: <https://trac.webkit.org/changeset/262524>

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

[Jason Lawrence]

Reverted r262524 for reason:

Reverting because this commit may have caused issues with other tests.

Committed r262651: <https://trac.webkit.org/changeset/262651>

[Pinki Gyanchandani]

Created attachment 401374 [details]
Patch

[Geoffrey Garen]

Comment on attachment 401374 [details]
Patch

r=me

[Geoffrey Garen]

We are re-landing this patch in pieces to see if the mysterious occasional bot failure re-surfaces, and if so, which part of the patch triggers it.

[EWS]

Committed r262743: <https://trac.webkit.org/changeset/262743>

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

[Pinki Gyanchandani]

Last submitted code changes have worked fine. No regression is observed.

Re-opening to land the test next to determine, if test was the cause of regression

[Pinki Gyanchandani]

Created attachment 401471 [details]
Patch

[Pinki Gyanchandani]

Comment on attachment 401471 [details]
Patch

Re-landing the test, to check if test triggers bot failures.

[Pinki Gyanchandani]

Created attachment 401494 [details]
Patch

[Pinki Gyanchandani]

Comment on attachment 401494 [details]
Patch

Re-landing second code change to see if this will result into any regression, seen earlier.

[Geoffrey Garen]

Comment on attachment 401494 [details]
Patch

r=me

[EWS]

Committed r262835: <https://trac.webkit.org/changeset/262835>

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

[Darin Adler]

Comment on attachment 401494 [details]
Patch

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

> Source/WebCore/rendering/RenderWidget.cpp:60
> +        auto map = WTFMove(widgetNewParentMap());

In cases like this where we *rely* in the behavior that the source ends up empty afterward, instead of just allowing move as an optimization, we should write using std::exchange instead.

    auto map = std::exchange(widgetNewParentMap(), { });

The WTFMove form allows move as an optimization, but also allows the source map to have a remnant value other than empty. The std::exchange form guarantees that the source map will be empty. Both should generate nearly identical code if all the optimization is working properly.

[Pinki Gyanchandani]

Re-opening to incorporate Darin's comment -

replacing auto map = WTFMove(widgetNewParentMap()) with auto map = std::exchange(widgetNewParentMap(), { }).

[Pinki Gyanchandani]

Created attachment 401570 [details]
Patch

[Pinki Gyanchandani]

should have updated webkit after Committ r262835: to make the new change. correcting it

[Pinki Gyanchandani]

Created attachment 401599 [details]
Patch

[Geoffrey Garen]

Comment on attachment 401599 [details]
Patch

r=me

[Geoffrey Garen]

> > Source/WebCore/rendering/RenderWidget.cpp:60
> > +        auto map = WTFMove(widgetNewParentMap());
> 
> In cases like this where we *rely* in the behavior that the source ends up
> empty afterward, instead of just allowing move as an optimization, we should
> write using std::exchange instead.
> 
>     auto map = std::exchange(widgetNewParentMap(), { });
> 
> The WTFMove form allows move as an optimization, but also allows the source
> map to have a remnant value other than empty. The std::exchange form
> guarantees that the source map will be empty. Both should generate nearly
> identical code if all the optimization is working properly.

+Chris because I think he has an opinion on move producing an empty value.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #25)
> > > Source/WebCore/rendering/RenderWidget.cpp:60
> > > +        auto map = WTFMove(widgetNewParentMap());
> > 
> > In cases like this where we *rely* in the behavior that the source ends up
> > empty afterward, instead of just allowing move as an optimization, we should
> > write using std::exchange instead.
> > 
> >     auto map = std::exchange(widgetNewParentMap(), { });
> > 
> > The WTFMove form allows move as an optimization, but also allows the source
> > map to have a remnant value other than empty. The std::exchange form
> > guarantees that the source map will be empty. Both should generate nearly
> > identical code if all the optimization is working properly.
> 
> +Chris because I think he has an opinion on move producing an empty value.

I indeed prefer if our move constructors leave the object empty and clean to reduce the change of bugs. We've had a lot of bugs like that in the past. This makes adopting of std::optional a bit controversial because its move constructor leaves the optional in a bad state.

That said, I am all for using std::exchange() instead of WTFMove() when we know we are going to re-use the value after. It is the proper C++ way and I think we should enforce it in new code. It would make a switch to std::optional some day easier too although I have concerns about that part :)

[Darin Adler]

Exactly. This use of std::exchange is something Chris and I agree on! We are still debating whether to ever move from WTF::Optional to std::optional; a related but distinct topic.

For what it’s worth, my patch to use std::optional did not show any failures in EWS regression tests, but that doesn’t prove whether or not it introduces bugs that were not detected by those tests.

[EWS]

Committed r262877: <https://trac.webkit.org/changeset/262877>

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

[Pinki Gyanchandani]

re-opening to commit the test again  to check if test is the actual cause of bot failure

[Pinki Gyanchandani]

Created attachment 401623 [details]
Patch

[EWS]

Committed r262909: <https://trac.webkit.org/changeset/262909>

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

[Ryan Haddad]

Reverted r262909 for reason:

This test causes macOS WK1 tests to intermittently exit early

Committed r262919: <https://trac.webkit.org/changeset/262919>

[Pinki Gyanchandani]

closing this ticket and corresponding radar rdar://problem/63494588 as the code changes are working fine. Its the test case which is causing regression in bots. 
Need to debug test and along with tool dumprendertree to figure out reason of failure.
A separate bug would be created for the same.

[Pinki Gyanchandani]

Re-opening to land the modified test.

[Pinki Gyanchandani]

Created attachment 401808 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 401808 [details]
Patch

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

> LayoutTests/fast/rendering/widget-removal-in-render-tree-builder-crash.html:38
> +<details id="z" open="true" ontoggle="runTest()">a</details>

I am still very skeptical of running test JS in the middle of parsing. Not blocking review, but this could be worth some more effort.

[Geoffrey Garen]

Comment on attachment 401808 [details]
Patch

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

r=me

>> LayoutTests/fast/rendering/widget-removal-in-render-tree-builder-crash.html:38
>> +<details id="z" open="true" ontoggle="runTest()">a</details>
> 
> I am still very skeptical of running test JS in the middle of parsing. Not blocking review, but this could be worth some more effort.

Pinki, can you test whether this test still fails without your patch if you move the call to runTest() into the window load event?

[EWS]

Committed r263015: <https://trac.webkit.org/changeset/263015>

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

[Pinki Gyanchandani]

(In reply to Geoffrey Garen from comment #37)
> Comment on attachment 401808 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401808&action=review
> 
> r=me
> 
> >> LayoutTests/fast/rendering/widget-removal-in-render-tree-builder-crash.html:38
> >> +<details id="z" open="true" ontoggle="runTest()">a</details>
> > 
> > I am still very skeptical of running test JS in the middle of parsing. Not blocking review, but this could be worth some more effort.
> 
> Pinki, can you test whether this test still fails without your patch if you
> move the call to runTest() into the window load event?

I tried to call runTest with window load event, but then crash is not reproduced, which means the test is meant to be calling runTest in middle of parsing.

Since the test is not causing any regression in bots, will not make any further changes in the code.

[Ryosuke Niwa]

*** Bug 211800 has been marked as a duplicate of this bug. ***