Bug 197457 - The JS wrapper of target in an ResizeObserverEntry should not get collected
Summary: The JS wrapper of target in an ResizeObserverEntry should not get collected
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL:
Keywords: InRadar
Depends on: 197908
Blocks: 157743
  Show dependency treegraph
 
Reported: 2019-05-01 07:23 PDT by cathiechen
Modified: 2019-06-07 10:50 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.16 KB, patch)
2019-05-01 08:07 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (18.89 KB, patch)
2019-05-08 10:12 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (13.53 MB, application/zip)
2019-05-08 15:01 PDT, Build Bot
no flags Details
Patch (22.22 KB, patch)
2019-05-12 11:36 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (3.05 MB, application/zip)
2019-05-12 12:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews214 for win-future (13.35 MB, application/zip)
2019-05-12 13:37 PDT, Build Bot
no flags Details
Patch (22.89 KB, patch)
2019-05-13 09:24 PDT, cathiechen
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.97 MB, application/zip)
2019-05-13 10:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.92 MB, application/zip)
2019-05-13 11:26 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (7.88 MB, application/zip)
2019-05-13 11:32 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-highsierra (3.18 MB, application/zip)
2019-05-13 11:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews210 for win-future (13.57 MB, application/zip)
2019-05-13 12:42 PDT, Build Bot
no flags Details
with logs (23.37 KB, patch)
2019-05-14 03:39 PDT, cathiechen
no flags Details | Formatted Diff | Diff
To test wincairo build error (23.38 KB, patch)
2019-05-14 05:37 PDT, cathiechen
no flags Details | Formatted Diff | Diff
WIP (23.38 KB, patch)
2019-05-14 06:41 PDT, cathiechen
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (754.33 KB, application/zip)
2019-05-14 07:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (523.74 KB, application/zip)
2019-05-14 07:44 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (509.34 KB, application/zip)
2019-05-14 07:49 PDT, Build Bot
no flags Details
WIP (22.74 KB, patch)
2019-05-14 08:01 PDT, cathiechen
no flags Details | Formatted Diff | Diff
WIP (22.72 KB, patch)
2019-05-14 08:15 PDT, cathiechen
no flags Details | Formatted Diff | Diff
WIP (20.60 KB, patch)
2019-05-14 08:53 PDT, cathiechen
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (1.26 MB, application/zip)
2019-05-14 09:32 PDT, Build Bot
no flags Details
WIP (20.18 KB, patch)
2019-05-14 09:49 PDT, cathiechen
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.48 MB, application/zip)
2019-05-14 10:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.70 MB, application/zip)
2019-05-14 10:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (7.94 MB, application/zip)
2019-05-14 11:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (3.30 MB, application/zip)
2019-05-14 11:45 PDT, Build Bot
no flags Details
WIP (28.10 KB, patch)
2019-05-14 21:03 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.39 MB, application/zip)
2019-05-14 22:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.83 MB, application/zip)
2019-05-14 22:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews214 for win-future (13.85 MB, application/zip)
2019-05-14 23:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.16 MB, application/zip)
2019-05-14 23:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.77 MB, application/zip)
2019-05-14 23:12 PDT, Build Bot
no flags Details
Patch (22.75 KB, patch)
2019-05-15 00:49 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.37 MB, application/zip)
2019-05-15 02:19 PDT, Build Bot
no flags Details
Patch (23.83 KB, patch)
2019-05-17 02:42 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (20.46 KB, patch)
2019-05-27 02:17 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (19.93 KB, patch)
2019-06-03 02:11 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.60 MB, application/zip)
2019-06-03 06:09 PDT, Build Bot
no flags Details
Patch (19.85 KB, patch)
2019-06-03 20:39 PDT, cathiechen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cathiechen 2019-05-01 07:23:50 PDT
The JS wrappers of targets in ResizeObserverEntry shouldn't be collected. Use GCReachableRef to make it reachable for GC.
Comment 1 cathiechen 2019-05-01 08:07:54 PDT
Created attachment 368670 [details]
Patch
Comment 2 Ryosuke Niwa 2019-05-01 10:26:13 PDT
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.
Comment 3 cathiechen 2019-05-08 10:12:23 PDT
Created attachment 369394 [details]
Patch
Comment 4 cathiechen 2019-05-08 10:28:37 PDT
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.
Comment 5 Build Bot 2019-05-08 15:01:25 PDT
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
Comment 6 Build Bot 2019-05-08 15:01:27 PDT
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
Comment 7 cathiechen 2019-05-12 11:36:07 PDT
Created attachment 369680 [details]
Patch
Comment 8 Build Bot 2019-05-12 12:48:24 PDT
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
Comment 9 Build Bot 2019-05-12 12:48:26 PDT
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
Comment 10 Build Bot 2019-05-12 13:37:53 PDT
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
Comment 11 Build Bot 2019-05-12 13:37:56 PDT
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
Comment 12 cathiechen 2019-05-13 09:24:23 PDT
Created attachment 369738 [details]
Patch
Comment 13 Build Bot 2019-05-13 10:35:05 PDT
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
Comment 14 Build Bot 2019-05-13 10:35:07 PDT
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
Comment 15 Build Bot 2019-05-13 11:26:39 PDT
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
Comment 16 Build Bot 2019-05-13 11:26:41 PDT
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
Comment 17 Build Bot 2019-05-13 11:32:09 PDT
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
Comment 18 Build Bot 2019-05-13 11:32:11 PDT
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
Comment 19 Build Bot 2019-05-13 11:41:43 PDT
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
Comment 20 Build Bot 2019-05-13 11:41:45 PDT
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
Comment 21 Build Bot 2019-05-13 12:42:44 PDT
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
Comment 22 Build Bot 2019-05-13 12:42:47 PDT
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
Comment 23 cathiechen 2019-05-14 03:39:10 PDT
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.
Comment 24 cathiechen 2019-05-14 05:37:38 PDT
Created attachment 369837 [details]
To test wincairo build error
Comment 25 cathiechen 2019-05-14 06:41:38 PDT
Created attachment 369840 [details]
WIP
Comment 26 Build Bot 2019-05-14 07:39:46 PDT
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.
Comment 27 Build Bot 2019-05-14 07:39:48 PDT
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
Comment 28 Build Bot 2019-05-14 07:44:23 PDT
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.
Comment 29 Build Bot 2019-05-14 07:44:25 PDT
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
Comment 30 Build Bot 2019-05-14 07:49:28 PDT
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.
Comment 31 Build Bot 2019-05-14 07:49:30 PDT
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
Comment 32 cathiechen 2019-05-14 08:01:19 PDT
Created attachment 369847 [details]
WIP
Comment 33 Build Bot 2019-05-14 08:05:39 PDT
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.
Comment 34 cathiechen 2019-05-14 08:15:35 PDT
Created attachment 369848 [details]
WIP
Comment 35 Build Bot 2019-05-14 08:19:04 PDT
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.
Comment 36 cathiechen 2019-05-14 08:53:57 PDT
Created attachment 369854 [details]
WIP
Comment 37 Build Bot 2019-05-14 09:32:42 PDT
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.
Comment 38 Build Bot 2019-05-14 09:32:45 PDT
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
Comment 39 cathiechen 2019-05-14 09:49:12 PDT
Created attachment 369862 [details]
WIP
Comment 40 Build Bot 2019-05-14 10:45:24 PDT
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
Comment 41 Build Bot 2019-05-14 10:45:26 PDT
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
Comment 42 Build Bot 2019-05-14 10:59:30 PDT
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
Comment 43 Build Bot 2019-05-14 10:59:32 PDT
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
Comment 44 Build Bot 2019-05-14 11:40:40 PDT
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
Comment 45 Build Bot 2019-05-14 11:40:45 PDT
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
Comment 46 Build Bot 2019-05-14 11:45:01 PDT
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
Comment 47 Build Bot 2019-05-14 11:45:05 PDT
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
Comment 48 cathiechen 2019-05-14 21:03:38 PDT
Created attachment 369921 [details]
WIP
Comment 49 Build Bot 2019-05-14 22:16:38 PDT
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
Comment 50 Build Bot 2019-05-14 22:16:41 PDT
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
Comment 51 Build Bot 2019-05-14 22:30:26 PDT
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
Comment 52 Build Bot 2019-05-14 22:30:29 PDT
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
Comment 53 Build Bot 2019-05-14 23:05:12 PDT
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
Comment 54 Build Bot 2019-05-14 23:05:15 PDT
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
Comment 55 Build Bot 2019-05-14 23:06:37 PDT
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
Comment 56 Build Bot 2019-05-14 23:06:40 PDT
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
Comment 57 Build Bot 2019-05-14 23:12:33 PDT
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
Comment 58 Build Bot 2019-05-14 23:12:36 PDT
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
Comment 59 cathiechen 2019-05-15 00:49:37 PDT
Created attachment 369935 [details]
Patch
Comment 60 Build Bot 2019-05-15 02:19:19 PDT
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
Comment 61 Build Bot 2019-05-15 02:19:21 PDT
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
Comment 62 cathiechen 2019-05-15 09:42:29 PDT
The failure in win seems not specific to ResizeObserver. I opened a new bug for it.
Comment 63 cathiechen 2019-05-15 09:45:14 PDT
(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
Comment 64 cathiechen 2019-05-17 02:42:40 PDT
Created attachment 370111 [details]
Patch
Comment 65 cathiechen 2019-05-18 01:14:07 PDT
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:)
Comment 66 Ryosuke Niwa 2019-05-20 15:21:32 PDT
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?
Comment 67 cathiechen 2019-05-27 01:55:50 PDT
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.
Comment 68 cathiechen 2019-05-27 02:17:49 PDT
Created attachment 370681 [details]
Patch
Comment 69 Ryosuke Niwa 2019-05-28 16:38:20 PDT
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.
Comment 70 cathiechen 2019-06-03 02:03:32 PDT
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
Comment 71 cathiechen 2019-06-03 02:11:08 PDT
Created attachment 371183 [details]
Patch
Comment 72 Build Bot 2019-06-03 06:09:13 PDT
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
Comment 73 Build Bot 2019-06-03 06:09:17 PDT
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
Comment 74 cathiechen 2019-06-03 20:39:31 PDT
Created attachment 371241 [details]
Patch
Comment 75 WebKit Commit Bot 2019-06-04 00:38:22 PDT
Comment on attachment 371241 [details]
Patch

Clearing flags on attachment: 371241

Committed r246057: <https://trac.webkit.org/changeset/246057>
Comment 76 WebKit Commit Bot 2019-06-04 00:38:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 77 Radar WebKit Bug Importer 2019-06-04 00:39:54 PDT
<rdar://problem/51387195>