Closed Bug 1364075 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ochameau, Assigned: jdescottes)

References

Details

Attachments

(1 file)

Bug 1364075 - remove DevTools dependency in ContentProcessSingleton;
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
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
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c9106dd622416ccef4e01cd98c64bc3832feae8
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
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)
(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
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.
Attachment #8872465 - Flags: review?(poirot.alex)
(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.
(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.
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+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e9b45f7ed37
remove DevTools dependency in ContentProcessSingleton;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/6e9b45f7ed37
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: