| Summary: | [FrameView::layout cleanup] Make m_subtreeLayoutRoot weak. | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | zalan <zalan> | ||||||||||||||||||||||||
| Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
| Severity: | Normal | CC: | bfulgham, buildbot, commit-queue, dbates, koivisto, rniwa, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
|
Description
zalan
2017-10-21 08:08:41 PDT
Created attachment 324501 [details]
Patch
Comment on attachment 324501 [details] Patch Attachment 324501 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4946487 Number of test failures exceeded the failure limit. Created attachment 324505 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 324507 [details]
Patch
Comment on attachment 324507 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324507&action=review > Source/WebCore/ChangeLog:9 > + This patch turn m_subtreeLayoutRoot into a optional weak pointer. Optional, because It seems weird that we need an optional weak ptr. Is it necessary to differentiate between having a null pointer and not having a pointer at all (std::nullopt)? Comment on attachment 324507 [details] Patch Attachment 324507 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4947417 New failing tests: svg/custom/delete-text-innerText-crash.html fast/forms/textarea-paste-newline.html svg/custom/bug79798.html editing/pasteboard/smart-paste-in-text-control.html svg/custom/delete-text-crash.html editing/execCommand/insert-list-xml.xhtml editing/pasteboard/copy-paste-pre-line-content.html editing/pasteboard/copy-paste-first-line-in-textarea.html Created attachment 324514 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to Daniel Bates from comment #7) > Comment on attachment 324507 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324507&action=review > > > Source/WebCore/ChangeLog:9 > > + This patch turn m_subtreeLayoutRoot into a optional weak pointer. Optional, because > > It seems weird that we need an optional weak ptr. Is it necessary to > differentiate between having a null pointer and not having a pointer at all > (std::nullopt)? Yes. Not having a pointer at all means we are not planning to run a subtree layout, while having a null pointer means that we are planning to run a subtree layout but due to some tree mutation we have destroyed the renderer that initiated this layout. (In reply to zalan from comment #10) > (In reply to Daniel Bates from comment #7) > > Comment on attachment 324507 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=324507&action=review > > > > > Source/WebCore/ChangeLog:9 > > > + This patch turn m_subtreeLayoutRoot into a optional weak pointer. Optional, because > > > > It seems weird that we need an optional weak ptr. Is it necessary to > > differentiate between having a null pointer and not having a pointer at all > > (std::nullopt)? > Yes. Not having a pointer at all means we are not planning to run a subtree > layout, while having a null pointer means that we are planning to run a > subtree layout but due to some tree mutation we have destroyed the renderer > that initiated this layout. Do we do something different for these cases when FrameView::layout() is called? From briefly looking at the now obsolete attachment #324507 [details] it seems that we always perform a full layout (unless we do not have a RenderView). (In reply to Build Bot from comment #9) > Created attachment 324514 [details] > Archive of layout-test-results from ews114 for mac-elcapitan > > The attached test failures were seen while running run-webkit-tests on the > mac-debug-ews. > Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6 These failures actually are not regressions. The patch reveals a bug where we mark the RenderView dirty soon after we schedule a subtree layout. (In reply to Daniel Bates from comment #11) > (In reply to zalan from comment #10) > > (In reply to Daniel Bates from comment #7) > > > Comment on attachment 324507 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=324507&action=review > > > > > > > Source/WebCore/ChangeLog:9 > > > > + This patch turn m_subtreeLayoutRoot into a optional weak pointer. Optional, because > > > > > > It seems weird that we need an optional weak ptr. Is it necessary to > > > differentiate between having a null pointer and not having a pointer at all > > > (std::nullopt)? > > Yes. Not having a pointer at all means we are not planning to run a subtree > > layout, while having a null pointer means that we are planning to run a > > subtree layout but due to some tree mutation we have destroyed the renderer > > that initiated this layout. > > Do we do something different for these cases when FrameView::layout() is > called? From briefly looking at the now obsolete attachment #324507 [details] > [details] it seems that we always perform a full layout (unless we do not > have a RenderView). ASSERT on the second case and skip the layout. (clearly missing from this patch) Created attachment 324518 [details]
Patch
Attachment 324518 [details] did not pass style-queue:
ERROR: Source/WebCore/page/FrameView.cpp:1399: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to zalan from comment #14) > Created attachment 324518 [details] > Patch This is just for EWSing -needs to be split up into 2 separate patches. Comment on attachment 324518 [details] Patch Attachment 324518 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4949265 New failing tests: fast/dynamic/remove-invisible-node-inside-selection.html Created attachment 324520 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 324518 [details] Patch Attachment 324518 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4949270 New failing tests: fast/dynamic/remove-invisible-node-inside-selection.html Created attachment 324521 [details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 324533 [details]
Patch
(In reply to zalan from comment #12) > (In reply to Build Bot from comment #9) > > Created attachment 324514 [details] > > Archive of layout-test-results from ews114 for mac-elcapitan > > > > The attached test failures were seen while running run-webkit-tests on the > > mac-debug-ews. > > Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6 > > These failures actually are not regressions. The patch reveals a bug where > we mark the RenderView dirty soon after we schedule a subtree layout. See bug 178651 Created attachment 324540 [details]
Patch
Created attachment 324541 [details]
Patch
Created attachment 324579 [details]
Patch
Comment on attachment 324579 [details] Patch Clearing flags on attachment: 324579 Committed r223853: <https://trac.webkit.org/changeset/223853> All reviewed patches have been landed. Closing bug. |