Closed Bug 1280267 Opened 5 years ago Closed 5 years ago

Make the browser content toolbox support multiple content processes

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: mrbkap, Assigned: ochameau)

References

Details

(Whiteboard: [e10s-multi:M1])

Attachments

(3 files, 2 obsolete files)

Currently, the browser content toolbox doesn't have any UI to choose which content processes to debug. Alex says that this should be relatively easy to add and that the backend already supports this.
Severity: normal → enhancement
Attached patch patch v1 (obsolete) — Splinter Review
Somewhat easy. The hard part is the xul menus...
Gabor, blake, could you confirm me it works fine for you?

This patch should show n items in the "Browser Content Toolbox" menu entry. One for each content process.
Flags: needinfo?(mrbkap)
Gabor, may be you are less busy then Blake these days!
Do you mind looking at this patch and confirm me it works as expected?
Flags: needinfo?(gkrizsanits)
(I'm not asking for a review but more to test it and confirm it works great)
(In reply to Alexandre Poirot [:ochameau] from comment #2)
> Gabor, blake, could you confirm me it works fine for you?
> 
> This patch should show n items in the "Browser Content Toolbox" menu entry.
> One for each content process.

I tested it on a fresh build (mc, debug mode) and the Browser Content Toolbox does have an arrow but the menu does not seem to have any children (hovering or clicking do not opens up any popup with the list of content processes)

Btw, another approach is that instead of the list of content processes it could just use the content process of the currently selected tab... But I'm not very familiar with the Browser Content Toolbox so I'm not sure which version is more desirable.
Flags: needinfo?(gkrizsanits)
Hum. It works fine here. May be some other patch in my queue is required... I'll check that.

Regarding the behavior of this, I don't know. It is helpful in one way as it would help opening a toolbox matching a given tab. But it could also be confusing as you wouldn't really understand the fact that there can be multiple distinct processes.
I think what you describe is doable if we can somehow find out for a given <xul:browser>, what is the related messageManager in Services.ppmm list. I don't know if that's something that exists today?

Do you think we could ask someone for feedback about what could be a good behavior?

Also I told you about a possible way to see chrome resources directly from a regular tab toolbox.
What do you think about that?
Pros: 
 - no new kind of toolbox to handle,
 - no seperate window,
 - easily match a given tab
Cons: 
 - need to find the right UX to enable this only when you are addon or platform developer and want to debug firefox guts
 - the toolbox lifecycle matches the tab, so when you close it, it will close the toolbox. It doesn't help debugging tab opening/closing. so regular browser content toolbox will always be helpful in some ways.
Hum. Something should be wrong with your build? Or is it broken on debug builds?
The try build from comment 7 works fine.
If there is no content process, the menu isn't empty, there is "no content process" message.
(In reply to Alexandre Poirot [:ochameau] PTO, back on 1st from comment #8)
> Hum. Something should be wrong with your build? Or is it broken on debug
> builds?
> The try build from comment 7 works fine.
> If there is no content process, the menu isn't empty, there is "no content
> process" message.

Sorry man, I tried with a new build again on OSX both debug and release version and I get the
same results. Have you tested it with dom.ipc.processCount set to some greater than 1 value?
For the record, I'm seeing the same results as Gabor. Furthermore, when I put a breakpoint in addContentProcessToolboxes, I don't see it being called (though a glance at the code shows that it might be getting called on startup, not when the menu is being opened). Do you maybe have a patch that changes the meaning of the addMenus function?
Flags: needinfo?(mrbkap) → needinfo?(poirot.alex)
Attached patch patch v2 (obsolete) — Splinter Review
I haven't seen any conflict after rebase, but just in case, a rebased patch against last m-c.

I still don't see any breakage in this patch.
Now I tested with just this patch on top of m-c in opt and debug builds.

The only thing I can think about is that it is broken only on Mac...
because I'm testing on linux here.

See previous comment with a try build on all platforms.
Flags: needinfo?(poirot.alex)
Also Blake, if the menu wasn't even appearing that just because you miss two prefs:
devtools.chrome.enabled and devtools.debugger.remote-enabled to be true.

Both can be toggles from devtools settings panel.
(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Also Blake, if the menu wasn't even appearing that just because you miss two
> prefs:

Sorry, I wasn't clear. The menu was appearing (with a "submenu" arrow), but the submenu was not being populated.
Are you both on OSX?
I am, yes.
So I tested this on windows 10 and it seems to work there. Here are my observations so far:

- It does not work on OSX, needs testing on Linux
- It does not work from the hamburger menu bar because Browser Content Toolbox does not even show up there
(Which means that on win10 I have to enable the Menu Bar first and only then can I find it. Plus the settings in the devtools menu... so this menu item seems to be in super ninja mode...)

(In reply to Alexandre Poirot [:ochameau] from comment #6)
> Regarding the behavior of this, I don't know. It is helpful in one way as it
> would help opening a toolbox matching a given tab. But it could also be
> confusing as you wouldn't really understand the fact that there can be
> multiple distinct processes.
> I think what you describe is doable if we can somehow find out for a given
> <xul:browser>, what is the related messageManager in Services.ppmm list. I
> don't know if that's something that exists today?

I think what would help if we could add a number temporarily to each tab while this menu
item is active, or highlight the relevant tabs when one of the content processes is selected.
I liked the later behavior the best.

I don't think we have such an API. Bill mentioned something like this I think in Bug 
1234583 comment 1. I think I'll file a new bug for that. This is something we will want to
have sooner or later anyway.

> Also I told you about a possible way to see chrome resources directly from a
> regular tab toolbox.

I don't use devtools enough to make that call. It seems to me that a checkbox somewhere
in the devtools options to toggle between the 2 modes would be the best, but then again,
I don't think my opinion counts much in this.
Depends on: 1291202
This bug is one of the few remaining issues that blocks us from enable 2 content processes on nightly. We plan to do that in FF52. Is there a chance to get this bug a bit higher priority?
Flags: needinfo?(jryans)
Alex, any chance you can help us out with this bug in the near future?
Flags: needinfo?(poirot.alex)
I'm not sure changing the Priority flag is going to change anything.

But ni? does! I stopped to procrastinate around this osx specific issue and get a hand on one to find a stupid difference that breaks only on mac.
Assignee: nobody → poirot.alex
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #20)
> I'm not sure changing the Priority flag is going to change anything.

Alright.
 
> But ni? does! I stopped to procrastinate around this osx specific issue and
> get a hand on one to find a stupid difference that breaks only on mac.

Thanks, let me know if you need help!
Flags: needinfo?(jryans)
About the hamburger menu, it looks like menu, i.e. menu entries with sub entries, like what I do here, are not shown into the hamburger menu. Instead we only see menuitems with direct action.
So, if we end up doing a menuitem without a list of all content processes, but just something "opening the toolbox for the current tab process", it should be visible in the hamburger menu.
Also it sounds easier now that you fixed bug 1291202. Showing a number on each tab sounds like a complex operation (anything involving Firefox UI isn't naive).

Do you think "opening the toolbox for the current tab process" is a right UX/behavior to provide?
Btw, is there any valuable information we can put in the toolbox title that can help understanding in which process we are connected?
Attached patch patch v3Splinter Review
Here is something that should work on OSX.
I kept the exact same behavior.
Attachment #8765948 - Attachment is obsolete: true
Attachment #8776577 - Attachment is obsolete: true
Here is an alternative. Keep the menuitem as-is, but instead, open a new content toolbox matching the current tab process.
It also allows the menu to stay in the hamburger menu.
Comment on attachment 8788895 [details] [diff] [review]
content toolbox for current tab process - v1

I imagine you prefer this patch. Does that work for you if I move forward with this instead of the other one?
Attachment #8788895 - Flags: feedback?(gkrizsanits)
(In reply to Alexandre Poirot [:ochameau] from comment #25)
> Comment on attachment 8788895 [details] [diff] [review]
> content toolbox for current tab process - v1
> 
> I imagine you prefer this patch. Does that work for you if I move forward
> with this instead of the other one?

Thanks! I'm too tired to try out / read it through today, but I would think this is the version we want.

(In reply to Alexandre Poirot [:ochameau] from comment #22)
> So, if we end up doing a menuitem without a list of all content processes,
> but just something "opening the toolbox for the current tab process", it
> should be visible in the hamburger menu.

Awesome! Thanks for explaining it I had no idea about this.

> Do you think "opening the toolbox for the current tab process" is a right
> UX/behavior to provide?

Yes. As a feature one could then select another content process within the
toolbox later.

> Btw, is there any valuable information we can put in the toolbox title that
> can help understanding in which process we are connected?

I don't think there is, I think we should do it the other way around. Marking the
tabs that are in the content process we're attached to. How that visual indicator
in the tab should look like is really not my cup of tee, but we could start with
something simple and then file a followup UX bug. And if we want to do that feature
I mentioned earlier and list the content processes so one can select one of the,
I would just list numbers, and if you hover over one of the numbers the related tabs
would get some indicator. Alternatively you can hover over tabs to select the related
content process. Anyway, this feature could be another follow up imo.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #26)
> > Btw, is there any valuable information we can put in the toolbox title that
> > can help understanding in which process we are connected?
> 
> I don't think there is, I think we should do it the other way around.

Isn't there at least a PID or something?

What about these IDs:
https://dxr.mozilla.org/mozilla-central/source/xpcom/system/nsIXULRuntime.idl#87
  /**
   * The system process ID of the caller's process.
   */
  readonly attribute unsigned long processID;

  /**
   * A globally unique and non-recycled ID of the caller's process.
   */
  readonly attribute uint64_t uniqueProcessID;

Aren't you using these ids? do they relate to process PID?

Also, while speaking about followups. To me once you are connected to a content process, the console is quite useless. There is no easy way to get a reference to one document living in it.
There easiest seems to set a breakpoint somewhere in a framescript and break on it.
Shouldn't we expose helpers to ease debugging things from the console?
(In reply to Alexandre Poirot [:ochameau] from comment #27)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #26)
> > > Btw, is there any valuable information we can put in the toolbox title that
> > > can help understanding in which process we are connected?
> > 
> > I don't think there is, I think we should do it the other way around.
> 
> Isn't there at least a PID or something?

Tab parents have that info: http://searchfox.org/mozilla-central/source/dom/interfaces/base/nsITabParent.idl#49

> 
> What about these IDs:
> https://dxr.mozilla.org/mozilla-central/source/xpcom/system/nsIXULRuntime.
> idl#87
>   /**
>    * The system process ID of the caller's process.
>    */
>   readonly attribute unsigned long processID;
> 
>   /**
>    * A globally unique and non-recycled ID of the caller's process.
>    */
>   readonly attribute uint64_t uniqueProcessID;
> 
> Aren't you using these ids? do they relate to process PID?

No, these seem to be related to the main processes PID, which we don't care about here.

> 
> Also, while speaking about followups. To me once you are connected to a
> content process, the console is quite useless. There is no easy way to get a
> reference to one document living in it.
> There easiest seems to set a breakpoint somewhere in a framescript and break
> on it.
> Shouldn't we expose helpers to ease debugging things from the console?

So the console executes the script in a system scope within the content process right?
Is there a chance we can play around with window watcher during initialization and fetch
the top level DOM windows or better yet the tabchildren?

Another way would be to teach the child process message manager to know about all the tab children that lives in its process. Bill, is this something that would fit in the design? Or is it already possible to enumerate all the tab children in a given content process (on content side)?
Flags: needinfo?(wmccloskey)
Comment on attachment 8788895 [details] [diff] [review]
content toolbox for current tab process - v1

Review of attachment 8788895 [details] [diff] [review]:
-----------------------------------------------------------------

Let me know if you need help with reviewing some parts, otherwise this looks/works great on OSX, thanks!
Attachment #8788895 - Flags: feedback?(gkrizsanits) → feedback+
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #29)
> > Also, while speaking about followups. To me once you are connected to a
> > content process, the console is quite useless. There is no easy way to get a
> > reference to one document living in it.
> > There easiest seems to set a breakpoint somewhere in a framescript and break
> > on it.
> > Shouldn't we expose helpers to ease debugging things from the console?

One of the main reasons I want to use the browser content toolbox is to debug JSMs. That's where most of the interesting code is. The console can definitely be useful for that purpose. You just import the JSM and then you can call methods and inspect state.

> So the console executes the script in a system scope within the content
> process right?
> Is there a chance we can play around with window watcher during
> initialization and fetch
> the top level DOM windows or better yet the tabchildren?

It might make more sense to integrate that sort of functionality into the normal debugger. Chrome already has a "content script" option in their devtools to debug add-on content scripts. We should probably have something like that for add-ons as well as an option to debug frame scripts for the given tab.

> Another way would be to teach the child process message manager to know
> about all the tab children that lives in its process. Bill, is this
> something that would fit in the design? Or is it already possible to
> enumerate all the tab children in a given content process (on content side)?

You can use the window watcher to find all top-level windows and then QI them to nsIContentFrameMessageManager. Here's a sort-of example:
http://searchfox.org/mozilla-central/rev/124c120ce9d7e8407c9ad330a9d6b1c4ad432637/dom/base/nsCCUncollectableMarker.cpp#299
Flags: needinfo?(wmccloskey)
Comment on attachment 8789328 [details]
Bug 1280267 - Open a browser content toolbox matching the current tab process.

https://reviewboard.mozilla.org/r/77608/#review76338

Seems resonable overall!  It would be good to avoid the looping through mms if we can, but that could also be follow up work.

::: devtools/client/framework/devtools-browser.js:274
(Diff revision 1)
> -  openContentProcessToolbox: function () {
> -    this._getContentProcessTarget()
> +  openContentProcessToolbox: function (gBrowser) {
> +    let { childCount } = Services.ppmm;
> +    // Get the process message manager for the current tab
> +    let mm = gBrowser.selectedBrowser.messageManager.processMessageManager;
> +    let processId = null;
> +    for (let i = 1; i < childCount; i++) {

Can you use the `osPid` from nsITabParent that :gabor mentioned instead of looping here?  I guess you'd need to change the actor to also receive such an ID.  What do you think about changing to that?
Attachment #8789328 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) (on PTO Sept. 10 - 25) from comment #32)
> ::: devtools/client/framework/devtools-browser.js:274
> (Diff revision 1)
> > -  openContentProcessToolbox: function () {
> > -    this._getContentProcessTarget()
> > +  openContentProcessToolbox: function (gBrowser) {
> > +    let { childCount } = Services.ppmm;
> > +    // Get the process message manager for the current tab
> > +    let mm = gBrowser.selectedBrowser.messageManager.processMessageManager;
> > +    let processId = null;
> > +    for (let i = 1; i < childCount; i++) {
> 
> Can you use the `osPid` from nsITabParent that :gabor mentioned instead of
> looping here?  I guess you'd need to change the actor to also receive such
> an ID.  What do you think about changing to that?

I'll try to polish that in a followup as that's not that easy.
I'm not sure I can get the os PIDs from the parent.
I can get the nsITabParent for a <xul:browser> via browser.frameLoader.tabParent.osPid
but I can't get it for a process message manager: Services.ppmm.getChildAt(1)
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a130813c15a4
Open a browser content toolbox matching the current tab process. r=jryans
https://hg.mozilla.org/mozilla-central/rev/a130813c15a4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.