Bug 293183 – Allow renderers to get destroyed async after detach

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[alan]

ssia

[alan]

Created attachment 475285 [details]
Patch

[alan]

Created attachment 475287 [details]
Patch with dependencies

[alan]

Created attachment 475289 [details]
Patch with dependencies

[alan]

Created attachment 475293 [details]
Patch with dependencies

[alan]

Created attachment 475295 [details]
Patch with dependencies

[alan]

Created attachment 475307 [details]
Patch

[alan]

Created attachment 475313 [details]
Patch

[alan]

Created attachment 475319 [details]
Patch

[alan]

Created attachment 475320 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 475320 [details]
Patch

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

> Source/WebCore/page/LocalFrameViewLayoutContext.cpp:762
> +    auto isEligibleForDelayedDeletation = [&] {

Typo

[Antti Koivisto]

Comment on attachment 475320 [details]
Patch

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

> Source/WebCore/page/LocalFrameViewLayoutContext.cpp:756
> +static constexpr int maximumNumberOfDetachedRenderers = 5000;

could be in the function scope

> Source/WebCore/page/LocalFrameViewLayoutContext.cpp:772
> +        if (m_renderers.size() == maximumNumberOfDetachedRenderers)
> +            return false;
> +        // FIXME: Cleanup RenderWidget's destruction process (ref vs. delete this. see RenderObject::destroy)
> +        return !is<RenderWidget>(detachedRenderer);
> +    };
> +    if (isEligibleForDelayedDeletation()) {
> +        m_renderers.append(detachedRenderer.release());
> +        return true;
> +    }
> +    return false;

or I guess we could fully delete the contents of the previous list and start building a new one at this point

[alan]

Created attachment 475325 [details]
Patch

[alan]

Created attachment 475326 [details]
Patch

[alan]

(In reply to Antti Koivisto from comment #11)
> Comment on attachment 475320 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=475320&action=review
> 
> > Source/WebCore/page/LocalFrameViewLayoutContext.cpp:756
> > +static constexpr int maximumNumberOfDetachedRenderers = 5000;
> 
> could be in the function scope
> 
> > Source/WebCore/page/LocalFrameViewLayoutContext.cpp:772
> > +        if (m_renderers.size() == maximumNumberOfDetachedRenderers)
> > +            return false;
> > +        // FIXME: Cleanup RenderWidget's destruction process (ref vs. delete this. see RenderObject::destroy)
> > +        return !is<RenderWidget>(detachedRenderer);
> > +    };
> > +    if (isEligibleForDelayedDeletation()) {
> > +        m_renderers.append(detachedRenderer.release());
> > +        return true;
> > +    }
> > +    return false;
> 
> or I guess we could fully delete the contents of the previous list and start
> building a new one at this point
we surely could!

[Antti Koivisto]

Comment on attachment 475326 [details]
Patch

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

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:236
> +        auto* rendererToDelete = toDestroy.get();

could probably be a CheckedRef

[alan]

Created attachment 475337 [details]
Patch

[alan]

(In reply to Antti Koivisto from comment #15)
> Comment on attachment 475326 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=475326&action=review
> 
> > Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:236
> > +        auto* rendererToDelete = toDestroy.get();
> 
> could probably be a CheckedRef
ofc

[alan]

Created attachment 475340 [details]
[fast-cq]Patch

[EWS]

Committed 295267@main (09f66151f2c1): <https://commits.webkit.org/295267@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/151847419>