1500000 - Remove DebuggerClient.release method

Status: RESOLVED FIXED

(DevTools :: Console, task, P3)

Description

[Alexandre Poirot [:ochameau]]

DebuggerClient host a "release" method that is only used by the WebConsoleActor:
https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/devtools/shared/client/debugger-client.js#484-493

https://searchfox.org/mozilla-central/search?q=client.release(&case=false&regexp=false&path=
It seems to always be used to release a value grip actor.

While switching all client to fronts, we will no longer manipulate actor IDs and instead always call requests through their front instance.

Today this release method expects one argument, the actor ID to release.
A first step in the process of refactoring WebConsoleClient to a front could be to expose a release method on the client and ensure that all the callsites switch from:
  DebuggerClient.release(actorID);
to:
  WebConsoleClient.release();

It may not be so trivial as the callsites seem to currently pass around the actorID only and not necessarily the client.

Comment 1

[Alexandre Poirot [:ochameau]]

(I'd be happy to move this bug to another bug entry if anyone wants to celebrate here ;))

Comment 2

[Yulia Startsev [:yulia]]

with that bug id this bug looks so... intense.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Yeah, this report should be used for a party (not for work :-)

@Alex: How much did you pay for the #?

Honza

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> @Alex: How much did you pay for the #?

11 years to create 991 bugs (https://bugzilla.mozilla.org/user_profile?user_id=283262)

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> (In reply to Jan Honza Odvarko [:Honza] from comment #3)
> > @Alex: How much did you pay for the #?
> 
> 11 years to create 991 bugs
> (https://bugzilla.mozilla.org/user_profile?user_id=283262)
You are awesome!

Honza

Comment 6

[Yulia Startsev [:yulia]]

Attachment: Bug 1500000 - use release from WebConsoleClient; r=ochameau

Comment 7

[Yulia Startsev [:yulia]]

Attachment: Bug 1500000 - remove release from debugger client; r=ochameau

Comment 8

[Alexandre Poirot [:ochameau]]

I'm sorry if I've been misleading in the bug comment, but this change:

• doesn't help switching to protocol JS types: we still pass around actor ID,
• we still emit a request from the console front, in the name of of another actor.

This comment was confusing:

  WebConsoleClient.release();

I actually meant to have a release method on all clients that have to be released.
And it is not clear if it is only used for ObjectActor, or if it also used from other type of actors. May be network events?
Unfortunately, doing that would most likely lead to modifying many things in the callsites (a significant one would be reps) in order to pass around the client/front instead of the actor ID. But it would be great to have a sense of how many modifications are required.

Note that protocol.js is having a special release flag on specs, which will force calling destroy method after calling the release one:
https://searchfox.org/mozilla-central/source/devtools/shared/specs/animation.js#35
https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/server/actors/animation.js#127-131
https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/shared/protocol.js#1205-1212

And some other fronts are using yet another pattern: one way "finalize" methods:
https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/shared/specs/highlighters.js#64-66
https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/server/actors/highlighters.js#559
https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/server/actors/highlighters.js#506

Finally, I should mention the RFC I opened: https://github.com/firefox-devtools/rfcs/issues/58
It highlights that a few panels are using actor IDs instead of passing around clients/fronts. I've not been about to make progress on that topic, but we may want to ease such practice or at very least acknowledge it. The main issue is that it seems to go against protocol.js fronts and having instances of things on the client side.

Comment 9

[Yulia Startsev [:yulia]]

Thanks for the clarification. I didn't understand the initial description correctly.

Taking another look at the codebase, we have three distinct panels that are using debuggerClient.release --> Debugger, Scratchpad, and Webconsole.

Debugger

For the debugger it appears to exclusively be used from reps: https://searchfox.org/mozilla-central/search?q=releaseActor&case=false&regexp=false&path=debugger

reps

reps usage of release is limited to the objectInspector:

https://searchfox.org/mozilla-central/source/devtools/client/debugger/new/packages/devtools-reps/src/object-inspector/actions.js#100-105

We set actors in two locations:

https://searchfox.org/mozilla-central/source/devtools/client/debugger/new/packages/devtools-reps/src/object-inspector/reducer.js#38-47,53-65

If we trace the NODE_PROPERTIES_LOADED event to where it is being set as the actor value, we come to this point:

We are operating on rdpGrips: https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/client/debugger/new/packages/devtools-reps/src/object-inspector/utils/node.js#54-68

It is called, for example, from nodeExpand: https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/client/shared/components/reps/reps.js#7026

Which sets its actor from this function: https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/client/shared/components/reps/reps.js#2969-2973

This is fortunately the only usage of getActor in the devtools-reps codebase. https://searchfox.org/mozilla-central/search?q=getActor(&case=false&regexp=false&path=devtools-reps

We can just replace it with getValue

For the GETTER_INVOKED event, it is easier, we just need to pass the object client from here:

https://searchfox.org/mozilla-central/source/devtools/client/debugger/new/packages/devtools-reps/src/object-inspector/actions.js#107-131

It looks like we are not even setting the actor id string there.

Webconsole

Within the webconsole -- ignoring reps, we have a collection of removed actors held in state:

https://searchfox.org/mozilla-central/search?q=removedActors&case=false&regexp=false&path=webconsole

We could modify the following to create an array of actors rather than actor strings: https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/client/webconsole/reducers/messages.js#563-583

Then modify this line to release the actors: https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/client/webconsole/enhancers/actor-releaser.js#53

We have a releaseObject method that does not seem to be used anywhere as well:

https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/client/webconsole/webconsole-ui.js#314-318

Scratchpad

Scratchpad seems to use it exclusively from the variablesview: https://searchfox.org/mozilla-central/search?q=releaseActor&case=false&regexp=false&path=scratchpad

VariableView

https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/client/shared/widgets/VariablesViewController.jsm#573

Like in other places, it appears that this collection only exists in order to release the actors.

These seem like large enough changes that each of these should have their own bug.

I think this is everything. Let me know if I missed something. I will break this into independent bugs.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Renaming the bug so it makes more sense (I think).

Scratchpad is no more
the VariablesViewController will be removed as part of Bug 1591874 since it's not used anywhere (I think)
WebConsole won't be using DebuggerClient.release as part of Bug 1579090
I think the last consumer will be the inspector extension, which will be handled by Bug 1599432.

Comment 11

[Chris Peterson [:cpeterson]]

dt-fission-reserve bugs do not need to block Fission Nightly (M6).

Comment 12

[Chris Peterson [:cpeterson]]

Tracking dt-fission-reserve bugs for Fission MVP until we know whether they really need to block Fission or not.

Comment 13

[Chris Peterson [:cpeterson]]

Moving old "dt-fission-reserve" bugs to "dt-fission-future" because they don't block Fission MVP.

Comment 14

[thuanphuoc]

this bug looks so... intense.

Comment 15

[Oliver VInce]

spam

Comment 16

[nathanlnes]

spam

Comment 17

[markt novo]

spam

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Lock this bug because of spam

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1500000 - [devtools] Remove now-unused DevToolsClient.release method.

This is no longer used, not even in tests.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b35009782736
[devtools] Remove now-unused DevToolsClient.release method. r=devtools-reviewers,nchevobbe

Comment 21

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/b35009782736