Bug 178621 – [FrameView::layout cleanup] Make m_subtreeLayoutRoot weak.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

ssia.

[Radar WebKit Bug Importer]

<rdar://problem/35110321>

[Radar WebKit Bug Importer]

<rdar://problem/35110347>

[alan]

Created attachment 324501 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[alan]

Created attachment 324507 [details]
Patch

[Daniel Bates]

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)?

[Build Bot]

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

[Build Bot]

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

[alan]

(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.

[Daniel Bates]

(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).

[alan]

(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.

[alan]

(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)

[alan]

Created attachment 324518 [details]
Patch

[Build Bot]

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.

[alan]

(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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[alan]

Created attachment 324533 [details]
Patch

[alan]

(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

[alan]

Created attachment 324540 [details]
Patch

[alan]

Created attachment 324541 [details]
Patch

[alan]

Created attachment 324579 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 324579 [details]
Patch

Clearing flags on attachment: 324579

Committed r223853: <https://trac.webkit.org/changeset/223853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.