The test is reliably failing on all platforms with the following diff: --- /home/yurys/WebKit/WebKitBuild/Release/layout-test-results/inspector/model/remote-object-weak-collection-expected.txt +++ /home/yurys/WebKit/WebKitBuild/Release/layout-test-results/inspector/model/remote-object-weak-collection-actual.txt @@ -129,5 +129,32 @@ ----------------------------------------------------- EXPRESSION: delete window.strongKey2; weakMap ENTRIES: -[] +[ + { + "_key": { + "_type": "object", + "_objectId": "<filtered>", + "_description": "Object", + "_preview": { + "_type": "object", + "_description": "Object", + "_lossless": true, + "_overflow": false, + "_properties": [ + { + "_name": "id", + "_type": "number", + "_value": "2" + } + ], + "_entries": null + } + }, + "_value": { + "_type": "number", + "_description": "2", + "_value": 2 + } + } +] Looks like a real issue as the entry is not being GC'ed after the key is deleted. It has something to do with the WeakMap capacity I believe as the test stops failing if one extra entry is inserted: - {expression: "weakMap.set({id:3}, 3); weakMap.set({id:4}, 4);"}, + {expression: "weakMap.set({id:3}, 3); weakMap.set({id:4}, 4); weakMap.set({id:5}, 5);"},
Is this a new failure? Given we use a conservative GC, maybe we get unlucky and something appears to reference the value; but you do mention this is reliably failing.
We might just be unlucky but It looks more like a genuine failure to me, the test fails on every run. Also adding one more element to the map with a key that doesn't have any strong references makes GC collect the unrelated entry (see attached patch).
Created attachment 380901 [details] a weird workaround
(In reply to Yury Semikhatsky from comment #3) > Created attachment 380901 [details] > a weird workaround Ok, running 1k iterations with this change also leads to intermittent failures in the same place, so I believe you are right and it may be a result of the conservative GC wrongly marking some objects as alive.
Inserting a side-effect 'free' recursive function before GCController.collect() call in the test makes it fail reliably.
Created attachment 380904 [details] function call that makes the test fail
Can we prioritize this, or update the expectations? This is causing false-positive on commit-queue and slowing down EWS. e.g.: https://bugs.webkit.org/show_bug.cgi?id=204012#c3 Flakiness dashboard: https://results.webkit.org/?suite=layout-tests&test=inspector%2Fmodel%2Fremote-object-weak-collection.html
Ryosuke, is there a reliable way to make GC remove all orphan keys from a WeakMap in a layout test?
Created attachment 383203 [details] Patch
*** Bug 204016 has been marked as a duplicate of this bug. ***
The commit-queue just saw inspector/model/remote-object-weak-collection.html flake (text diff) while processing attachment 383252 [details] on bug 203493. Bot: webkit-cq-03 Port: <class 'webkitpy.common.config.ports.MacPort'> Platform: Mac OS X 10.13.6
Created attachment 383279 [details] Archive of layout-test-results from webkit-cq-03
Comment on attachment 383203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383203&action=review > LayoutTests/inspector/model/remote-object-weak-collection.html:7 > +window.strongKey1 = {id:1}; why did this change? > LayoutTests/inspector/model/remote-object-weak-collection.html:-57 > - WI.runtimeManager.evaluateInInspectedWindow(step.expression, {objectGroup: "test", doNotPauseOnExceptionsAndMuteConsole: true, generatePreview: true}, function(remoteObject, wasThrown) { Does it mean that in normal mode when the preview does get generated for the UI it will still create strong references and disrupt memory management in the page? It'd be much better if releasing the 'test' object group would also discard all preview artifacts. Is there a reason it cannot be fixed?
The commit-queue just saw inspector/model/remote-object-weak-collection.html flake (text diff) while processing attachment 383289 [details] on bug 176674. Bot: webkit-cq-01 Port: <class 'webkitpy.common.config.ports.MacPort'> Platform: Mac OS X 10.13.6
Created attachment 383327 [details] Archive of layout-test-results from webkit-cq-01
The commit-queue just saw inspector/model/remote-object-weak-collection.html flake (text diff) while processing attachment 383331 [details] on bug 204099. Bot: webkit-cq-03 Port: <class 'webkitpy.common.config.ports.MacPort'> Platform: Mac OS X 10.13.6
Created attachment 383409 [details] Archive of layout-test-results from webkit-cq-03
Comment on attachment 383203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383203&action=review >> LayoutTests/inspector/model/remote-object-weak-collection.html:7 >> +window.strongKey1 = {id:1}; > > why did this change? No particular reason, other than that I think this reads better (it's clearer). >> LayoutTests/inspector/model/remote-object-weak-collection.html:-57 >> - WI.runtimeManager.evaluateInInspectedWindow(step.expression, {objectGroup: "test", doNotPauseOnExceptionsAndMuteConsole: true, generatePreview: true}, function(remoteObject, wasThrown) { > > Does it mean that in normal mode when the preview does get generated for the UI it will still create strong references and disrupt memory management in the page? > > It'd be much better if releasing the 'test' object group would also discard all preview artifacts. Is there a reason it cannot be fixed? I'm not sure exactly. Testing this manually in a browser (e.g. loading this file and then evaluating each of the items in `steps` via the Console) and then forcing a GC (click the icon that looks like a recycle symbol) didn't reproduce the issue. It may be something odd about the test harness or even this test itself. For the sake of getting this passing again on the bots, I think this is OK. I tried running it myself locally with 1000 iterations and it didn't fail once.
Comment on attachment 383203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383203&action=review Changing 'generatePreview' to false sounds as a reasonable fix to me to unflake the test. >>> LayoutTests/inspector/model/remote-object-weak-collection.html:7 >>> +window.strongKey1 = {id:1}; >> >> why did this change? > > No particular reason, other than that I think this reads better (it's clearer). I'd say that it's just more verbose and hence less readable. It's a matter of personal preferences though, but given that it has nothing to do with the failure I'd revert this to keep 'git blame' of the file more meaningful. >>> LayoutTests/inspector/model/remote-object-weak-collection.html:-57 >>> - WI.runtimeManager.evaluateInInspectedWindow(step.expression, {objectGroup: "test", doNotPauseOnExceptionsAndMuteConsole: true, generatePreview: true}, function(remoteObject, wasThrown) { >> >> Does it mean that in normal mode when the preview does get generated for the UI it will still create strong references and disrupt memory management in the page? >> >> It'd be much better if releasing the 'test' object group would also discard all preview artifacts. Is there a reason it cannot be fixed? > > I'm not sure exactly. Testing this manually in a browser (e.g. loading this file and then evaluating each of the items in `steps` via the Console) and then forcing a GC (click the icon that looks like a recycle symbol) didn't reproduce the issue. It may be something odd about the test harness or even this test itself. > > For the sake of getting this passing again on the bots, I think this is OK. I tried running it myself locally with 1000 iterations and it didn't fail once. This should be easy to figure out looking at heap snapshot, have you tried that? I agree that unflaking the test is high priority so I'm ok with landing the fix with 'generatePreview: false'. It'd be nice to understand the root cause though, can you file a separate bug for that?
The commit-queue just saw inspector/model/remote-object-weak-collection.html flake (text diff) while processing attachment 383425 [details] on bug 204139. Bot: webkit-cq-03 Port: <class 'webkitpy.common.config.ports.MacPort'> Platform: Mac OS X 10.13.6
Created attachment 383434 [details] Archive of layout-test-results from webkit-cq-03
The commit-queue just saw inspector/model/remote-object-weak-collection.html flake (text diff) while processing attachment 383436 [details] on bug 201461. Bot: webkit-cq-03 Port: <class 'webkitpy.common.config.ports.MacPort'> Platform: Mac OS X 10.13.6
Created attachment 383441 [details] Archive of layout-test-results from webkit-cq-03
Created attachment 383467 [details] Patch
Comment on attachment 383203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383203&action=review >>>> LayoutTests/inspector/model/remote-object-weak-collection.html:-57 >>>> - WI.runtimeManager.evaluateInInspectedWindow(step.expression, {objectGroup: "test", doNotPauseOnExceptionsAndMuteConsole: true, generatePreview: true}, function(remoteObject, wasThrown) { >>> >>> Does it mean that in normal mode when the preview does get generated for the UI it will still create strong references and disrupt memory management in the page? >>> >>> It'd be much better if releasing the 'test' object group would also discard all preview artifacts. Is there a reason it cannot be fixed? >> >> I'm not sure exactly. Testing this manually in a browser (e.g. loading this file and then evaluating each of the items in `steps` via the Console) and then forcing a GC (click the icon that looks like a recycle symbol) didn't reproduce the issue. It may be something odd about the test harness or even this test itself. >> >> For the sake of getting this passing again on the bots, I think this is OK. I tried running it myself locally with 1000 iterations and it didn't fail once. > > This should be easy to figure out looking at heap snapshot, have you tried that? > > I agree that unflaking the test is high priority so I'm ok with landing the fix with 'generatePreview: false'. It'd be nice to understand the root cause though, can you file a separate bug for that? I tried using a heap snapshot, but like I said it didn't reproduce at all if I open the test file in a browser, inspect it, and then copypaste each `expression` in `steps` into the Console. The heap snapshot didn't tell me anything other than that the object didn't exist anymore, which is what I expected given that evaluating `weakMap` in the Console didn't show it either. <https://webkit.org/b/204162> 👍
Comment on attachment 383467 [details] Patch r=mews
(In reply to Devin Rousso from comment #25) > I tried using a heap snapshot, but like I said it didn't reproduce at all if > I open the test file in a browser, inspect it, and then copypaste each > `expression` in `steps` into the Console. Why not take a heap snapshot right in the test where it is failing?
Comment on attachment 383467 [details] Patch Clearing flags on attachment: 383467 Committed r252420: <https://trac.webkit.org/changeset/252420>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57161698>