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.
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.
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
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
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
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
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
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
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
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
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
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
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
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.
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.
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
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
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
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
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
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
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
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
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
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
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 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 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() == ⌖
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 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 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 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
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
2019-05-01 08:07 PDT, cathiechen
2019-05-08 10:12 PDT, cathiechen
2019-05-08 15:01 PDT, EWS Watchlist
2019-05-12 11:36 PDT, cathiechen
2019-05-12 12:48 PDT, EWS Watchlist
2019-05-12 13:37 PDT, EWS Watchlist
2019-05-13 09:24 PDT, cathiechen
2019-05-13 10:35 PDT, EWS Watchlist
2019-05-13 11:26 PDT, EWS Watchlist
2019-05-13 11:32 PDT, EWS Watchlist
2019-05-13 11:41 PDT, EWS Watchlist
2019-05-13 12:42 PDT, EWS Watchlist
2019-05-14 03:39 PDT, cathiechen
2019-05-14 05:37 PDT, cathiechen
2019-05-14 06:41 PDT, cathiechen
2019-05-14 07:39 PDT, EWS Watchlist
2019-05-14 07:44 PDT, EWS Watchlist
2019-05-14 07:49 PDT, EWS Watchlist
2019-05-14 08:01 PDT, cathiechen
2019-05-14 08:15 PDT, cathiechen
2019-05-14 08:53 PDT, cathiechen
2019-05-14 09:32 PDT, EWS Watchlist
2019-05-14 09:49 PDT, cathiechen
2019-05-14 10:45 PDT, EWS Watchlist
2019-05-14 10:59 PDT, EWS Watchlist
2019-05-14 11:40 PDT, EWS Watchlist
2019-05-14 11:45 PDT, EWS Watchlist
2019-05-14 21:03 PDT, cathiechen
2019-05-14 22:16 PDT, EWS Watchlist
2019-05-14 22:30 PDT, EWS Watchlist
2019-05-14 23:05 PDT, EWS Watchlist
2019-05-14 23:06 PDT, EWS Watchlist
2019-05-14 23:12 PDT, EWS Watchlist
2019-05-15 00:49 PDT, cathiechen
2019-05-15 02:19 PDT, EWS Watchlist
2019-05-17 02:42 PDT, cathiechen
2019-05-27 02:17 PDT, cathiechen
2019-06-03 02:11 PDT, cathiechen
2019-06-03 06:09 PDT, EWS Watchlist
2019-06-03 20:39 PDT, cathiechen