Bug 196956 – Web Inspector: use weak collections for holding event listeners

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

The current `WI.Object` can create reference cycles when it saves `thisObject` via `addEventListener`.  We should use a WeakMap/WeakSet to hold these objects (and maybe even the listener function) so that they can be GC'd.

[Devin Rousso]

Created attachment 367502 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 367517 [details]
[Patch] WIP

[EWS Watchlist]

Comment on attachment 367517 [details]
[Patch] WIP

Attachment 367517 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11883731

New failing tests:
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
inspector/unit-tests/event-listener-set.html
inspector/unit-tests/event-listener.html

[EWS Watchlist]

Created attachment 367523 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 367517 [details]
[Patch] WIP

Attachment 367517 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11884067

New failing tests:
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
inspector/unit-tests/event-listener-set.html
inspector/unit-tests/event-listener.html

[EWS Watchlist]

Created attachment 367527 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 367517 [details]
[Patch] WIP

Attachment 367517 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11889201

New failing tests:
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
inspector/unit-tests/event-listener-set.html
inspector/unit-tests/event-listener.html

[EWS Watchlist]

Created attachment 367568 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 372431 [details]
[Patch] WIP

[EWS Watchlist]

Attachment 372431 [details] did not pass style-queue:


ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:2337:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:2338:  Path does not exist.  [test/expectations] [5]
Total errors found: 2 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Comment on attachment 372431 [details]
[Patch] WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=372431&action=review

> Source/WebInspectorUI/UserInterface/Base/Object.js:37
> +        console.assert(eventType, "Object.addEventListener: invalid eventType ", eventType, "(listener: ", listener, " thisObject: ", thisObject, ")");

Part of me also want's to add an `assert` that `eventType` exists in a `WI.*.Event` object that's related to `this` (e.g. `Object.values(this.prototype.constructor.Event).includes(eventType)`), but I'm not sure how worth it that would be, as it can basically be caught in style review instead.

> Source/WebInspectorUI/UserInterface/Base/Object.js:45
> +        if (window.InspectorTest && !thisObject)

I'd like to remove this in the future (e.g. make all tests provide a `thisObject`), but I think this is fine for now, as it's basically guaranteed to keep the listener alive.

I'm also not against the idea of allowing this for non-tests too, as it would eliminate much of the diff in this patch :P

> Source/WebInspectorUI/UserInterface/Base/Object.js:67
> +        console.assert(!listenersForThisObject.has(listener), "Object.addEventListener: duplicate listener ", listener, "(eventType: ", eventType, " thisObject ", thisObject, ")");

I added this extra `assert` so we can find places where we're doing unnecessary work :)

> Source/WebInspectorUI/UserInterface/Base/Object.js:90
> +            this._listeners.forEach((thisObjectsForEventType) => {

NIT: this doesn't need to be an arrow function.

[Devin Rousso]

Created attachment 372436 [details]
[Patch] WIP

[EWS Watchlist]

Comment on attachment 372436 [details]
[Patch] WIP

Attachment 372436 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12517473

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 372441 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 372477 [details]
[Patch] WIP

Add overflow check.

[EWS Watchlist]

Comment on attachment 372477 [details]
[Patch] WIP

Attachment 372477 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12522807

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-no-ftl
apiTests

[Devin Rousso]

Created attachment 372530 [details]
Patch

[Devin Rousso]

Comment on attachment 372530 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372530&action=review

> Source/WebInspectorUI/UserInterface/Base/Object.js:-160
> -            let listeners = listenersTable.toArray();

The only downside I can see to using a `WeakMap` is that it no longer preserves the order if there's more than one `thisObject`.  Since `listener`s are stored in a `Set` (per `thisObject`), the order of execution (per `thisObject`) will match the order of insertion.  If I had the following scenario:

Object A adds Listener 1
Object B adds Listener 2
Object A adds Listener 3

Then the new order of execution would be [A1, A3, B2], which wouldn't match the "true" execution order.  I think this is probably fine, however, as we likely don't have that many (if any) cases where the same object adds multiple different listeners to the same target, and for any where that's the case we should probably remove them.

[Joseph Pecoraro]

There seems to be work on a WeakRef type in Ecmascript. That might be better than this.

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #19)
> There seems to be work on a WeakRef type in Ecmascript. That might be better than this.
If we use attachment 372530 [details] as is, the I don't think so actually.  All it would entail is replacing the `WeakMap` with a `Map` where the keys are `WeakRef`s.  The same issues with order preservation and anonymous functions being used as the `listener` without a `thisObject` still apply, albeit in a slightly different form.

With that having been said, we could instead just use `WeakRef` in place of the `thisObject` with the existing `ListMultimap`, but frankly I'd like to simplify that system anyways.

Another option would be to ditch the concept of a `Map` altogether and just use an array of `{thisObject: WeakRef(...), listener}` items.  Calls to `addEventListener` and `removeEventListener` are "rare" (when compared to how often the `listener` itself is fired/used) that I think we can "afford" being less efficient there in order to preserve the order of addition.

[Joseph Pecoraro]

Comment on attachment 372530 [details]
Patch

I don't like the idea of only Web Inspector having custom JS to iterate WeakMap/Set. I'd rather we wait for something like WeakRef. I'm going to r- but let me know if you disagree and I can take another look.

[Joseph Pecoraro]

Seems we can implement our own InspectorIterableWeakMap / InspectorIterableWeakSet using WeakRef?

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #22)
> Seems we can implement our own InspectorIterableWeakMap / InspectorIterableWeakSet using WeakRef?
That's basically my plan.  I'm thinking of moving back to an array of `{listener, thisObject}` and removing most of the `removeEventListener(null, null, this)` that wouldn't be needed anymore.

[Nikita Vasilyev]

Comment on attachment 372530 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372530&action=review

> Source/WebInspectorUI/ChangeLog:8
> +        Replace the `ListMultimap` with a `WeakMap` and the `LinkedList` with a `Set` for listeners.

Note that 3-4 years ago when, iterating a linked list was significantly faster than Set.

[Nikita Vasilyev]

Removing, adding, and triggering event listeners is a performance sensitive part of Web Inspector. It's worth measuring how long it takes to remove, add, fire >1000 events.

[Devin Rousso]

Created attachment 376656 [details]
Patch

[EWS Watchlist]

Comment on attachment 376656 [details]
Patch

Attachment 376656 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12938272

New failing tests:
inspector/unit-tests/resource-collection.html
inspector/script-profiler/event-type-Other.html
inspector/runtime/promise-native-getter.html
inspector/unit-tests/protocol-test-dispatch-event-to-frontend.html
inspector/unit-tests/event-listener.html

[EWS Watchlist]

Created attachment 376661 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 376656 [details]
Patch

Attachment 376656 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12938309

New failing tests:
inspector/unit-tests/resource-collection.html
inspector/script-profiler/event-type-Other.html
inspector/runtime/promise-native-getter.html
inspector/unit-tests/protocol-test-dispatch-event-to-frontend.html
inspector/unit-tests/event-listener.html

[EWS Watchlist]

Created attachment 376662 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Blaze Burg]

Comment on attachment 376656 [details]
Patch

Clearing r?, patch is quite stale at this point.

[Devin Rousso]

Created attachment 411024 [details]
Patch

`WeakRef` and `FinalizationRegistry` were enabled in r268284

[Devin Rousso]

Created attachment 411030 [details]
Patch

[Blaze Burg]

Comment on attachment 411030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411030&action=review

r=me, great work!

> Source/WebInspectorUI/ChangeLog:265
> +        Remove the `closed` method that only `removeEventListener` as they are no longer needed

Nit: grammaro

> Source/WebInspectorUI/UserInterface/Base/Object.js:60
> +        let wrappedCallback = (...args) => {

Nice to get rid of unneeded `arguments`!

> Source/WebInspectorUI/UserInterface/Base/Object.js:124
>              // Make a copy with slice so mutations during the loop doesn't affect us.

Nit: unnecessary comment.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:566
> +        let promises = [this.awaitEvent(WI.DebuggerManager.Event.Paused, this)];

Nice!

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:38
> +}, WI.settings.engineeringPauseForInternalScripts);

LOL, oops

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1037
> +                this.singleFireEventListener(WI.Resource.Event.LoadingDidFinish, resolve, this);

Nice.

> Source/WebInspectorUI/UserInterface/Views/AuditTreeElement.js:100
> +        if (this.representedObject instanceof WI.AuditTestBase) {

Cranky logic like this is why I wrote EventListenerSet in the first place.

> Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:493
> +            this.dispatchEventToListeners(DataGridNode.Event.Populate);

O_O

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:-108
> -        WI.HeapSnapshotWorkerProxy.singleton().addEventListener("HeapSnapshot.CollectionEvent", this._heapSnapshotCollectionEvent, this);

o_O

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:-264
> -        WI.HeapSnapshotWorkerProxy.singleton().removeEventListener("HeapSnapshot.CollectionEvent", this._heapSnapshotCollectionEvent, this);

It seems like we lost this removeEventListener.

[Devin Rousso]

Comment on attachment 411030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411030&action=review

>> Source/WebInspectorUI/UserInterface/Views/AuditTreeElement.js:100
>> +        if (this.representedObject instanceof WI.AuditTestBase) {
> 
> Cranky logic like this is why I wrote EventListenerSet in the first place.

Yeah I can see the appeal.  IMO this happens so infrequently that it's not worth another data structure.

If anything, I'd rather loosen the restrictions around `removeEventListener` so that you can call it even when nothing is registered which would allow us to drop all the `if`.  I do see a benefit to having an assertion that the registration exists, but if others feel strongly I can change it (and this).

>> Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:493
>> +            this.dispatchEventToListeners(DataGridNode.Event.Populate);
> 
> O_O

ikr

>> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:-264
>> -        WI.HeapSnapshotWorkerProxy.singleton().removeEventListener("HeapSnapshot.CollectionEvent", this._heapSnapshotCollectionEvent, this);
> 
> It seems like we lost this removeEventListener.

This should just disappear when the `WI.HeapAllocationsTimelineView` is destructed, which should only ever happen if the user hides the heap allocations timeline.

EDIT: after discussing this offline with you, I've decided to undo my removal of all the `removeEventListener` in `closed` throughout this patch in the hopes of not introducing subtle regressions and keeping behavior as close to what it was before

[Devin Rousso]

Created attachment 413134 [details]
Patch

streamline changes by
 - not eliminating `closed` and instead just replacing any `removeEventListener(null, null, this)` therein
 - not adding `closed` to handle `WI.DataGrid`, etc.

[Devin Rousso]

Created attachment 413142 [details]
Patch

[EWS]

Committed r269359: <https://trac.webkit.org/changeset/269359>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413142 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71031192>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/269359/webkit

broke inspector/unit-tests/heap-snapshot-collection-event.html

History:
https://results.webkit.org/?suite=layout-tests&test=inspector%2Funit-tests%2Fheap-snapshot-collection-event.html

Diff:
--- /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/inspector/unit-tests/heap-snapshot-collection-event-expected.txt
+++ /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/inspector/unit-tests/heap-snapshot-collection-event-actual.txt
@@ -8,9 +8,7 @@
 
 -- Running test case: HeapSnapshotCollectionEvent
 PASS: Should not have an error creating a snapshot.
-PASS: Received HeapSnapshot.CollectionEvent.
-PASS: Collection should include at least 200 nodes (100 objects and 100 strings).
-PASS: Collection should affect the first snapshot.
+!! TIMEOUT: took longer than 10000ms
 
 -- Running test case: HeapSnapshot.prototype.update
 PASS: 0 Objects were dead before.

[Devin Rousso]

Created attachment 413341 [details]
[Patch] followup

[Joseph Pecoraro]

Comment on attachment 413341 [details]
[Patch] followup

rs=me

[Keith Miller]

Comment on attachment 413341 [details]
[Patch] followup

FWIW, it seems wrong that this test is expecting every object to be collected. That's a recipe for a flaky test, you should probably only test that at least one object was collected.

[Ryan Haddad]

Comment on attachment 413341 [details]
[Patch] followup

Clearing flags on attachment: 413341

Committed r269532: <https://trac.webkit.org/changeset/269532>

[Ryan Haddad]

All reviewed patches have been landed.  Closing bug.