Extract DevTools specifics from toolkit/components/processsingleton/ContentProcessSingleton.js

RESOLVED FIXED in Firefox 55

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: ochameau, Assigned: jdescottes)

Tracking

(Blocks 1 bug)

unspecified
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

2 years ago
ContentProcessSingleton.js currently interacts with devtools codebase.
We should refactor that and prune everything related to devtools from this components.

We could possibly fork this xpcom in order to keep devtools specifics.
And the from here:
http://searchfox.org/mozilla-central/source/devtools/server/main.js#758
we would call loadProcessScript.
We may inline content-server.jsm in it as lazy loading it wouldn't be useful anymore.
DevTools bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
Comment on attachment 8872465 [details]
Bug 1364075 - remove DevTools dependency in ContentProcessSingleton;

Switching to f?

Few things I am unsure about this:

#1: I load the process script from main.js, but it feels redundant to have to send a message, just in order to pass the "prefix" info. Is there a simpler way to send this information, a way to do it at the same time as we load the script for instance?

#2: in the ContentProcessSingleton, the message listener is removed on xpcom-shutdown. I created this new content script based on service-worker-child.js which doesn't take care of removing the message listener. I could still add an observer on xpcom-shutdown and remove the listener if it's necessary.

Thanks.
Attachment #8872465 - Flags: review?(poirot.alex) → feedback?(poirot.alex)
Reporter

Comment 6

2 years ago
(In reply to Julian Descottes [:jdescottes] from comment #5)
> #1: I load the process script from main.js, but it feels redundant to have
> to send a message, just in order to pass the "prefix" info. Is there a
> simpler way to send this information, a way to do it at the same time as we
> load the script for instance?

No, you can't send data while loading a script, but that is fine having two calls.
It is only aesthetical...

> #2: in the ContentProcessSingleton, the message listener is removed on
> xpcom-shutdown. I created this new content script based on
> service-worker-child.js which doesn't take care of removing the message
> listener. I could still add an observer on xpcom-shutdown and remove the
> listener if it's necessary.

It isn't clear how important such removal is.
I imagine message listener will be removed anyway.
But I would see more value in unregistering the process script on devtools unload
via Services.ppmm.removeDelayedProcessScript[1] and another message to unregister the message listener.

[1] you can use Services.ppmm to simplify this
Reporter

Comment 7

2 years ago
mozreview-review
Comment on attachment 8872465 [details]
Bug 1364075 - remove DevTools dependency in ContentProcessSingleton;

https://reviewboard.mozilla.org/r/143968/#review147786

::: devtools/server/content-process-debugger-server.js:15
(Diff revision 2)
> +const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
> +
> +// Only reply if we are in a real content process
> +if (Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT) {
> +  let {init} = Cu.import("resource://devtools/server/content-server.jsm", {});
> +  init(message);

You are missing the addMessageListener here.

::: devtools/server/main.js:759
(Diff revision 2)
>  
>        deferred.resolve(actor);
>      });
>  
> -    mm.sendAsyncMessage("DevTools:InitDebuggerServer", {
> +    // Load the process script that will receive the debug:init-content-server message
> +    mm.loadProcessScript(

You should call loadProcessScript only once to prevent loading it multiple times.
Here, every time you open a browser content toolbox, a new script will be evaluated.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8872465 - Flags: review?(poirot.alex)
Assignee

Comment 9

2 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #6)
> (In reply to Julian Descottes [:jdescottes] from comment #5)
> 
> > #2: in the ContentProcessSingleton, the message listener is removed on
> > xpcom-shutdown. I created this new content script based on
> > service-worker-child.js which doesn't take care of removing the message
> > listener. I could still add an observer on xpcom-shutdown and remove the
> > listener if it's necessary.
> 
> It isn't clear how important such removal is.
> I imagine message listener will be removed anyway.
> But I would see more value in unregistering the process script on devtools
> unload
> via Services.ppmm.removeDelayedProcessScript[1] and another message to
> unregister the message listener.
> 
> [1] you can use Services.ppmm to simplify this

Thanks! Still some questions.

Here Services.ppmm is the GPPMM and calling removeDelayedProcessScript avoids loading the script in new content processes when they spawn. But I used a child PPMM to load the script, so I think it will only be loaded in the content process linked to this PPMM? I thought it was preferable to only load it in the content processes targeted by a browser content toolbox.

Should I rather load this script "globally" using the GPPMM? And remove it on destroy() etc...
Another try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c972ee71786cef4378c296a19f2d13a8d6703766 
Loading the script from the GPPMM and removing it on devtools shutdown.
Comment hidden (mozreview-request)
Reporter

Comment 12

2 years ago
(In reply to Julian Descottes [:jdescottes] from comment #9)
> Thanks! Still some questions.
> 
> Here Services.ppmm is the GPPMM and calling removeDelayedProcessScript
> avoids loading the script in new content processes when they spawn. But I
> used a child PPMM to load the script, so I think it will only be loaded in
> the content process linked to this PPMM? I thought it was preferable to only
> load it in the content processes targeted by a browser content toolbox.

True, I missed that. Yes it would be ideal to load only against the targeted process.

> Should I rather load this script "globally" using the GPPMM? And remove it
> on destroy() etc...

Yes, I think it will be easier. We have to handle devtools unload properly,
otherwise it may break when devtools add-on is going to be reloaded :/
So proper support of destroy/cleanup is more important than targetting only the right process.
Reporter

Comment 13

2 years ago
mozreview-review
Comment on attachment 8872465 [details]
Bug 1364075 - remove DevTools dependency in ContentProcessSingleton;

https://reviewboard.mozilla.org/r/143968/#review148246

Looks good, thanks!
Attachment #8872465 - Flags: review?(poirot.alex) → review+

Comment 14

2 years ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e9b45f7ed37
remove DevTools dependency in ContentProcessSingleton;r=ochameau

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e9b45f7ed37
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.