Bug 197908 – resize-observer/element-leak.html fails on Windows platform

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[cathiechen]

Created attachment 369932 [details]
test cases

The removed node couldn't be released after testFrame.remove();

[cathiechen]

Created attachment 369933 [details]
Test case

[cathiechen]

Created attachment 369948 [details]
test case

[EWS Watchlist]

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

[EWS Watchlist]

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

[cathiechen]

resize-observer/element-leak-002.html

The elements seem couldn't be released after it removed in win platform.

[Fujii Hironori]

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

[cathiechen]

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. :)

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ryosuke Niwa]

I don't get what this bug title is about.

Is this about adding tests for resize observers to not leak observed elements?

[cathiechen]

> 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.

[Fujii Hironori]

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

[cathiechen]

(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.

[Ryosuke Niwa]

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.

[cathiechen]

Created attachment 371578 [details]
test cases

[cathiechen]

Created attachment 371595 [details]
Patch

[cathiechen]

(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.

[cathiechen]

Created attachment 371597 [details]
Patch

[Ryosuke Niwa]

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.

[cathiechen]

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.

[cathiechen]

Created attachment 371645 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Comment on attachment 371645 [details]
Patch

As requested by cchen.

[cathiechen]

(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.

[WebKit Commit Bot]

Comment on attachment 371645 [details]
Patch

Clearing flags on attachment: 371645

Committed r246232: <https://trac.webkit.org/changeset/246232>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51548967>

[cathiechen]

Hi Jesse,

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