Bug 194527 – Add two regression tests for reference cycle in IndexedDB

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Sihui Liu]

Inspired by https://bugs.webkit.org/show_bug.cgi?id=189435#c17.

[Sihui Liu]

Created attachment 361751 [details]
Patch

[Geoffrey Garen]

Comment on attachment 361751 [details]
Patch

r=me

[EWS Watchlist]

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

[EWS Watchlist]

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

[Geoffrey Garen]

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

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 361809 [details]
Patch

[Geoffrey Garen]

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?

[Sihui Liu]

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

[Sihui Liu]

Created attachment 361920 [details]
Patch for landing

[WebKit Commit Bot]

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 [e-mail]...
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

[WebKit Commit Bot]

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 [e-mail]...
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

[Sihui Liu]

This is committed: https://trac.webkit.org/changeset/241436/webkit.

[Radar WebKit Bug Importer]

<rdar://problem/48087805>

[Darin Adler]

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.

[Sihui Liu]

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

[Keith Miller]

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)");

[Sihui Liu]

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