Bug 188575 - [IntersectionObserver] Do not hold a strong reference to the root element
Summary: [IntersectionObserver] Do not hold a strong reference to the root element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ali Juma
URL:
Keywords: InRadar
Depends on:
Blocks: 159475
  Show dependency treegraph
 
Reported: 2018-08-14 13:25 PDT by Ali Juma
Modified: 2018-08-15 07:15 PDT (History)
11 users (show)

See Also:


Attachments
Patch (13.34 KB, patch)
2018-08-14 13:34 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch for landing (13.27 KB, patch)
2018-08-15 06:29 PDT, Ali Juma
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ali Juma 2018-08-14 13:25:21 PDT
An IntersectionObserver should not keep its root element alive when there aren't any other references to the root.
Comment 1 Ali Juma 2018-08-14 13:34:31 PDT
Created attachment 347106 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-08-14 16:03:01 PDT
Comment on attachment 347106 [details]
Patch

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

> Source/WebCore/page/IntersectionObserver.cpp:126
> +        for (size_t i = 0; i < elementObservers.size(); ++i) {
> +            if (elementObservers.at(i) == this) {
> +                elementObservers.remove(i);
> +                break;
> +            }

This should use removeFirst()

> Source/WebCore/page/IntersectionObserver.h:43
> +    Vector<IntersectionObserver*> observers;

Who owns the IntersectionObservers? Would be nice for the comment to say what the ownership model is.

> Source/WebCore/page/IntersectionObserver.h:75
> +    Element* m_root;

We should make WeakPtr work for Elements.
Comment 3 Ali Juma 2018-08-15 06:29:18 PDT
Created attachment 347159 [details]
Patch for landing
Comment 4 Ali Juma 2018-08-15 06:31:22 PDT
Comment on attachment 347106 [details]
Patch

Thanks for the review!

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

>> Source/WebCore/page/IntersectionObserver.cpp:126
>> +            }
> 
> This should use removeFirst()

Done.

>> Source/WebCore/page/IntersectionObserver.h:43
>> +    Vector<IntersectionObserver*> observers;
> 
> Who owns the IntersectionObservers? Would be nice for the comment to say what the ownership model is.

Updated the comment to mention that observers are owned by JavaScript wrappers and by IntersectionObserverRegistrations for currently-observed targets (to be implemented in a future patch).
Comment 5 WebKit Commit Bot 2018-08-15 07:13:13 PDT
Comment on attachment 347159 [details]
Patch for landing

Clearing flags on attachment: 347159

Committed r234884: <https://trac.webkit.org/changeset/234884>
Comment 6 WebKit Commit Bot 2018-08-15 07:13:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2018-08-15 07:15:43 PDT
<rdar://problem/43332185>