Bug 197457 – The JS wrapper of target in an ResizeObserverEntry should not get collected

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[cathiechen]

The JS wrappers of targets in ResizeObserverEntry shouldn't be collected. Use GCReachableRef to make it reachable for GC.

[cathiechen]

Created attachment 368670 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 368670 [details]
Patch

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

> Source/WebCore/page/ResizeObserverEntry.h:58
> -    RefPtr<Element> m_target;
> +    GCReachableRef<Element> m_target;

This isn't right. Because ResizeObserverEntry is exposed to JS, this can lead to leaks.
Namely, if you store this object as a property on the element, it would leak the node permanently.
Please add a layout test to test that.

For ResizeObserverEntry, you'd need a custom visitAdditionalChildren function as we do for MutationRecord.
Add JSCustomMarkFunction to ResizeObserverEntry and add JSResizeObserverEntryCustom.cpp
with JSResizeObserverEntryCustom::visitAdditionalChildren which visits the target.
This makes ResizeObserverEntry's target visible from GC as long as ResizeObserverEntry is alive.

In MutationObserver, we have to separately keep JS wrappers of the observed elements using GCReachableRef
because mutation record of an element is delivered even if  the element had been removed from a document.
Is that possible with ResizeObserver?
Or would an element get automatically removed from the active observation when it's removed from a document?

> LayoutTests/resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-alive.html:13
> +const iterationCount = 5;

Let's do like 10 or 20 iterations to be safe.

[cathiechen]

Created attachment 369394 [details]
Patch

[cathiechen]

Hi Ryosuke,

Thanks for the review! :)

> This isn't right. Because ResizeObserverEntry is exposed to JS, this can
> lead to leaks.
> Namely, if you store this object as a property on the element, it would leak
> the node permanently.
> Please add a layout test to test that.

Ah! Yes, there's leak. I've changed it as you suggested in the new patch. 'entry.target.myEntry = entry;' could reproduce this leak. But it seems not easy to add the test case. I'll add this test case in the next patch.

> 
> For ResizeObserverEntry, you'd need a custom visitAdditionalChildren
> function as we do for MutationRecord.
> Add JSCustomMarkFunction to ResizeObserverEntry and add
> JSResizeObserverEntryCustom.cpp
> with JSResizeObserverEntryCustom::visitAdditionalChildren which visits the
> target.
> This makes ResizeObserverEntry's target visible from GC as long as
> ResizeObserverEntry is alive.
> 
> In MutationObserver, we have to separately keep JS wrappers of the observed
> elements using GCReachableRef
> because mutation record of an element is delivered even if  the element had
> been removed from a document.
> Is that possible with ResizeObserver?
> Or would an element get automatically removed from the active observation
> when it's removed from a document?

Yes, the target would get delivered for the last time if it's removed. I used GCReachableRef like MutationObserver. But the adding time would be different. The target will be added to the list when it's removing from DOM tree.

> 
> > LayoutTests/resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-alive.html:13
> > +const iterationCount = 5;
> 
> Let's do like 10 or 20 iterations to be safe.

Done. Changed it to 10.

[EWS Watchlist]

Comment on attachment 369394 [details]
Patch

Attachment 369394 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12136650

New failing tests:
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html
http/tests/css/filters-on-iframes.html

[EWS Watchlist]

Created attachment 369426 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[cathiechen]

Created attachment 369680 [details]
Patch

[EWS Watchlist]

Comment on attachment 369680 [details]
Patch

Attachment 369680 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12170201

New failing tests:
resize-observer/element-leak.html

[EWS Watchlist]

Created attachment 369681 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369680 [details]
Patch

Attachment 369680 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12170312

New failing tests:
resize-observer/element-leak.html

[EWS Watchlist]

Created attachment 369683 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[cathiechen]

Created attachment 369738 [details]
Patch

[EWS Watchlist]

Comment on attachment 369738 [details]
Patch

Attachment 369738 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12177576

New failing tests:
resize-observer/element-leak.html

[EWS Watchlist]

Created attachment 369746 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369738 [details]
Patch

Attachment 369738 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12178000

New failing tests:
resize-observer/element-leak.html

[EWS Watchlist]

Created attachment 369752 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369738 [details]
Patch

Attachment 369738 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12178014

New failing tests:
resize-observer/element-leak.html

[EWS Watchlist]

Created attachment 369755 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4

[EWS Watchlist]

Comment on attachment 369738 [details]
Patch

Attachment 369738 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12178424

New failing tests:
resize-observer/element-leak.html

[EWS Watchlist]

Created attachment 369756 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369738 [details]
Patch

Attachment 369738 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12179247

New failing tests:
resize-observer/element-leak.html

[EWS Watchlist]

Created attachment 369764 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[cathiechen]

Created attachment 369833 [details]
with logs

Sorry, I couldn't reproduce the issue in mac highsierra and win-future. I'm uploading a patch with logs.

[cathiechen]

Created attachment 369837 [details]
To test wincairo build error

[cathiechen]

Created attachment 369840 [details]
WIP

[EWS Watchlist]

Comment on attachment 369840 [details]
WIP

Attachment 369840 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12187564

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 369844 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369840 [details]
WIP

Attachment 369840 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12187567

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 369845 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369840 [details]
WIP

Attachment 369840 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12187551

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 369846 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[cathiechen]

Created attachment 369847 [details]
WIP

[EWS Watchlist]

Attachment 369847 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:563:  Missing space before ( in if(  [whitespace/parens] [5]
ERROR: Source/WebCore/dom/Document.cpp:582:  Missing space before ( in if(  [whitespace/parens] [5]
ERROR: Source/WebCore/dom/Document.cpp:654:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[cathiechen]

Created attachment 369848 [details]
WIP

[EWS Watchlist]

Attachment 369848 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:563:  Missing space before ( in if(  [whitespace/parens] [5]
ERROR: Source/WebCore/dom/Document.cpp:582:  Missing space before ( in if(  [whitespace/parens] [5]
ERROR: Source/WebCore/dom/Document.cpp:654:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[cathiechen]

Created attachment 369854 [details]
WIP

[EWS Watchlist]

Comment on attachment 369854 [details]
WIP

Attachment 369854 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12188510

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 369861 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[cathiechen]

Created attachment 369862 [details]
WIP

[EWS Watchlist]

Comment on attachment 369862 [details]
WIP

Attachment 369862 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12189155

New failing tests:
http/tests/IndexedDB/collect-IDB-objects.https.html
performance-api/performance-observer-no-document-leak.html
http/tests/media/clearkey/collect-webkit-media-session.html
resize-observer/element-leak.html
resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-alive.html

[EWS Watchlist]

Created attachment 369866 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369862 [details]
WIP

Attachment 369862 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12189275

New failing tests:
http/tests/IndexedDB/collect-IDB-objects.https.html
performance-api/performance-observer-no-document-leak.html
http/tests/media/media-stream/collect-media-devices.https.html
http/tests/media/clearkey/collect-webkit-media-session.html
intersection-observer/no-document-leak.html
resize-observer/element-leak.html

[EWS Watchlist]

Created attachment 369870 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369862 [details]
WIP

Attachment 369862 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12189374

New failing tests:
intersection-observer/no-document-leak.html
http/tests/IndexedDB/collect-IDB-objects.https.html
performance-api/performance-observer-no-document-leak.html
resize-observer/element-leak.html
resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-alive.html

[EWS Watchlist]

Created attachment 369876 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4

[EWS Watchlist]

Comment on attachment 369862 [details]
WIP

Attachment 369862 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12189363

New failing tests:
http/tests/IndexedDB/collect-IDB-objects.https.html
performance-api/performance-observer-no-document-leak.html
http/tests/media/clearkey/collect-webkit-media-session.html
resize-observer/element-leak.html
resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-alive.html

[EWS Watchlist]

Created attachment 369877 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[cathiechen]

Created attachment 369921 [details]
WIP

[EWS Watchlist]

Comment on attachment 369921 [details]
WIP

Attachment 369921 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12194376

New failing tests:
resize-observer/element-leak-2.html
performance-api/performance-observer-no-document-leak.html
http/tests/media/clearkey/collect-webkit-media-session.html
http/tests/IndexedDB/collect-IDB-objects.https.html
resize-observer/element-leak.html
resize-observer/resize-observer-keeps-js-wrapper-of-target-alive.html
resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-alive.html
resize-observer/element-leak-1.html

[EWS Watchlist]

Created attachment 369922 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369921 [details]
WIP

Attachment 369921 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12194386

New failing tests:
http/tests/IndexedDB/collect-IDB-objects.https.html
performance-api/performance-observer-no-document-leak.html
http/tests/media/media-stream/collect-media-devices.https.html
resize-observer/element-leak-2.html
resize-observer/element-leak.html
http/tests/media/clearkey/collect-webkit-media-session.html
intersection-observer/no-document-leak.html
resize-observer/element-leak-1.html

[EWS Watchlist]

Created attachment 369923 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369921 [details]
WIP

Attachment 369921 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12194461

New failing tests:
resize-observer/element-leak-1.html
resize-observer/element-leak.html

[EWS Watchlist]

Created attachment 369925 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[EWS Watchlist]

Comment on attachment 369921 [details]
WIP

Attachment 369921 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12194427

New failing tests:
resize-observer/element-leak-2.html
performance-api/performance-observer-no-document-leak.html
http/tests/media/clearkey/collect-webkit-media-session.html
http/tests/IndexedDB/collect-IDB-objects.https.html
resize-observer/element-leak.html
resize-observer/resize-observer-keeps-js-wrapper-of-target-alive.html
resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-alive.html
resize-observer/element-leak-1.html

[EWS Watchlist]

Created attachment 369926 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369921 [details]
WIP

Attachment 369921 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12194447

New failing tests:
resize-observer/element-leak-2.html
performance-api/performance-observer-no-document-leak.html
http/tests/IndexedDB/collect-IDB-objects.https.html
resize-observer/element-leak.html
intersection-observer/no-document-leak.html
resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-alive.html
resize-observer/element-leak-1.html

[EWS Watchlist]

Created attachment 369927 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4

[cathiechen]

Created attachment 369935 [details]
Patch

[EWS Watchlist]

Comment on attachment 369935 [details]
Patch

Attachment 369935 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12195705

New failing tests:
storage/indexeddb/modern/gc-closes-database.html
resize-observer/element-leak.html

[EWS Watchlist]

Created attachment 369942 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[cathiechen]

The failure in win seems not specific to ResizeObserver. I opened a new bug for it.

[cathiechen]

(In reply to cathiechen from comment #62)
> The failure in win seems not specific to ResizeObserver. I opened a new bug
> for it.

https://bugs.webkit.org/show_bug.cgi?id=197908

[cathiechen]

Created attachment 370111 [details]
Patch

[cathiechen]

Comment on attachment 370111 [details]
Patch

Hi Ryosuke,

I think this patch it ready to review.
Sorry, I couldn't look into the release problem in win platform, because I don't have windows env. So I filed a bug(197908) for it. And make element-leak.html skipped in win.

Please take a look of this patch when you have time. Thanks:)

[Ryosuke Niwa]

Comment on attachment 370111 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        For ResizeObserver, if targets removed, it will get fired for the last time. So we should keep the JS wrappers

Nit: if targets *are* removed,

> Source/WebCore/ChangeLog:11
> +        live from the point target removed to observations delivered. Add these targets to a GCReachableRef list

from the point target*s are* removed to *when* observations *are* delivered.

> Source/WebCore/dom/Document.cpp:4432
> +#if ENABLE(RESIZE_OBSERVER)
> +            if (is<Element>(*n))
> +                downcast<Element>(*n).addResizeObserverPendingTargetIfNeeded();
> +#endif

I think it's better if we just always added the element in m_observations / m_activeObservations instead.
Otherwise, we can forget to call this function elsewhere in the future when more code paths get introduced.

> Source/WebCore/page/ResizeObserver.cpp:152
> +    m_pendingTargets.removeFirstMatching([&target](auto& pendingTarget) {
> +        return pendingTarget.ptr() == &target;

Why is it correct to only remove the first entry?

> Source/WebCore/page/ResizeObserver.cpp:166
> +    m_pendingTargets.append(target);

It seems that we can add a single element multiple times. How is that correct?

[cathiechen]

Comment on attachment 370111 [details]
Patch

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

Sorry for the late reply.

>> Source/WebCore/ChangeLog:10
>> +        For ResizeObserver, if targets removed, it will get fired for the last time. So we should keep the JS wrappers
> 
> Nit: if targets *are* removed,

Done

>> Source/WebCore/ChangeLog:11
>> +        live from the point target removed to observations delivered. Add these targets to a GCReachableRef list
> 
> from the point target*s are* removed to *when* observations *are* delivered.

Done. Changed to:
"For ResizeObserver, if targets are removed, it will get fired for the last time. We also need to keep these JS 
 wrappers live. So add these targets to a GCReachableRef list once they're observed."

>> Source/WebCore/dom/Document.cpp:4432
>> +#endif
> 
> I think it's better if we just always added the element in m_observations / m_activeObservations instead.
> Otherwise, we can forget to call this function elsewhere in the future when more code paths get introduced.

Make sense! I will add the element to the list once it's observed.

>> Source/WebCore/page/ResizeObserver.cpp:166
>> +    m_pendingTargets.append(target);
> 
> It seems that we can add a single element multiple times. How is that correct?

Ah, indeed! Sorry for the mistake.

Since we'll always add the element to the list, I moved `m_pendingTargets.append(target);` to `observe()`. 
This could make sure the element could be added once.

[cathiechen]

Created attachment 370681 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 370681 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        * bindings/js/JSResizeObserverEntryCustom.cpp: Copied from Source/WebCore/page/ResizeObserverEntry.idl.

This cpp file isn't a copy of idl file??

> Source/WebCore/page/ResizeObserver.cpp:-177
> -    m_observations.clear();
> -    m_activeObservations.clear();

Please clarify why we're no longer clearing m_observations.clear and  m_activeObservations here.

> LayoutTests/resize-observer/element-leak.html:13
> +if (window.testRunner)
> +    testRunner.dumpAsText();

No need to call dumpAsText if you're including testharness.js / testharnessreport.js

> LayoutTests/resize-observer/element-leak.html:23
> +            if (internals && !internals.isDocumentAlive(documentIdentifier)) {
> +                clearInterval(handle);

Seems like this test always requires internals?
In that case, it seems like we don't even need to use testharness.js.

> LayoutTests/resize-observer/element-leak.html:32
> +    let test = async_test('test0: Test elements leak');

Can we use promise_test instead?

> LayoutTests/resize-observer/element-leak.html:55
> +        log('timeout: ');
> +        test.done();
> +    }, 5000);

Just specify timeout to promise_test instead.

> LayoutTests/resize-observer/element-leak.html:62
> +test0();

Ugh... can we give this more descriptive name like runTest?

> LayoutTests/resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-alive.html:46
> +        document.querySelectorAll('div').forEach(target => { target.remove(); });

No need for { ~; }. You can just write `target => target.remove()` here.

> LayoutTests/resize-observer/resize-observer-keeps-js-wrapper-of-target-alive.html:33
> +    document.querySelectorAll('div').forEach(target => { target.remove(); });

Ditto.

> LayoutTests/resize-observer/resize-observer-keeps-js-wrapper-of-target-alive.html:41
> +    document.querySelectorAll('div').forEach(target => { observer.observe(target); });

Ditto.

> LayoutTests/resize-observer/resources/element-leak-frame.html:1
> +<body></body>

Missing DOCTYPE.

> LayoutTests/resize-observer/resources/element-leak-frame.html:7
> +var ro = new ResizeObserver( entries => {

Please spell out words such as resizeObserver.

> LayoutTests/resize-observer/resources/element-leak-frame.html:8
> +    for (let entry of entries) {

No curly braces around a single line statements.

[cathiechen]

Comment on attachment 370681 [details]
Patch

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

Hi Ryosuke,

Thanks for the review:)

>> Source/WebCore/ChangeLog:21
>> +        * bindings/js/JSResizeObserverEntryCustom.cpp: Copied from Source/WebCore/page/ResizeObserverEntry.idl.
> 
> This cpp file isn't a copy of idl file??

Done, changed to "Added".

>> Source/WebCore/page/ResizeObserver.cpp:-177
>> -    m_activeObservations.clear();
> 
> Please clarify why we're no longer clearing m_observations.clear and  m_activeObservations here.

We will clear m_observations and m_activeObservations in removeAllTargets() which is invoked by disconnect().

>> LayoutTests/resize-observer/element-leak.html:13
>> +    testRunner.dumpAsText();
> 
> No need to call dumpAsText if you're including testharness.js / testharnessreport.js

Done

>> LayoutTests/resize-observer/element-leak.html:23
>> +                clearInterval(handle);
> 
> Seems like this test always requires internals?
> In that case, it seems like we don't even need to use testharness.js.

Hmm, looks like we need testharness.js to report the result after changing it to promise_test.

>> LayoutTests/resize-observer/element-leak.html:32
>> +    let test = async_test('test0: Test elements leak');
> 
> Can we use promise_test instead?

Done.

>> LayoutTests/resize-observer/element-leak.html:55
>> +    }, 5000);
> 
> Just specify timeout to promise_test instead.

Done

>> LayoutTests/resize-observer/element-leak.html:62
>> +test0();
> 
> Ugh... can we give this more descriptive name like runTest?

Sorry, done!

>> LayoutTests/resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-alive.html:46
>> +        document.querySelectorAll('div').forEach(target => { target.remove(); });
> 
> No need for { ~; }. You can just write `target => target.remove()` here.

Done

>> LayoutTests/resize-observer/resize-observer-keeps-js-wrapper-of-target-alive.html:33
>> +    document.querySelectorAll('div').forEach(target => { target.remove(); });
> 
> Ditto.

Done

>> LayoutTests/resize-observer/resize-observer-keeps-js-wrapper-of-target-alive.html:41
>> +    document.querySelectorAll('div').forEach(target => { observer.observe(target); });
> 
> Ditto.

Done

>> LayoutTests/resize-observer/resources/element-leak-frame.html:1
>> +<body></body>
> 
> Missing DOCTYPE.

Done

>> LayoutTests/resize-observer/resources/element-leak-frame.html:7
>> +var ro = new ResizeObserver( entries => {
> 
> Please spell out words such as resizeObserver.

Done

>> LayoutTests/resize-observer/resources/element-leak-frame.html:8
>> +    for (let entry of entries) {
> 
> No curly braces around a single line statements.

Done

[cathiechen]

Created attachment 371183 [details]
Patch

[EWS Watchlist]

Comment on attachment 371183 [details]
Patch

Attachment 371183 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12362270

New failing tests:
storage/indexeddb/index-cursor.html

[EWS Watchlist]

Created attachment 371186 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[cathiechen]

Created attachment 371241 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 371241 [details]
Patch

Clearing flags on attachment: 371241

Committed r246057: <https://trac.webkit.org/changeset/246057>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51387195>