Bug 202932

Summary: Web Inspector: inspector/model/remote-object-weak-collection.html is failing
Product: WebKit Reporter: Yury Semikhatsky <yurys>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, bburg, commit-queue, hi, inspector-bugzilla-changes, joepeck, rniwa, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=204162
Attachments:
Description Flags
a weird workaround
none
function call that makes the test fail
none
Patch
none
Archive of layout-test-results from webkit-cq-03
none
Archive of layout-test-results from webkit-cq-01
none
Archive of layout-test-results from webkit-cq-03
none
Archive of layout-test-results from webkit-cq-03
none
Archive of layout-test-results from webkit-cq-03
none
Patch none

Yury Semikhatsky
Reported 2019-10-14 09:34:16 PDT
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);"},
Attachments
a weird workaround (1.33 KB, patch)
2019-10-14 11:27 PDT, Yury Semikhatsky 		no flags
DetailsFormatted DiffDiff
function call that makes the test fail (1.50 KB, patch)
2019-10-14 11:42 PDT, Yury Semikhatsky 		no flags
DetailsFormatted DiffDiff
Patch (2.93 KB, patch)
2019-11-08 22:41 PST, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from webkit-cq-03 (3.57 MB, application/zip)
2019-11-11 10:00 PST, WebKit Commit Bot 		no flags
Details
Archive of layout-test-results from webkit-cq-01 (3.68 MB, application/zip)
2019-11-11 17:34 PST, WebKit Commit Bot 		no flags
Details
Archive of layout-test-results from webkit-cq-03 (3.98 MB, application/zip)
2019-11-12 17:07 PST, WebKit Commit Bot 		no flags
Details
Archive of layout-test-results from webkit-cq-03 (3.44 MB, application/zip)
2019-11-12 22:51 PST, WebKit Commit Bot 		no flags
Details
Archive of layout-test-results from webkit-cq-03 (3.45 MB, application/zip)
2019-11-13 02:20 PST, WebKit Commit Bot 		no flags
Details
Patch (2.36 KB, patch)
2019-11-13 10:17 PST, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1 2019-10-14 11:01:19 PDT
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.
Yury Semikhatsky
Comment 2 2019-10-14 11:26:49 PDT
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).
Yury Semikhatsky
Comment 3 2019-10-14 11:27:11 PDT
Created attachment 380901 [details] a weird workaround
Yury Semikhatsky
Comment 4 2019-10-14 11:34:45 PDT
(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.
Yury Semikhatsky
Comment 5 2019-10-14 11:41:50 PDT
Inserting a side-effect 'free' recursive function before GCController.collect() call in the test makes it fail reliably.
Yury Semikhatsky
Comment 6 2019-10-14 11:42:18 PDT
Created attachment 380904 [details] function call that makes the test fail
Aakash Jain
Comment 7 2019-11-08 11:52:07 PST
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
Yury Semikhatsky
Comment 8 2019-11-08 16:59:00 PST
Ryosuke, is there a reliable way to make GC remove all orphan keys from a WeakMap in a layout test?
Devin Rousso
Comment 9 2019-11-08 22:41:14 PST
Created attachment 383203 [details] Patch
Alexey Proskuryakov
Comment 10 2019-11-10 14:10:48 PST
*** Bug 204016 has been marked as a duplicate of this bug. ***
WebKit Commit Bot
Comment 11 2019-11-11 10:00:31 PST
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
WebKit Commit Bot
Comment 12 2019-11-11 10:00:32 PST
Created attachment 383279 [details] Archive of layout-test-results from webkit-cq-03
Yury Semikhatsky
Comment 13 2019-11-11 10:01:18 PST
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?
WebKit Commit Bot
Comment 14 2019-11-11 17:34:29 PST
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
WebKit Commit Bot
Comment 15 2019-11-11 17:34:30 PST
Created attachment 383327 [details] Archive of layout-test-results from webkit-cq-01
WebKit Commit Bot
Comment 16 2019-11-12 17:07:33 PST
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
WebKit Commit Bot
Comment 17 2019-11-12 17:07:34 PST
Created attachment 383409 [details] Archive of layout-test-results from webkit-cq-03
Devin Rousso
Comment 18 2019-11-12 17:21:32 PST
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.
Yury Semikhatsky
Comment 19 2019-11-12 18:27:53 PST
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?
WebKit Commit Bot
Comment 20 2019-11-12 22:51:15 PST
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
WebKit Commit Bot
Comment 21 2019-11-12 22:51:16 PST
Created attachment 383434 [details] Archive of layout-test-results from webkit-cq-03
WebKit Commit Bot
Comment 22 2019-11-13 02:20:47 PST
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
WebKit Commit Bot
Comment 23 2019-11-13 02:20:48 PST
Created attachment 383441 [details] Archive of layout-test-results from webkit-cq-03
Devin Rousso
Comment 24 2019-11-13 10:17:39 PST
Created attachment 383467 [details] Patch
Devin Rousso
Comment 25 2019-11-13 10:22:39 PST
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> 👍
Blaze Burg
Comment 26 2019-11-13 10:25:03 PST
Comment on attachment 383467 [details] Patch r=mews
Yury Semikhatsky
Comment 27 2019-11-13 11:33:15 PST
(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?
WebKit Commit Bot
Comment 28 2019-11-13 11:36:47 PST
Comment on attachment 383467 [details] Patch Clearing flags on attachment: 383467 Committed r252420: <https://trac.webkit.org/changeset/252420>
WebKit Commit Bot
Comment 29 2019-11-13 11:36:49 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 30 2019-11-13 11:37:46 PST
<rdar://problem/57161698>
Note You need to log in before you can comment on or make changes to this bug.