202932 – Web Inspector: inspector/model/remote-object-weak-collection.html is failing

RESOLVED FIXED 202932

Web Inspector: inspector/model/remote-object-weak-collection.html is failing

https://bugs.webkit.org/show_bug.cgi?id=202932

Summary Web Inspector: inspector/model/remote-object-weak-collection.html is failing

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	Details Formatted Diff Diff
function call that makes the test fail (1.50 KB, patch) 2019-10-14 11:42 PDT, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Patch (2.93 KB, patch) 2019-11-08 22:41 PST, Devin Rousso	no flags	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Devin Rousso

Reported

2019-10-14 09:34 PDT

Modified

2019-11-13 11:37 PST History

CC List

9 users Show

URL

Keywords InRadar

Duplicates (1)

204016 View as bug list

Depends on

Blocks