Release Assert @ WebCore::RenderTreeBuilder::RenderTreeBuilder
Created attachment 400966 [details] Patch
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".
Created attachment 400972 [details] Patch
rdar://problem/63494588
Comment on attachment 400972 [details] Patch r=me
Committed r262524: <https://trac.webkit.org/changeset/262524> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400972 [details].
Reverted r262524 for reason: Reverting because this commit may have caused issues with other tests. Committed r262651: <https://trac.webkit.org/changeset/262651>
Created attachment 401374 [details] Patch
Comment on attachment 401374 [details] Patch r=me
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.
Committed r262743: <https://trac.webkit.org/changeset/262743> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401374 [details].
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
Created attachment 401471 [details] Patch
Comment on attachment 401471 [details] Patch Re-landing the test, to check if test triggers bot failures.
Created attachment 401494 [details] Patch
Comment on attachment 401494 [details] Patch Re-landing second code change to see if this will result into any regression, seen earlier.
Comment on attachment 401494 [details] Patch r=me
Committed r262835: <https://trac.webkit.org/changeset/262835> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401494 [details].
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.
Re-opening to incorporate Darin's comment - replacing auto map = WTFMove(widgetNewParentMap()) with auto map = std::exchange(widgetNewParentMap(), { }).
Created attachment 401570 [details] Patch
should have updated webkit after Committ r262835: to make the new change. correcting it
Created attachment 401599 [details] Patch
Comment on attachment 401599 [details] Patch r=me
> > 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.
(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 :)
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.
Committed r262877: <https://trac.webkit.org/changeset/262877> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401599 [details].
re-opening to commit the test again to check if test is the actual cause of bot failure
Created attachment 401623 [details] Patch
Committed r262909: <https://trac.webkit.org/changeset/262909> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401623 [details].
Reverted r262909 for reason: This test causes macOS WK1 tests to intermittently exit early Committed r262919: <https://trac.webkit.org/changeset/262919>
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.
Re-opening to land the modified test.
Created attachment 401808 [details] Patch
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.
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?
Committed r263015: <https://trac.webkit.org/changeset/263015> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401808 [details].
(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.
*** Bug 211800 has been marked as a duplicate of this bug. ***