Bug 221175 - StaticRange should keep its start and end containers alive
Summary: StaticRange should keep its start and end containers alive
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-29 21:07 PST by Ryosuke Niwa
Modified: 2021-03-05 01:41 PST (History)
11 users (show)

See Also:


Attachments
Fixes the bug (15.75 KB, patch)
2021-01-29 21:10 PST, Ryosuke Niwa
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2021-01-29 21:07:17 PST
StaticRange doesn't keep its start and end containers alive right now.
As a result, some DOM nodes can get garbage collected prematurely.
Comment 1 Ryosuke Niwa 2021-01-29 21:10:56 PST
Created attachment 418807 [details]
Fixes the bug
Comment 2 Wenson Hsieh 2021-01-29 21:15:00 PST
Comment on attachment 418807 [details]
Fixes the bug

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

> Source/WebCore/ChangeLog:9
> +        To do that, we sff them as opaque roots during the marking phase in visitAdditionalChildren.

Nit - sff => add
Comment 3 Ryosuke Niwa 2021-01-30 00:52:16 PST
(In reply to Wenson Hsieh from comment #2)
> Comment on attachment 418807 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418807&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        To do that, we sff them as opaque roots during the marking phase in visitAdditionalChildren.
> 
> Nit - sff => add

Fixed.

Thanks for the review!
Comment 4 Ryosuke Niwa 2021-01-30 00:53:12 PST
Committed r272114: <https://trac.webkit.org/changeset/272114>
Comment 5 Radar WebKit Bug Importer 2021-01-30 00:54:13 PST
<rdar://problem/73788028>
Comment 6 Darin Adler 2021-02-01 17:04:43 PST
Should we add a test to make sure that we can’t create a reference cycle that causes leaks by having a static range as the value of a property on one of its endpoint nodes?
Comment 7 Ryosuke Niwa 2021-02-01 21:57:51 PST
(In reply to Darin Adler from comment #6)
> Should we add a test to make sure that we can’t create a reference cycle
> that causes leaks by having a static range as the value of a property on one
> of its endpoint nodes?

Maybe? I guess we can artificially introduce a leaking bug & verify the test works?
Comment 8 Darin Adler 2021-02-02 09:15:05 PST
We have tests for this, like LayoutTests/fast/dom/reference-cycle-leaks.html, but I don’t think we have one that covers StaticRange.
Comment 9 Ryosuke Niwa 2021-03-05 01:41:35 PST
(In reply to Darin Adler from comment #8)
> We have tests for this, like
> LayoutTests/fast/dom/reference-cycle-leaks.html, but I don’t think we have
> one that covers StaticRange.

Alright, filed https://bugs.webkit.org/show_bug.cgi?id=222786 to add those tests.