Bug 202932 - Web Inspector: inspector/model/remote-object-weak-collection.html is failing
Summary: Web Inspector: inspector/model/remote-object-weak-collection.html is failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
: 204016 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-10-14 09:34 PDT by Yury Semikhatsky
Modified: 2019-11-13 11:37 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 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);"},
Comment 1 Joseph Pecoraro 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.
Comment 2 Yury Semikhatsky 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).
Comment 3 Yury Semikhatsky 2019-10-14 11:27:11 PDT
Created attachment 380901 [details]
a weird workaround
Comment 4 Yury Semikhatsky 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.
Comment 5 Yury Semikhatsky 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.
Comment 6 Yury Semikhatsky 2019-10-14 11:42:18 PDT
Created attachment 380904 [details]
function call that makes the test fail
Comment 7 Aakash Jain 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
Comment 8 Yury Semikhatsky 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?
Comment 9 Devin Rousso 2019-11-08 22:41:14 PST
Created attachment 383203 [details]
Patch
Comment 10 Alexey Proskuryakov 2019-11-10 14:10:48 PST
*** Bug 204016 has been marked as a duplicate of this bug. ***
Comment 11 WebKit Commit Bot 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
Comment 12 WebKit Commit Bot 2019-11-11 10:00:32 PST
Created attachment 383279 [details]
Archive of layout-test-results from webkit-cq-03
Comment 13 Yury Semikhatsky 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?
Comment 14 WebKit Commit Bot 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
Comment 15 WebKit Commit Bot 2019-11-11 17:34:30 PST
Created attachment 383327 [details]
Archive of layout-test-results from webkit-cq-01
Comment 16 WebKit Commit Bot 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
Comment 17 WebKit Commit Bot 2019-11-12 17:07:34 PST
Created attachment 383409 [details]
Archive of layout-test-results from webkit-cq-03
Comment 18 Devin Rousso 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.
Comment 19 Yury Semikhatsky 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?
Comment 20 WebKit Commit Bot 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
Comment 21 WebKit Commit Bot 2019-11-12 22:51:16 PST
Created attachment 383434 [details]
Archive of layout-test-results from webkit-cq-03
Comment 22 WebKit Commit Bot 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
Comment 23 WebKit Commit Bot 2019-11-13 02:20:48 PST
Created attachment 383441 [details]
Archive of layout-test-results from webkit-cq-03
Comment 24 Devin Rousso 2019-11-13 10:17:39 PST
Created attachment 383467 [details]
Patch
Comment 25 Devin Rousso 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> 👍
Comment 26 Brian Burg 2019-11-13 10:25:03 PST
Comment on attachment 383467 [details]
Patch

r=mews
Comment 27 Yury Semikhatsky 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?
Comment 28 WebKit Commit Bot 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>
Comment 29 WebKit Commit Bot 2019-11-13 11:36:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Radar WebKit Bug Importer 2019-11-13 11:37:46 PST
<rdar://problem/57161698>