Bug 197908 - resize-observer/element-leak.html fails on Windows platform
Summary: resize-observer/element-leak.html fails on Windows platform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL:
Keywords: InRadar
Depends on:
Blocks: 197457
  Show dependency treegraph
 
Reported: 2019-05-15 00:30 PDT by cathiechen
Modified: 2019-06-09 22:01 PDT (History)
13 users (show)

See Also:


Attachments
test cases (3.05 KB, patch)
2019-05-15 00:30 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Test case (3.10 KB, patch)
2019-05-15 00:35 PDT, cathiechen
no flags Details | Formatted Diff | Diff
test case (3.10 KB, patch)
2019-05-15 07:00 PDT, cathiechen
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.43 MB, application/zip)
2019-05-15 08:46 PDT, Build Bot
no flags Details
test cases (9.92 KB, patch)
2019-05-16 07:56 PDT, cathiechen
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (13.46 MB, application/zip)
2019-05-16 09:47 PDT, Build Bot
no flags Details
test cases (22.03 KB, patch)
2019-06-07 02:49 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (5.68 KB, patch)
2019-06-07 10:33 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (5.75 KB, patch)
2019-06-07 10:47 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2019-06-07 20:53 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.61 MB, application/zip)
2019-06-08 02:58 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description cathiechen 2019-05-15 00:30:26 PDT
Created attachment 369932 [details]
test cases

The removed node couldn't be released after testFrame.remove();
Comment 1 cathiechen 2019-05-15 00:35:15 PDT
Created attachment 369933 [details]
Test case
Comment 2 cathiechen 2019-05-15 07:00:46 PDT
Created attachment 369948 [details]
test case
Comment 3 Build Bot 2019-05-15 08:46:18 PDT
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
Comment 4 Build Bot 2019-05-15 08:46:21 PDT
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
Comment 5 cathiechen 2019-05-15 09:44:35 PDT
resize-observer/element-leak-002.html

The elements seem couldn't be released after it removed in win platform.
Comment 6 Fujii Hironori 2019-05-16 04:00:37 PDT
I tested with WinCairo DRT and WTR on my PC, and confirmed your test case passing.
Comment 7 cathiechen 2019-05-16 07:56:25 PDT
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 8 Build Bot 2019-05-16 09:47:45 PDT
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
Comment 9 Build Bot 2019-05-16 09:47:47 PDT
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
Comment 10 Ryosuke Niwa 2019-05-20 15:09:22 PDT
I don't get what this bug title is about.

Is this about adding tests for resize observers to not leak observed elements?
Comment 11 cathiechen 2019-05-27 02:46:59 PDT
> 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.
Comment 12 Fujii Hironori 2019-05-28 02:23:56 PDT
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
Comment 13 cathiechen 2019-05-28 04:05:30 PDT
(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.
Comment 14 Ryosuke Niwa 2019-05-28 16:41:13 PDT
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.
Comment 15 cathiechen 2019-06-07 02:49:28 PDT
Created attachment 371578 [details]
test cases
Comment 16 cathiechen 2019-06-07 10:33:18 PDT
Created attachment 371595 [details]
Patch
Comment 17 cathiechen 2019-06-07 10:40:51 PDT
(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.
Comment 18 cathiechen 2019-06-07 10:47:35 PDT
Created attachment 371597 [details]
Patch
Comment 19 Ryosuke Niwa 2019-06-07 15:48:07 PDT
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 20 cathiechen 2019-06-07 20:50:43 PDT
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.
Comment 21 cathiechen 2019-06-07 20:53:52 PDT
Created attachment 371645 [details]
Patch
Comment 22 Build Bot 2019-06-08 02:58:40 PDT
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
Comment 23 Build Bot 2019-06-08 02:58:45 PDT
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 24 Rob Buis 2019-06-08 03:02:31 PDT
Comment on attachment 371645 [details]
Patch

As requested by cchen.
Comment 25 cathiechen 2019-06-08 03:07:34 PDT
(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 26 WebKit Commit Bot 2019-06-08 03:32:13 PDT
Comment on attachment 371645 [details]
Patch

Clearing flags on attachment: 371645

Committed r246232: <https://trac.webkit.org/changeset/246232>
Comment 27 WebKit Commit Bot 2019-06-08 03:32:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Radar WebKit Bug Importer 2019-06-08 03:33:27 PDT
<rdar://problem/51548967>
Comment 30 cathiechen 2019-06-08 14:15:54 PDT
Hi Jesse,

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