Open Bug 1500000 Opened 3 years ago Updated 6 months ago

Remove DebuggerClient.release method

Categories

(DevTools :: Console, task, P3)

task

Tracking

(Fission Milestone:Future)

Fission Milestone Future

People

(Reporter: ochameau, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: dt-fission-future)

Attachments

(2 obsolete files)

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.
Priority: -- → P2
Whiteboard: [boogaloo-mvp]
(I'd be happy to move this bug to another bug entry if anyone wants to celebrate here ;))
with that bug id this bug looks so... intense.
Yeah, this report should be used for a party (not for work :-)

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

Honza
(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)
(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
Whiteboard: [boogaloo-mvp] → dt-fission
Whiteboard: dt-fission → [boogaloo-mvp]
Whiteboard: [boogaloo-mvp]

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.

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.

Depends on: 1529916
Depends on: 1529917
Attachment #9044629 - Attachment is obsolete: true
Attachment #9044630 - Attachment is obsolete: true
Whiteboard: dt-fission
Priority: P2 → P3
Whiteboard: dt-fission → dt-fission-reserve

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.

Depends on: 1599432, 1579090
Summary: Move DebuggerClient.release to WebConsoleClient → Remove DebuggerClient.release method

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

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

Fission Milestone: --- → MVP
Type: enhancement → task

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

Fission Milestone: MVP → Future
Whiteboard: dt-fission-reserve → dt-fission-future
You need to log in before you can comment on or make changes to this bug.