Bug 194527

Summary: Add two regression tests for reference cycle in IndexedDB
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: Tools / TestsAssignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, commit-queue, darin, ews-watchlist, ggaren, jsbell, keith_miller, lforschler, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews204 for win-future
none
Patch
none
Patch for landing commit-queue: commit-queue-

Description Sihui Liu 2019-02-11 18:19:05 PST
Inspired by https://bugs.webkit.org/show_bug.cgi?id=189435#c17.
Comment 1 Sihui Liu 2019-02-11 18:22:37 PST
Created attachment 361751 [details]
Patch
Comment 2 Geoffrey Garen 2019-02-11 21:16:23 PST
Comment on attachment 361751 [details]
Patch

r=me
Comment 3 EWS Watchlist 2019-02-11 21:17:22 PST
Comment on attachment 361751 [details]
Patch

Attachment 361751 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11117414

New failing tests:
storage/indexeddb/value-cursor-cycle.html
storage/indexeddb/result-request-cycle.html
storage/indexeddb/modern/gc-closes-database-private.html
Comment 4 EWS Watchlist 2019-02-11 21:17:33 PST
Created attachment 361763 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 5 Geoffrey Garen 2019-02-11 21:22:12 PST
> New failing tests:
> storage/indexeddb/value-cursor-cycle.html
> storage/indexeddb/result-request-cycle.html
> storage/indexeddb/modern/gc-closes-database-private.html

Hmmm... Seems like a problem.
Comment 6 Sihui Liu 2019-02-12 10:28:26 PST
storage/indexeddb/modern/gc-closes-database-private.html has been flaky on our win bots for a while: https://bugs.webkit.org/show_bug.cgi?id=194450. 
It sometimes times out: https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r241257%20(1355)/results.html, so this should not be caused by this patch. 

I suspect there is a bug in gc on win bots. Many gc related tests were marked as fail/skip recently on win: https://trac.webkit.org/changeset/240116/webkit.

I think we can skip added tests on win until https://bugs.webkit.org/show_bug.cgi?id=193540 is fixed.
Comment 7 Sihui Liu 2019-02-12 11:12:35 PST
Created attachment 361809 [details]
Patch
Comment 8 Geoffrey Garen 2019-02-12 18:45:40 PST
Comment on attachment 361809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361809&action=review

r=me

> LayoutTests/fast/dom/reference-cycle-leaks.html:120
> +// FIXME: Need to write this test and reorganize so it can be asynchronous.

Do we still need this FIXME?

What is "this test" in the FIXME?
Comment 9 Sihui Liu 2019-02-13 08:28:44 PST
(In reply to Geoffrey Garen from comment #8)
> Comment on attachment 361809 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361809&action=review
> 
> r=me
> 
> > LayoutTests/fast/dom/reference-cycle-leaks.html:120
> > +// FIXME: Need to write this test and reorganize so it can be asynchronous.
> 
> Do we still need this FIXME?
> 
No we don't. There is another FIXME for making the test async inside createRTCStatsReportCycle(). I will remove it.
Comment 10 Sihui Liu 2019-02-13 08:35:29 PST
Created attachment 361920 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2019-02-13 09:13:00 PST
Comment on attachment 361920 [details]
Patch for landing

Rejecting attachment 361920 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 361920, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=361920&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=194527&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 361920 from bug 194527.
Fetching: https://bugs.webkit.org/attachment.cgi?id=361920
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	LayoutTests/storage/indexeddb/resources/result-request-cycle.js
	A	LayoutTests/storage/indexeddb/resources/value-cursor-cycle.js
	A	LayoutTests/storage/indexeddb/result-request-cycle-expected.txt
	A	LayoutTests/storage/indexeddb/result-request-cycle.html
	A	LayoutTests/storage/indexeddb/value-cursor-cycle-expected.txt
	A	LayoutTests/storage/indexeddb/value-cursor-cycle.html
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/dom/reference-cycle-leaks.html
	M	LayoutTests/platform/win/TestExpectations
Committed r241436
fatal: Unable to create '/Volumes/Data/EWS/WebKit/.git/svn/refs/remotes/origin/master/index.lock': File exists.

Another git process seems to be running in this repository, e.g.
an editor opened by 'git commit'. Please make sure all processes
are terminated then try again. If it still fails, a git process
may have crashed in this repository earlier:
remove the file manually to continue.
write-tree: command returned error: 128


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	LayoutTests/storage/indexeddb/resources/result-request-cycle.js
	A	LayoutTests/storage/indexeddb/resources/value-cursor-cycle.js
	A	LayoutTests/storage/indexeddb/result-request-cycle-expected.txt
	A	LayoutTests/storage/indexeddb/result-request-cycle.html
	A	LayoutTests/storage/indexeddb/value-cursor-cycle-expected.txt
	A	LayoutTests/storage/indexeddb/value-cursor-cycle.html
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/dom/reference-cycle-leaks.html
	M	LayoutTests/platform/win/TestExpectations
Committed r241436
fatal: Unable to create '/Volumes/Data/EWS/WebKit/.git/svn/refs/remotes/origin/master/index.lock': File exists.

Another git process seems to be running in this repository, e.g.
an editor opened by 'git commit'. Please make sure all processes
are terminated then try again. If it still fails, a git process
may have crashed in this repository earlier:
remove the file manually to continue.
write-tree: command returned error: 128


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From https://git.webkit.org/git/WebKit
   e9b8ef1c6e0..01f1dc0717d  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 241433 = e9b8ef1c6e09aaff4981f13bc06c63acf5397893
r241435 = 01f1dc0717d8e4d1ab3d7f64c53f12b99c5b84d0
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Total errors found: 0 in 1 files

Full output: https://webkit-queues.webkit.org/results/11135551
Comment 12 WebKit Commit Bot 2019-02-13 13:05:31 PST
Comment on attachment 361920 [details]
Patch for landing

Rejecting attachment 361920 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 361920, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=361920&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=194527&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 361920 from bug 194527.
Fetching: https://bugs.webkit.org/attachment.cgi?id=361920
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 9 diffs from patch file(s).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/dom/reference-cycle-leaks.html
Hunk #1 FAILED at 117.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/fast/dom/reference-cycle-leaks.html.rej
patching file LayoutTests/platform/win/TestExpectations
Hunk #1 FAILED at 4269.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/win/TestExpectations.rej
patching file LayoutTests/storage/indexeddb/resources/result-request-cycle.js
patching file LayoutTests/storage/indexeddb/resources/value-cursor-cycle.js
patching file LayoutTests/storage/indexeddb/result-request-cycle-expected.txt
patching file LayoutTests/storage/indexeddb/result-request-cycle.html
patching file LayoutTests/storage/indexeddb/value-cursor-cycle-expected.txt
patching file LayoutTests/storage/indexeddb/value-cursor-cycle.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/11137454
Comment 13 Sihui Liu 2019-02-14 14:06:41 PST
This is committed: https://trac.webkit.org/changeset/241436/webkit.
Comment 14 Radar WebKit Bug Importer 2019-02-14 14:13:10 PST
<rdar://problem/48087805>
Comment 15 Darin Adler 2019-02-14 18:31:04 PST
Comment on attachment 361920 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=361920&action=review

> LayoutTests/storage/indexeddb/result-request-cycle.html:7
> +<script src="resources/result-request-cycle.js"></script>

For consideration in the future, Alexey has been discouraging us from putting HTML shells with all the JavaScript code in a separate file, as is done here. He suggests putting the entire test in one HTML file including the JavaScript code and considers this two=file structure an obsolete one that should not be used for new tests.

I’ve been following that guideline. Let us know if you don’t agree and we can debate the merits.
Comment 16 Sihui Liu 2019-02-15 09:48:36 PST
(In reply to Darin Adler from comment #15)
> Comment on attachment 361920 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361920&action=review
> 
> > LayoutTests/storage/indexeddb/result-request-cycle.html:7
> > +<script src="resources/result-request-cycle.js"></script>
> 
> For consideration in the future, Alexey has been discouraging us from
> putting HTML shells with all the JavaScript code in a separate file, as is
> done here. He suggests putting the entire test in one HTML file including
> the JavaScript code and considers this two=file structure an obsolete one
> that should not be used for new tests.
> 
> I’ve been following that guideline. Let us know if you don’t agree and we
> can debate the merits.

I agree we should do that. I was following structure of tests in LayoutTests/storage/indexeddb for consistency. Looking back on that, I found most of the tests has a "-private.html" file for doing the same test in private mode. That's why they have a separate Javascript file. 

There are also tests under this folder which don't have test in private mode but still inherit this "two=file" structure. I should add a "-private.html" test or merge test files that don't need to be tested in private mode for better reference.
Comment 17 Keith Miller 2020-04-06 08:24:08 PDT
Comment on attachment 361920 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=361920&action=review

> LayoutTests/storage/indexeddb/resources/value-cursor-cycle.js:50
> +        cursorObservation = internals.observeGC(cursor);
> +        valueObservation = internals.observeGC(value);
> +        evalAndLog("cursor = null");
> +        evalAndLog("cursorRequest = null");
> +        evalAndLog("gc()");
> +        shouldBeFalse("cursorObservation.wasCollected");
> +
> +        evalAndLog("value = null");
> +        evalAndLog("gc()");
> +        shouldBeTrue("cursorObservation.wasCollected");
> +        shouldBeTrue("valueObservation.wasCollected");

This test seems incorrect. There's no reason that cursor and value have to be collected here. Our GC is conservative so you can't assert something is collected. If you want to ensure there's no GC leak, you should write your test in the form:

let observers = [];
for (i = 0; i < 1e3; ++i)
    observers.push(internals.observeGC(createValue()));

gc();
shouldBeTrue("observers.some((o) => o.wasCollected)");
Comment 18 Sihui Liu 2020-04-06 09:44:46 PDT
(In reply to Keith Miller from comment #17)
> Comment on attachment 361920 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361920&action=review
> 
> > LayoutTests/storage/indexeddb/resources/value-cursor-cycle.js:50
> > +        cursorObservation = internals.observeGC(cursor);
> > +        valueObservation = internals.observeGC(value);
> > +        evalAndLog("cursor = null");
> > +        evalAndLog("cursorRequest = null");
> > +        evalAndLog("gc()");
> > +        shouldBeFalse("cursorObservation.wasCollected");
> > +
> > +        evalAndLog("value = null");
> > +        evalAndLog("gc()");
> > +        shouldBeTrue("cursorObservation.wasCollected");
> > +        shouldBeTrue("valueObservation.wasCollected");
> 
> This test seems incorrect. There's no reason that cursor and value have to
> be collected here. Our GC is conservative so you can't assert something is
> collected. If you want to ensure there's no GC leak, you should write your
> test in the form:
> 
> let observers = [];
> for (i = 0; i < 1e3; ++i)
>     observers.push(internals.observeGC(createValue()));
> 
> gc();
> shouldBeTrue("observers.some((o) => o.wasCollected)");

Thanks for the comment. This bug is noted (like in https://trac.webkit.org/changeset/259462). I have not modified other IDB tests as there is no reported failure/flakiness yet. To fix them all, will need to revisit all the IDB gc() tests.