1709672 - ResourceCommand's watchResources and unwatchResource still have race conditions

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

Bug 1708635 highlighted a race condition that still exists between watchResources and unwatchResources:
https://treeherder.mozilla.org/logviewer?job_id=338677989&repo=autoland&lineNumber=6500

[task 2021-05-04T14:50:19.411Z] 14:50:19     INFO - TEST-PASS | devtools/client/responsive/test/browser/browser_device_change.js | UA should be set to Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0 - 
[task 2021-05-04T14:50:19.414Z] 14:50:19     INFO - Closing responsive design mode
[task 2021-05-04T14:50:19.700Z] 14:50:19     INFO - GECKO(1513) | console.error: "Exception while calling a ResourceCommand" "available" "callback" ":" (new Error("Stopped listening for resource 'document-event' that isn't being listened to", "resource://devtools

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1709672 - [devtools] Start listening to all resources in parallel.

This isn't solely for performance reason.
This will help ensure calling _startListening synchronously when calling watchResources.
This is to help simplify the various states store to track which resources are being watched.
And ultimately fix races between watch and unwatch.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1709672 - [devtools] Do not await on synchronous _stopListening method.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1709672 - [devtools] Memoize StorageActor.listStores result.

Calling it twice in a raw makes it never resolve.
The actor implementation could probably be made more resilient,
but memoizing it on the front would probably help reduce the number of request we are doing.

This issues appears with the first changeset, parellizing the call to ResourceCommand._startListening.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1709672 - [devtools] Move the call to watchAllTargets.

This is the ultimate change in order to ensure calling _startListening
synchronously from watchResources.

Fixes are required to styles tests, as watchAllTarget is only called if we pass a valid resource type.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1709672 - [devtools] Simplify how we track currently watched resources.

Now that _startListening is synchronous, we can fix the race conditions
that exists between many calls made to watch and unwatch resources.
And we would benefit from simplifying the various ways to track
what resources is being watched.
We no either use _watchers and _pendindWatchers to have the live number
of registered listeners. And in parellel to that, only record
which resources is being watched on the server via _listenedResources.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e47322fa2d09
[devtools] Start listening to all resources in parallel. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/accb5722566d
[devtools] Do not await on synchronous _stopListening method. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/a239123a371a
[devtools] Memoize StorageActor.listStores result. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/9f6b42ce7f5d
[devtools] Move the call to watchAllTargets. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/54ac35a2ef29
[devtools] Simplify how we track currently watched resources. r=nchevobbe

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/e47322fa2d09
https://hg.mozilla.org/mozilla-central/rev/accb5722566d
https://hg.mozilla.org/mozilla-central/rev/a239123a371a
https://hg.mozilla.org/mozilla-central/rev/9f6b42ce7f5d
https://hg.mozilla.org/mozilla-central/rev/54ac35a2ef29