Bug 197908

Created attachment 369933 [details] Test case

Created attachment 369948 [details] test case

Comment on attachment 369948 [details] test case Attachment 369948 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12197612 New failing tests: resize-observer/element-leak-002.html

Created attachment 369954 [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

resize-observer/element-leak-002.html The elements seem couldn't be released after it removed in win platform.

I tested with WinCairo DRT and WTR on my PC, and confirmed your test case passing.

Created attachment 370042 [details] test cases Sorry, I don't have Win env, couldn't confirm this locally. Let me upload some variants and see. :)

Comment on attachment 370042 [details] test cases Attachment 370042 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12207551 New failing tests: resize-observer/element-leak-001.html storage/indexeddb/modern/gc-closes-database.html resize-observer/element-leak.html resize-observer/element-leak-002.html

Created attachment 370045 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

I don't get what this bug title is about. Is this about adding tests for resize observers to not leak observed elements?

> I don't get what this bug title is about. > Is this about adding tests for resize observers to not leak observed elements? Sorry for the confusing description. Yes, element-leak.html tests resize observers leaking. But I mean the removed nodes seem not be released properly in Win platform. And this is not specific to ResizeObserver targets. So I added LayoutTests/resize-observer/element-leak-002.html to confirm it. Maybe I should relocate it to some GC related path? Sorry again for my poor expression.

I compiled AppleWin port Debug build to debug an other issue today. I checked this issue too, but all your tests were passing on my PC. https://gist.github.com/fujii/e42ceb00ffa23be349672e6dbf5196ea

(In reply to Fujii Hironori from comment #12) > I compiled AppleWin port Debug build to debug an other issue today. > I checked this issue too, but all your tests were passing on my PC. > https://gist.github.com/fujii/e42ceb00ffa23be349672e6dbf5196ea Hi Fujii, Thanks a lot! :) Hmmm, this is strange. I could only find one slight difference. Test configuration: <win10, x86, debug> Test configuration: <win10, x86, release> Not sure would debug/release effect this.

Hm... what if we created 100 iframes? Do they all leak? Or do some of them get collected? FWIW, we have conservative GC so it's possible for some objects to be kept alive when even it should be dead.

Created attachment 371578 [details] test cases

Created attachment 371595 [details] Patch

(In reply to Ryosuke Niwa from comment #14) > Hm... what if we created 100 iframes? Do they all leak? Or do some of them > get collected? FWIW, we have conservative GC so it's possible for some > objects to be kept alive when even it should be dead. Yes, thanks. The case could detect document release after changed to multi iframes. I created 20 iframes, because 100 iframes always cause tese timeout.

Created attachment 371597 [details] Patch

Comment on attachment 371597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371597&action=review r=me assuming you've verified that the test still catches the original GC bug (you can confirm this by reverting your code change temporarily and make sure the test fails) > LayoutTests/ChangeLog:3 > + Make element-leak.html tests 20 iframes Les go back to the original title. That describes more of the symptom. Telling what change we're making doesn't explain why, and we generally prefer why over what comments. > LayoutTests/resize-observer/element-leak.html:18 > + let ifrm = document.createElement("iframe"); Please spell out iframe. > LayoutTests/resize-observer/element-leak.html:20 > + ifrm.setAttribute("src", "resources/element-leak-frame.html"); You can use iframe.src = ~ instead. > LayoutTests/resize-observer/element-leak.html:25 > + let notifyTimer = setTimeout(() => reject("Waiting Notified timed out."), 10000); There is no need to have a separate timer like this. testharness.js has a timeout of its own. If any, pass an option to promise_test(~) to adjust the timeout.

Comment on attachment 371597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371597&action=review Hi Ryosuke, Thanks for the swifter review :) > r=me assuming you've verified that the test still catches the original GC bug (you can confirm this by reverting your code change temporarily and make sure the test fails) Thanks for the advice. I tested it in my local evn. The test could detect the GC bug if I use `GCReachableRef<Element> m_target` in ResizeObserverEntry.h, and it'll get a timeout. >> LayoutTests/ChangeLog:3 >> + Make element-leak.html tests 20 iframes > > Les go back to the original title. That describes more of the symptom. > Telling what change we're making doesn't explain why, and we generally prefer why over what comments. Done. Thanks! >> LayoutTests/resize-observer/element-leak.html:18 >> + let ifrm = document.createElement("iframe"); > > Please spell out iframe. Done. >> LayoutTests/resize-observer/element-leak.html:20 >> + ifrm.setAttribute("src", "resources/element-leak-frame.html"); > > You can use iframe.src = ~ instead. Done. >> LayoutTests/resize-observer/element-leak.html:25 >> + let notifyTimer = setTimeout(() => reject("Waiting Notified timed out."), 10000); > > There is no need to have a separate timer like this. testharness.js has a timeout of its own. > If any, pass an option to promise_test(~) to adjust the timeout. Done. Removed the timers, it seems the default timeout is long enough to test 20 iframes. Then we wouldn't directly know which part is timeout by test result.

Created attachment 371645 [details] Patch

Comment on attachment 371645 [details] Patch Attachment 371645 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12416744 New failing tests: imported/blink/fast/canvas/bug382588.html

Created attachment 371650 [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 on attachment 371645 [details] Patch As requested by cchen.

(In reply to Rob Buis from comment #24) > Comment on attachment 371645 [details] > Patch > > As requested by cchen. Thanks, Rob! PS: The failure in imported/blink/fast/canvas/bug382588.html is not related to this patch.

Comment on attachment 371645 [details] Patch Clearing flags on attachment: 371645 Committed r246232: <https://trac.webkit.org/changeset/246232>

All reviewed patches have been landed. Closing bug.

<rdar://problem/51548967>

Hi Jesse, I couldn't see your words in coment29. Would you mind send them again? Thanks:)