Closed Bug 1060093 Opened 10 years ago Closed 10 years ago

[e10s] Get Browser Toolbox up for Content Process

Categories

(DevTools :: General, defect, P3)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(e10sm3+)

Status:
RESOLVED FIXED
Milestone:
Firefox 36
Tracking Flags:
Tracking Status
e10s m3+ ---

People

(Reporter: cpeterson, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 13 obsolete files)

ContentProcessactor v6
33.90 KB, patch
jryans
: review+
jryans
: checkin+
Details | Diff | Splinter Review
Report cloned scripts to Debugger.
8.78 KB, patch
jimb
: checkin+
Details | Diff | Splinter Review 
Ally says the debugger and console output work for JS running in the e10s content process when testing with the "File > New e10s Window", but not with the "browser.tabs.remote.autostart" pref enabled. The "New e10s Window" is a temporary testing feature; autostart enables the e10s code we plan to ship, including extra add-on compatibility shims that apparently interfere with the devtools.

Ally recommends that we fix the debugger and console for our e10s "M2" milestone so add-on developers have a fighting chance of fixing their add-ons' e10s problems.
I just tested on Linux and everything seems to be working fine in both scenarios. In my Mac however I get empty pages in both cases, along with such messages in the terminal as this:

[Parent 98694] WARNING: No docshells for remote frames!: file /Users/past/src/fx-team/content/base/src/nsFrameLoader.cpp, line 511

I got that behavior once on Linux with a new e10s window, but I couldn't reproduce it since.

All of this is with local builds from fx-team tip.
Blocks: 1042253
Blocks: old-e10s-m2
tracking-e10s: ? → +
Priority: -- → P3
Assignee: nobody → mconley
i'm looking into what/how console logging is supposed to work for addon JS (and SDK in particular) in the child process.

should addons/sdk use the same devtools/Console.jsm API, even in the child process, and it will automagically be handled as appropriate, or should all the logging be forwarded to the parent process, and logged to the console from there?

if that works, secondary question: is listening for console observer notifications in the parent process going to get notifications for console messages logged in the child process?
Hey ally,

I don't think I fully understand this bug... I'm able, for example, to see console.log output for a site in a remote browser just fine. Do you have STR that I can go through to see what you're seeing?

-Mike
Flags: needinfo?(ally)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> I don't think I fully understand this bug... I'm able, for example, to see
> console.log output for a site in a remote browser just fine. Do you have STR
> that I can go through to see what you're seeing?

In bug 1042253 comment 22, Heather said she sees log output in the web console, but not the browser console.
Ah, yes - got it. I've reproduced this now.
Flags: needinfo?(ally)
STR:

1) Create a site that makes liberal use of console.log
2) Visit that site using an e10s window / remote tab
3) Open the Browser Console

ER:

The console.log messages should appear in the Browser Console.

AR:

While the messages appear in the web console for that tab, they do not appear in the Browser Console.
Summary: [e10s] Debugger and console output do not work for JS running in the e10s content process → [e10s] Debugger and console output do not show up in Browser Console when running in the e10s content process
Hm, according to past, the STR I listed in comment 6 actually describe a desirable scenario. It's been a long-standing bug that console.* called from web content will show up in the Browser Console that they've been unable to fix up until now.

Browser Console was only ever supposed to show messages from chrome code and add-ons.

I'm testing those two scenarios now. ally - is there a more specific set of STR for this bug? Also, can you describe how the debugger is involved? past was very interested in that bit.
Flags: needinfo?(ally)
Bug 860705 is for stopping the browser console from displaying console messages from open tabs. We just hadn't found the time to do it yet. I would be happy if e10s magically solved that problem!
So I've done some testing...

Calling console.* from both a frame and chrome script via Console.jsm within Firefox code shows up in the Browser Console, with and without browser.tabs.remote.autostart set to true.

Calling console.* from an SDK add-on from its main.js shows up in the Browser Console, with and without browser.tabs.remote.autostart set to true.

Calling console.* from a framescript in an SDK add-on shows up in the Browser Console without browser.tabs.remote.autostart set to true. If, however, it's set to true, I get the following trace in the Browser Console:

TypeError: window is null
Stack trace:
getInnerId@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/window/utils.js:76:3
@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/content/worker.js:138:20
dispatch@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/method/core.js:119:12
WorkerConstructor@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/content/worker.js:74:7
constructor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/heritage.js:145:23
Worker@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/tabs/worker.js:11:16
attach@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/tabs/tab-firefox.js:233:12
@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://jid1-flty937oi566ca-at-jetpack/test-addon/lib/main.js:5:16
_emitOnObject@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/deprecated/events.js:153:9
_emit@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/deprecated/events.js:123:12
_emitEvent@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/windows/tabs-firefox.js:135:5
_emitOnObject@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/deprecated/events.js:153:9
_emit@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/deprecated/events.js:123:12
TabTrait<._onContentEvent@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/mikeconley/Library/Application%20Support/Firefox/Profiles/32tsfewj.e10s/extensions/jid1-fLty937OI566CA@jetpack.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/tabs/tab-firefox.js:111:5

The code in my lib/main.js was:

var tabs = require("sdk/tabs");
var self = require("sdk/self");

tabs.on('ready', function(tab) {
  var worker = tab.attach({
    contentScriptFile: self.data.url("content-script.js")
  });
  worker.port.emit("console", "Message from the add-on");
});

and from data/content-script.js, it was:

self.port.on("console", function(message) {
  console.log("Logging from content script: " + message);
});

So that's busted, but I don't think that's strictly related to console.* calls. Anything I attempt to do in this content-script.js results in this error.

I have not yet tried a "traditional" add-on yet - I'll do that next.
Calling console.* from the chrome-level of a traditional add-on works properly with and without autostart.

Calling Console.jsm's console.* from a framescript of a traditional add-on works properly with and without autostart.

ally - have I hit the case that you're talking about in this comment or comment 9? I don't think I can move forward on this until I have a better sense of what we're trying to fix, so I'll hold off further investigation until I hear back.
This bug should be fixed for the e10s M2 milestone.
tracking-e10s: + → m2+
0) autostart = true
1) console output from the content process does not show up in the devtools console
2) set a break point in content process js, and try to hit it. 
3) if there is js code that is only in the content process, it will not show up in the debugger's left hand panel of places to set breakpoints
Flags: needinfo?(ally)
What's the correct procedure to get a Console.jsm instance to work in a sandbox instantiated inside a child process frame script?

I have a SDK addon which loads a frame script. Inside the framescript itself I can import console.jsm and log without problems. But either passing that console instance via the sandbox prototype or creating jet another instance via evalInSandbox() and then logging from within the sandbox does not seem to work.

By "does not work" I mean that the created console instance only logs to the CLI but not to the browser console.
So I just spoke to past, and here's what I've found out:

Similar to the fact that console.* from content is not supposed to go into the Browser Console, DevTools does not want console.* from content to go into the Browser Toolbox console either. e10s fixes this long-standing bug.

Using Console.jsm from content scripts, however, should result in stuff going to the Browser Toolbox console. I just checked that it works, and it seems to.

So I think in terms of Console stuff, I believe we're working as intended / desired.

I'm looking into the debugger stuff now...
So, for the debugger thing, it appears as if we simply do not have support for debugging scripts that are only loaded in the content process. They just do not appear in the script list. I looked at old builds as far back as April, and it looks like it never was supported. I was using browser-child.js, which is loaded by remote browsers, as my litmus test.

And for the scripts that are loaded in both processes (content.js, for example), breakpoints set in that script will only ever be hit in the parent process. For example, I set a breakpoint in the click event handler for about:home in content.js[1], and was able to hit it when about:home is loaded in a non-remote tab. No hits if it's loaded in a remote tab.

So I'm going to modify this bug summary to reflect my findings today.

[1]: http://hg.mozilla.org/mozilla-central/file/f7a27a866c47/browser/base/content/content.js#l194
Summary: [e10s] Debugger and console output do not show up in Browser Console when running in the e10s content process → [e10s] Cannot debug scripts running in the content process
So I spent the morning digging at the debugger. There's several communication points:

1) Client ("Browser Toolbox")
2) Server (Firefox chrome process)
3) Firefox content process

Communication between 2 and 3 takes place over "ChildDebuggerTransport". 2 is the mediator between 1 and 3.

I can see the ChildDebuggerTransport layer getting hooked up properly. The "child.js" script is loaded into the content process of a remote tab, and communications are established over the message manager. However, once that is hooked up, the channel is silent - no communications take place between the two.

With victorporof's help, I found the point where the client requests script sources from the server (ThreadActor.onSources), and I can see the chrome process getting its scripts retrieved properly... but at no point do we ask the content process for its sources.

I think at this point, I might need to wait for someone from devtools (past) to advise on this, since I'm slightly out of my depth here. past said he'd have some cycles tomorrow, so needinfo'ing.
Flags: needinfo?(past)
Status: NEW → ASSIGNED
Summary: [e10s] Cannot debug scripts running in the content process → [e10s] Cannot debug chrome scripts running in the content process with the Browser Toolbox
So, STR for those following along:

1) Start Firefox with browser.tabs.remote.autostart enabled
2) Open the Browser Toolbox (if that's not available in the Developer Tools, follow these steps: http://screencast.com/t/fBpGhGgqwQV)
3) Give the Browser Toolbox permission to connect to your e10s browser
4) Switch to the debugger pane, and look for browser-child.js.

ER:

browser-child.js should be available in the list of scripts one can set breakpoints on and debug.

AR:

browser-child.js is missing.
Gavin, what do you think about moving this bug to e10s-M3? I don't think it's a huge amount of work, but it's taking a while to come up with a plan for fixing it. Additionally, this doesn't seem like a do-or-die sort of thing for M2. If we can't debug chrome code in the child process (mainly content scripts), it will be irritating for frontend and add-on devs, but probably no worse than a lot of other things.
Flags: needinfo?(gavin.sharp)
(In reply to Bill McCloskey (:billm) from comment #18)
> If we can't debug chrome code in the child process (mainly
> content scripts), it will be irritating for frontend and add-on devs, but
> probably no worse than a lot of other things.

Plus, anyone blocked by debugger problems can just disable e10s.
IIRC we put this in the m2 list due to concerns about content script errors not showing up in the console (as mentioned in comment 0). If that's not actually a problem then I agree this doesn't need to block m2.
Flags: needinfo?(gavin.sharp)
To recap yesterday's discussion on IRC, there are two parts in the required changes here:
- the simple one is the debugger backend changes, which is also needed for b2g, and Alex has a WIP patch for that in bug 1027310
- the more involved one is the debugger frontend changes that depend on how we want to present the option to target chrome content scripts to Firefox/add-on devs

The second part overlaps somewhat with our older goal of giving said devs the option to target a specific browser window. My favorite solution so far involves repurposing the toolbox's recent iframe switch in the Browser Toolbox as a process/window switch. The user would pick from the drop-down list a child process and then the toolbox would use the actors of that process as its target.

Another option will come in the near future from the worker debugging changes in the UI. The plan so far involves adding a new pane with workers in the debugger, which would include the main thread as well. Depending on the final shape of it, it may turn out to be a good place for the Browser Debugger to also display child processes.
Flags: needinfo?(past)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #20)
> IIRC we put this in the m2 list due to concerns about content script errors
> not showing up in the console (as mentioned in comment 0). If that's not
> actually a problem then I agree this doesn't need to block m2.

Ok, I'll go ahead and bump this to tracking-e10s:m3+.
tracking-e10s: m2+ → m3+
No longer blocks: 1063680
(In reply to Panos Astithas [:past] from comment #21)
> To recap yesterday's discussion on IRC, there are two parts in the required
> changes here:
> - the simple one is the debugger backend changes, which is also needed for
> b2g, and Alex has a WIP patch for that in bug 1027310
> - the more involved one is the debugger frontend changes that depend on how
> we want to present the option to target chrome content scripts to
> Firefox/add-on devs
> 
> The second part overlaps somewhat with our older goal of giving said devs
> the option to target a specific browser window. My favorite solution so far
> involves repurposing the toolbox's recent iframe switch in the Browser
> Toolbox as a process/window switch. The user would pick from the drop-down
> list a child process and then the toolbox would use the actors of that
> process as its target.
> 
> Another option will come in the near future from the worker debugging
> changes in the UI. The plan so far involves adding a new pane with workers
> in the debugger, which would include the main thread as well. Depending on
> the final shape of it, it may turn out to be a good place for the Browser
> Debugger to also display child processes.

As someone who often uses the Browser Toolbox to debug chrome scripts, I'd want to make sure that I can set breakpoints in several targets - so, one breakpoint in a main process chrome script, and another breakpoint in a content process chrome script. I'd need to be able to hit both of these.

Does the first option, where one can switch between targets allow this? If not, we might want to go with the latter option, since it sounds like it'd allow one to view / debug scripts across processes.
Bug 1003097 is the bug for making it possible to debug workers.
At least to just bootstrap being able to debug both chrome process and chrome-in-content-process, we could just open another instance of the browser toolbox that is connected to the content process. Not the greatest UI, but should be fairly easy to get up and running and allows setting BPs wherever needed.

Something to consider doing first, and then making the UI better in follow ups.
That sounds like a short path I can get behind. We can expose something like Browser Content Toolbox in the Developer Tools menu, and only show it if:

1) Addon / chrome debugging is enabled
2) The browser window is an e10s window

I'll look into what it'll take to pull this off now.
Ok, here's my gameplan:

1) Create two new prefs in firefox.js: devtools.debugger.remote-chrome-debugging-port (default 6081), devtools.debugger.remote-chrome-debugging-host (default "localhost").

2) Expose a new item in the Developer Tools menu iff chrome debugging is enabled and the current window has gMultiProcessBrowser set to true. Call this menu item "Browser Content Toolbox", or similar.

3) Make that menuitem fire a command that calls BrowserToolboxProcess.init() with an option - specifically, tell it that we're opening a Browser Toolbox for connecting to the content process.

4) Make BrowserToolboxProcess.init notice that option, and somehow... somehow communicate with the content process to get the chrome debugger server listening at the port that we specified. We probably want to not proceed until we get confirmation that the server is listening, so perhaps wait for a success message from the content process.

5) Once the server is listening, start up the Browser Toolbox process (using the same profile as one would normally use for Browser Toolbox) - but do not to overwrite the prefs file if the profile is locked. We want to be able to have the Browser Toolbox and Browser Content Toolbox open simultaneously. We start up the process, and pass in a parameter (like the addonId), that tells the toolbox that it should attempt to connect to the content process.

6) Browser Toolbox process starts, and toolbox-process-window.js loads up. Once the Toolbox is ready, it reads the parameter that says that we're connecting to the content process, and reads the host / port pref that we set up in (1). It then connects to it.

7) I assume the content process is going to try to hit the same code path for prompting for the incoming connection. We need to forward that to the parent.

8) Once the parent OK's the connection, we're connected and... we're done?

Does that sound like a sane approach, or am I overlooking something?
Flags: needinfo?(nfitzgerald)
Sounds pretty good to me, but I haven't really dealt with the e10s stuff too much. A couple questions though: For e10s, do we only have two processes? One chrome and one for all content? If not, how would your plan handle debugging multiple content processes?

Panos, what do you think of the above plan?
Flags: needinfo?(nfitzgerald) → needinfo?(past)
For now and for the foreseeable future, we'll have just a single content process. We'll definitely be experimenting with multiple content processes in the future, but that's not a shipping goal for E10S right now.

So it's safe to assume a single chrome process, and a single content process.

I've reached part 4 of my 8 step plan, but I've not yet figured out the best way of telling the content process to start the debugger server... I see that we have a RemoteBrowserTabActor that'll do the frame script injection for a remote tab, and I'm trying to see how to make use of that, but this is some pretty sophisticated architecture, and it's slow going. My ears are wide open for tips and advice. :)
The plan sounds like a reasonable compromise to me, although I expect those who are annoyed about the redundancy of having both a Browser Console and the console in the Browser Toolbox, to be not that overjoyed about it :)

If I am not mistaken, even when we have multiple content processes, there will only be one that hosts the content process for a particular tab, so for the main use case described in comment 23 we should be fine. For having access to every script loaded in any running process, the target switch in the Browser Toolbox would be the best solution.
Flags: needinfo?(past)
It seems I glossed over some of the details in the 8 steps Mike proposed and although the direction is fine, some details need adjustment.

One important detail is the transport we use for each debugging scenario. Content debugging currently uses direct object references and a message manager channel for remote tabs. Chrome debugging is done from a separate process and uses a remote transport based on TCP sockets. Debugging chrome frame scripts doesn't require an additional process, since the debuggee code is already in a separate one.

Furthermore, in terms of protocol communication, chrome debugging makes a listTabs request and then concentrates on global-scoped actors that are *not* in the list of tabs (where tab-scoped actors are). On the contrary, the actors we are interested in this case will be RemoteBrowserTabActors, which represent tabs.

So to cut a long story short, what I think we need to do here (for the expedient solution) is make the Browser Content Toolbox launch a normal toolbox as a separate window, like we do for example here (although using TargetFactory.forTab, not TargetFactory.forRemoteTab):

https://github.com/campd/fxdt-adapters/blob/master/lib/main.js#L125

then tweak debugger/panel.js and debugger-controller.js to attempt to listTabs and finally pick the tab.chromeDebugger (instead of tab.threadActor) actor that the patch from bug 1027310 should expose.

I think that should work.
hey Mike, i tested console in the child process on windows, using scratchpad, and it doesn't seem to show up, either in the browser console nor the browser toolbox console:

  let mm = gBrowser.addTab('data:text/html,dumb').linkedBrowser.messageManager;

  mm.addMessageListener(7, function() {
    console.log('truc'); 
  })

  mm.loadFrameScript('data:,new ' + function() {
    let CA = Components.utils.import("resource://gre/modules/devtools/Console.jsm", {});
    let console = new CA.ConsoleAPI();
    console.log('blah');    
    sendAsyncMessage(7);
  }, true);

according to my discussion with Panos and your testing (on OSX), i understand this should work, so maybe it's a windows-only bug?
No longer blocks: 1067576
Assignee: mconley → ally
ally - I realized I had an old experimental patch lying around. Not sure if you'll find it useful or not, but I rebased it and attached it just in case.

The patch adds the menu item for a "Browser Content Toolbox", and starts to do the rigging for signaling the content process to start listening on a particular port, but that's as far as I got with it.
Summary: [e10s] Cannot debug chrome scripts running in the content process with the Browser Toolbox → [e10s] Cannot debug scripts running in the content process with the Browser Toolbox
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #31)
> It seems I glossed over some of the details in the 8 steps Mike proposed and
> although the direction is fine, some details need adjustment.
> 
> One important detail is the transport we use for each debugging scenario.
> Content debugging currently uses direct object references and a message
> manager channel for remote tabs. Chrome debugging is done from a separate
> process and uses a remote transport based on TCP sockets. Debugging chrome
> frame scripts doesn't require an additional process, since the debuggee code
> is already in a separate one.
> 
> Furthermore, in terms of protocol communication, chrome debugging makes a
> listTabs request and then concentrates on global-scoped actors that are
> *not* in the list of tabs (where tab-scoped actors are). On the contrary,
> the actors we are interested in this case will be RemoteBrowserTabActors,
> which represent tabs.
> 
> So to cut a long story short, what I think we need to do here (for the
> expedient solution) is make the Browser Content Toolbox launch a normal
> toolbox as a separate window, like we do for example here (although using
> TargetFactory.forTab, not TargetFactory.forRemoteTab):
> 
> https://github.com/campd/fxdt-adapters/blob/master/lib/main.js#L125
> 
> then tweak debugger/panel.js and debugger-controller.js to attempt to
> listTabs and finally pick the tab.chromeDebugger (instead of
> tab.threadActor) actor that the patch from bug 1027310 should expose.
> 
> I think that should work.

I'm taking the bug over from Mike, and I don't understand the context that Panos is talking about here. Could you fill me in?
Flags: needinfo?(nfitzgerald)
The protocol Panos is talking about is (mostly) documented here: https://wiki.mozilla.org/Remote_Debugging_Protocol

We use different types of transports for our remote debugging protocol packets depending on what/where the client (debugger front end) and server (device being debugged, serving debugging information) are. For example, for non-e10s local debugging, we just use a transport that does pretty much the equivalent of Object.freeze(packet) so we don't have to actually parse and stringify JSON packets that are staying on the same machine. For remote debugging, the transport we use is a wrapper around a TCP socket so we can communicate with the remote instance of Firefox. As we introduce multi process stuff, the transport needs to know how to marshal packets between these processes; that's pretty much its only job.

I know less about the Target stuff, perhaps Panos or someone else can fill in more of that.
Flags: needinfo?(nfitzgerald)
I stumbled upon this problem today when trying to set breakpoints in protocol.js actor classes via the browser toolbox.
This bug makes it impossible to debug the devtools server-side code in e10s.
Summary: [e10s] Cannot debug scripts running in the content process with the Browser Toolbox → [e10s] Get Browser Toolbox up for Content Process
debug:anaaktge -> start browser toolbox in content process
debug:anaaktge:ack -> child notifies parent that the browser toolbox is set up & listening

Prototyping
-- seems to only really work if browser toolbox is started before content browser toolbox. the debugger.init() call fails if child.js hasn't been invoked first. It's really unclear to me how that code should/does interact with the plan laid out in comment 27.

Current console output (does appear out of order):
Initializing chrome debugging process. ToolboxProcess.jsm:215
Running chrome debugging process. ToolboxProcess.jsm:225
Chrome toolbox is now running... ToolboxProcess.jsm:232
child: Content process server running on content side in child.js child.js:37
parent: Initializing the content toolbox server. ToolboxProcess.jsm:123
parent: loaded parent process message manager singleton ToolboxProcess.jsm:126
parent:added listener for debug:anaaktge:ack, sent debug:anaaktge ToolboxProcess.jsm:129
parent receiveMsg debug:anaaktge:ack ToolboxProcess.jsm:169
parent received debug:anaaktge:ack ToolboxProcess.jsm:171
child: cpmm received debug:anaaktge ContentProcessSingleton.js:71
child: opened listener on port 6081 ContentProcessSingleton.js:83
child: cpmm sending debug:anaktge:ack to parent
parent browser toolbox sets up normally

parent crashes once the ack from the child comes back and we try to connect....

windows informs me:
Unhandled exception at 0x525BCB8B (xul.dll) in firefox.exe: 0xC0000005: Access violation reading location 0x00000000.

I also see shortly before hand ...
[JavaScript Error: "child: cpmm sending debug:anaktge:ack to parent" {file: "file:///c:/Users/A/fx-team/obj/dist/bin/components/ContentProcessSingleton.js" line: 86}]
The thread 'Font Loader' (0x2330) has exited with code 0 (0x0).

###!!! [Child][MessageChannel::SendAndWait] Error: Channel error: cannot send/recv
Attachment #8499690 - Attachment is obsolete: true
Attachment #8504283 - Attachment is obsolete: true
Bill, we're wondering if the modal prompt is what causes the near immediate crash of fx.

prompt connection code is http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#193

I'm also wondering if I've run into another ipc/cpow issue.
Flags: needinfo?(wmccloskey)
stack from firefox.exe

[JavaScript Error: "parent: Initializing the content toolbox server." {file: "resource:///modules/devtools/ToolboxProcess.jsm" line: 123}]
[JavaScript Error: "parent: loaded parent process message manager singleton" {file: "resource:///modules/devtools/ToolboxProcess.jsm" line: 125}]
[JavaScript Error: "parent:added listener for debug:anaaktge:ack, sent debug:anaaktge" {file: "resource:///modules/devtools/ToolboxProcess.jsm" line: 131}]
[JavaScript Error: "parent receiveMsg debug:anaaktge:ack" {file: "resource:///modules/devtools/ToolboxProcess.jsm" line: 172}]
[JavaScript Error: "parent received debug:anaaktge:ack, connect to port" {file: "resource:///modules/devtools/ToolboxProcess.jsm" line: 174}]
[JavaScript Error: "Initializing chrome debugging process." {file: "resource:///modules/devtools/ToolboxProcess.jsm" line: 220}]
[JavaScript Error: "Running chrome debugging process." {file: "resource:///modules/devtools/ToolboxProcess.jsm" line: 234}]
First-chance exception at 0x6578CB8B (xul.dll) in firefox.exe: 0xC0000005: Access violation reading location 0x00000000.
Unhandled exception at 0x6578CB8B (xul.dll) in firefox.exe: 0xC0000005: Access violation reading location 0x00000000.
I ran this in the debugger and got the crash in the parent. I got a stack trace, and the crash is here:
  http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsProcessCommon.cpp#412
It crashes because aArgs[3] is null.

I used DumpJSStack() to see how we got here. This is the top frame:
  http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/ToolboxProcess.jsm#203
Apparently this._dbgProfilePath is null, and our process creation code doesn't like that.
Flags: needinfo?(wmccloskey)
 	xul.dll!nsDependentString::nsDependentString(const wchar_t * aData) Line 55	C++
 	xul.dll!nsProcess::CopyArgsAndRunProcessw(bool aBlocking, const wchar_t * * aArgs, unsigned int aCount, nsIObserver * aObserver, bool aHoldWeak) Line 412	C++
 	xul.dll!nsProcess::RunwAsync(const wchar_t * * aArgs, unsigned int aCount, nsIObserver * aObserver, bool aHoldWeak) Line 395	C++
 	xul.dll!NS_InvokeByIndex(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params) Line 71	C++
 	xul.dll!CallMethodHelper::Invoke() Line 2395	C++
 	xul.dll!CallMethodHelper::Call() Line 1749	C++
 	xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx, XPCWrappedNative::CallMode mode) Line 1714	C++
 	xul.dll!XPC_WN_CallMethod(JSContext * cx, unsigned int argc, JS::Value * vp) Line 1247	C++
 	mozjs.dll!js::CallJSNative(JSContext * cx, bool (JSContext *, unsigned int, JS::Value *) * native, const JS::CallArgs & args) Line 231	C++
 	mozjs.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct) Line 481	C++
 	mozjs.dll!Interpret(JSContext * cx, js::RunState & state) Line 2546	C++
 	mozjs.dll!js::RunScript(JSContext * cx, js::RunState & state) Line 431	C++
 	mozjs.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct) Line 500	C++
 	mozjs.dll!js::Invoke(JSContext * cx, const JS::Value & thisv, const JS::Value & fval, unsigned int argc, const JS::Value * argv, JS::MutableHandle<JS::Value> rval) Line 537	C++
>	mozjs.dll!JS_CallFunctionValue(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<JS::Value> fval, const JS::HandleValueArray & args, JS::MutableHandle<JS::Value> rval) Line 5026	C++
 	xul.dll!nsFrameMessageManager::ReceiveMessage(nsISupports * aTarget, const nsAString_internal & aMessage, bool aIsSync, const mozilla::dom::StructuredCloneData * aCloneData, CpowHolder * aCpows, nsIPrincipal * aPrincipal, nsTArray<nsString> * aJSONRetVal) Line 1079	C++
 	xul.dll!nsFrameMessageManager::ReceiveMessage(nsISupports * aTarget, const nsAString_internal & aMessage, bool aIsSync, const mozilla::dom::StructuredCloneData * aCloneData, CpowHolder * aCpows, nsIPrincipal * aPrincipal, nsTArray<nsString> * aJSONRetVal) Line 1099	C++
 	xul.dll!mozilla::dom::nsIContentParent::RecvAsyncMessage(const nsString & aMsg, const mozilla::dom::ClonedMessageData & aData, const nsTArray<mozilla::jsipc::CpowEntry> & aCpows, const IPC::Principal & aPrincipal) Line 243	C++
 	xul.dll!mozilla::dom::PContentParent::OnMessageReceived(const IPC::Message & __msg) Line 4214	C++
 	xul.dll!mozilla::ipc::MessageChannel::DispatchAsyncMessage(const IPC::Message & aMsg) Line 1110	C++
 	xul.dll!mozilla::ipc::MessageChannel::DispatchMessageW(const IPC::Message & aMsg) Line 1050	C++
 	xul.dll!mozilla::ipc::MessageChannel::OnMaybeDequeueOne() Line 1038	C++
 	xul.dll!MessageLoop::RunTask(Task * task) Line 359	C++
 	xul.dll!MessageLoop::DeferOrRunPendingTask(const MessageLoop::PendingTask & pending_task) Line 369	C++
 	xul.dll!MessageLoop::DoWork() Line 444	C++
 	xul.dll!mozilla::ipc::DoWorkRunnable::Run() Line 234	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 830	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 265	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 99	C++
 	xul.dll!MessageLoop::RunInternal() Line 230	C++
 	xul.dll!MessageLoop::RunHandler() Line 224	C++
 	xul.dll!MessageLoop::Run() Line 198	C++
 	xul.dll!nsBaseAppShell::Run() Line 166	C++
 	xul.dll!nsAppShell::Run() Line 180	C++
 	xul.dll!nsAppStartup::Run() Line 281	C++
 	xul.dll!XREMain::XRE_mainRun() Line 4165	C++
 	xul.dll!XREMain::XRE_main(int argc, char * * argv, const nsXREAppData * aAppData) Line 4238	C++
 	xul.dll!XRE_main(int argc, char * * argv, const nsXREAppData * aAppData, unsigned int aFlags) Line 4452	C++
 	firefox.exe!do_main(int argc, char * * argv, nsIFile * xreDirectory) Line 287	C++
 	firefox.exe!NS_internal_main(int argc, char * * argv) Line 652	C++
 	firefox.exe!wmain(int argc, wchar_t * * argv) Line 120	C++
 	firefox.exe!__tmainCRTStartup() Line 240	C
 	kernel32.dll!76ee850d()	Unknown
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	ntdll.dll!775cbf39()	Unknown
 	ntdll.dll!775cbf0c()	Unknown
Panos, were you expecting (1) a third "content browser toolbox" process as the client or (2) should the existing parent process just open a toolbox itself that content to the content process's debug server?

I've been guiding Ally towards option 2 so far, though it does make the browser toolbox code a bit crazy, since the process split is quite different from today.
Flags: needinfo?(past)
(In reply to J. Ryan Stinnett [:jryans] from comment #45)
> Panos, were you expecting (1) a third "content browser toolbox" process as
> the client or (2) should the existing parent process just open a toolbox
> itself that content to the content process's debug server?

I meant to say:

(2) should the existing parent process just open a toolbox itself that connects to the content process's debug server?
Attached patch remoteDebuggingWIP.txt (obsolete) — — Splinter Review 
- now the child process crashes with QuickExit() from MsgDropped, billm tells me that that means the connection has been closed on the parent's side.

I have also noticed at the start that the task manager indicates that there are firefox.exes, and two plugin-container.exes, all rooted in my objdir.

The task manager indicates that the parent process has exited (it's no longer there) and vs appears to have silently detached from both firefox.exes
Attachment #8504409 - Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
The 4 process thing is weird. We start with 2 processes like normal, I am wondering if the devtools browser toolbox starting its new process actually starts an e10s chrome process, which starts its own child process...and thats where everything goes to hell.

Past, jryans, harth, fitzgen, billm, thoughts?
(In reply to J. Ryan Stinnett [:jryans] from comment #46)
> (In reply to J. Ryan Stinnett [:jryans] from comment #45)
> > Panos, were you expecting (1) a third "content browser toolbox" process as
> > the client or (2) should the existing parent process just open a toolbox
> > itself that content to the content process's debug server?
> 
> I meant to say:
> 
> (2) should the existing parent process just open a toolbox itself that
> connects to the content process's debug server?

I did mean (2), with the toolbox opened in a separate window. No need for a 3rd process at all (unless it somehow makes things simpler, but that doesn't seem to be the case). I think the code that would be triggered by the Browser Content Toolbox menu item would be something like:

let target = yield devtools.TargetFactory.forTab(tab);
let hostType = devtools.Toolbox.HostType.WINDOW;
let toolbox = yield gDevTools.showToolbox(target, "jsdebugger", hostType, options);

But as I elaborated in comment 31, debugger/panel.js and debugger-controller.js would have to be tweaked to attempt to listTabs and finally pick the tab.chromeDebugger (instead of tab.threadActor) actor that the patch from bug 1027310 should expose.
Flags: needinfo?(past)
Here's a WIP version of what I believe Panos is suggesting in comment 49 and comment 31.  This also includes some of the work from bug 1027310.

This approach does not start a new TCP socket at all.  Instead the tab gets a new |chromeDebugger| actor, and we connect a toolbox to it through the existing framework we have that uses the message manager to talk to the child process.

Via protocol logging (devtools.debugger.log), I can see that this toolbox is indeed talking to the new |chromeDebugger|, however the sources list only shows the regular tab sources, so there is still more work to do.

It feels like something is not quite right on the server-side (actor) and is still filtering sources or something like this, because the protocol logs only show the tab sources in the packets, which suggests the filtering is not something being done client side.
Flags: needinfo?(wmccloskey)
seeing an occasional crash in 
            MOZ_ASSERT(fun->isInterpretedLazy()); fn CreateLazyScriptsForCompartment() in jscompartment.cpp
The wip patch seems to attach the tab actor as the parent actor in ThreadActor(). To get global sources, we need the root actor.

The telltale sign is the attach packet in the server log is for tab1: {to:conn2.child2/tab1 type:attach}
(In reply to please NEEDINFO? :ally  Allison Naaktgeboren from comment #52)
> The wip patch seems to attach the tab actor as the parent actor in
> ThreadActor(). To get global sources, we need the root actor.
> 
> The telltale sign is the attach packet in the server log is for tab1:
> {to:conn2.child2/tab1 type:attach}

I believe we do want a tab here, because that allows us to go into the child / content process for the tab.

Using the root actor would give the same behavior as the existing parent process browser toolbox, as that is indeed what it connects to.

It's true that historically connecting the debugger to a tab doesn't get the chrome sources we want.  That's why a "chromeDebugger" was added in the WIP to every tab.  Panos' plan was that by connecting the debugger to "chromeDebugger" for the tab, the debugger would get access to the chrome sources in the same process as the tab.
(In reply to J. Ryan Stinnett [:jryans] from comment #53)
> (In reply to please NEEDINFO? :ally  Allison Naaktgeboren from comment #52)
> > The wip patch seems to attach the tab actor as the parent actor in
> > ThreadActor(). To get global sources, we need the root actor.
> > 
> > The telltale sign is the attach packet in the server log is for tab1:
> > {to:conn2.child2/tab1 type:attach}
> 
> I believe we do want a tab here, because that allows us to go into the child
> / content process for the tab.
> 
> Using the root actor would give the same behavior as the existing parent
> process browser toolbox, as that is indeed what it connects to.

Don't we load the whole server into child processes? So doesn't the child process have its own root actor? I think that's the root actor we want, not the chrome process's root actor.
So jryans and I seem to agree on IRC that the Correct(tm) solution is to unify the way we deal with e10s and b2g child processes. What we really want to say here is "start me a browser toolbox for each firefox process" and we have no way to express that over the RDP as it is now. We need to change the actor hierarchy looks something like this:

RootActor
|-- ChildProcessActor1
|   |-- childProcess1/ChromeDebuggerActor
|   |-- childProcess1/TabActor1
|   `-- childProcess1/TabActor2
|-- ChildProcessActor2
|   |-- childProcess2/ChromeDebuggerActor
|   `-- childProcess2/TabActor1
|-- ChildProcessActor3
|   |-- childProcess3/ChromeDebuggerActor
|   |-- childProcess3/TabActor1
|   |-- childProcess3/TabActor2
|   `-- childProcess3/TabActor3
`-- ChromeDebuggerActor

On b2g, there would be many ChildProcessActors that only have a single TabActor. On e10s there would be one ChildProcessActor and many TabActors.

We would have a ChromeDebuggerActor per process, which is really what we want here (one for the RootActor, which is the one we can connect to now; plus one per ChildProcessActor, which is what we want to add support for).

This is likely a bit of work, and it might be better to hack a work around for now :/

Questions for past and ochameau:

a) Do you agree with what we've come up with as the "Correct(tm)" solution?

b) Should we implement the (or some other) "Correct(tm)" solution, or do a work around hack?
Flags: needinfo?(poirot.alex)
Flags: needinfo?(past)
c) if case of work around hack, could fryans & I have some more help with moving this implementation along?
Thanks for the ping, I see now the problem with my proposed plan. First of all, it goes without saying that we want to use the same approach for both e10s and b2g. That has always been the goal and any deviations from that goal have always been justified by impending deadlines. That said, they are still quite similar: billm borrowed heavily from the existing b2g design when he implemented the original e10s actors.

Nick's approach looks close to the original design Jim proposed in bug 797627 and even the initial ideas I had sketched in that same bug, so I'm approaching it with a warm feeling of familiarity. Alex eventually came up with a simpler approach in bug 817580, which is mostly what we are still using now. There is a good bit of design discussion in those two bugs, if you are interested in the rationale behind some of the decisions.

Another factor that has guided the current design is our improved understanding of the platform and its implementation quirks. This manifests itself in things like apps in b2g and the netmonitor backend split between main and child process. Of course we are just starting to comprehend the implementation details behind e10s, so we still need to tweak things.

With the background piece out of the way, I see 3 issues with the proposed approach. One is the already recognized effort needed to do the refactoring. But that only takes a management decision to overcome.

Another more serious one is that it breaks backwards compatibility with existing clients. And I'm not referring to the devtools toolbox here (that's issue #1), but to add-ons, like Firebug and Ember Inspector, and perhaps other projects that are using our protocol for debugging custom XULRunner apps or things like Thunderbird, etc. I don't know how many 3rd-party projects are out there using our protocol (I've only seen vague mentions for some of those), but I don't think it would be wise to break Firebug, Ember and Angular inspectors like that. The good news is that those last ones are all open source, so we could even do the work ourselves and just factor it into the overall cost of issue #1. An additional runtime cost of course is in keeping our protocol client backwards compatible with older servers (for b2g, ESR, etc.), but that's rather obvious.

The third issue is that the proposed hierarchy doesn't seem well aligned with the needs of a protocol client. In all the existing cases that I can think of, the client either wants to target a tab (content debugging), an add-on (add-on debugging), an app (b2g debugging) or the entire application (browser debugging). In every one of those cases, enumerating the available targets and selecting the one of interest is the first step. With the proposed hierarchy, retrieving the list of processes becomes dead simple, but it doesn't seem like we have a need for it. "Start me a browser toolbox for each firefox process" doesn't seem to me like a useful thing to request. In order to target a specific tab, the client would need to either enumerate all tabs in all processes, or we would have to use a selectedProcess property in the "processes" array to indicate the "process containing the current tab".

Another important detail to consider is that the actor ownership model should reflect actor lifecycles. In a non-e10s Firefox, closing a tab causes the BrowserTabActor (the concrete TabActor we use there) to disappear, taking its children, like ThreadActor with it. ChromeDebuggerActor in a non-e10s Firefox on the other hand should stick around as long as the main process (or the debugger server) is alive. On e10s however, both ContentActor (the child process part of TabActor on e10s) and ChromeDebuggerActor (for the child process) should disappear when the tab is closed.

This could be achieved with a root actor for the child process, but that brings back old design issues into the discussion, like whether the main process server acts as a bridge or as a router. The router approach is still attractive to me due to its conceptual simplicity, but our current bridge approach is faster and less memory-intensive. I think we can keep using it if we make the ChildProcessActor the IPC connection sibling of the RemoteBrowserTabActor, while keeping ContentActor the entity that handles the TabActor protocol requests, but I admit I haven't thought this option entirely through. This of course leads to a conceptual mismatch, which can only be fixed by breaking backwards compatibility.

A simpler workaround (in line with my suggested hack from comment 31) emerges if we just acknowledge the fact that in our design a ChromeDebuggerActor can be a child of a ContentActor (TabActor) and tweak the makeDebugger() design to take that into account. In more concrete terms, I can think of two approaches we can take. One is to let makeDebugger take an allGlobals flag that indicates whether it would return a Debugger for chrome or content. The other one is taking advantage of the fact that the only existing actor that cares about chrome debugging is the ChromeDebuggerActor and make it override the way ThreadActor uses makeDebugger() to do its own thing.

That's all I can think of right now, but happy to help some more if needed.
Flags: needinfo?(past)
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #57)

Yes, the backwards compat is a real issue. No good answer right now.

> This could be achieved with a root actor for the child process, but that
> brings back old design issues into the discussion, like whether the main
> process server acts as a bridge or as a router. The router approach is still
> attractive to me due to its conceptual simplicity, but our current bridge
> approach is faster and less memory-intensive. I think we can keep using it
> if we make the ChildProcessActor the IPC connection sibling of the
> RemoteBrowserTabActor, while keeping ContentActor the entity that handles
> the TabActor protocol requests, but I admit I haven't thought this option
> entirely through. This of course leads to a conceptual mismatch, which can
> only be fixed by breaking backwards compatibility.

I had originally assumed that we loaded the full server into each content process and just routed packets; it would make this a whole lot easier. In fact, the hierarchy I proposed above was basically doing that without being explicit about it.

:-/

> A simpler workaround (in line with my suggested hack from comment 31)
> emerges if we just acknowledge the fact that in our design a
> ChromeDebuggerActor can be a child of a ContentActor (TabActor) and tweak
> the makeDebugger() design to take that into account. In more concrete terms,
> I can think of two approaches we can take. One is to let makeDebugger take
> an allGlobals flag that indicates whether it would return a Debugger for
> chrome or content. The other one is taking advantage of the fact that the
> only existing actor that cares about chrome debugging is the
> ChromeDebuggerActor and make it override the way ThreadActor uses
> makeDebugger() to do its own thing.

Ignoring other issues for a second, we wouldn't want to change makeDebugger itself; you'd just want to curry different parameters to the makeDebugger function you pass to the ChromeDebuggerActor.

But.

Why should the content process's ChromeDebuggerActor be a child of a TabActor? It makes no sense intuitively (a TabActor's one job is to know only about tab-scoped things, and now we want it to also do the job of the RootActor??) nor from a lifetime perspective (why should a ChromeDebuggerActor die when the tab dies?? In fact, it /should/ outlive the tab: it should have the same lifetime as the process...) We can't say "you have to stop chrome debugging the child process, because the tab that happened to be selected when you started chrome debugging was closed". We /need/ some actor whose lifetime is equal to the content process's lifetime that we can root the ChromeDebuggerActor from.

The fact is, we really /do/ want to talk about processes here, and we currently have no way to express that over the RDP. Maybe it doesn't make sense to ask each child process about its list of tabs, but in this case we want to start a browser toolbox for both the root/chrome process and for the content process. We just happen not to have other content or addon processes, but we /are/ effectively saying we want a browser toolbox for each Firefox process, and the ChromeDebuggerActor needs to have the same lifetime as the process.

I'd be OK with just extending the RDP to talk about processes without making the TabActors children of the ChildProcessActors. This would sidestep the backwards compatibility issues and the extra round trip indirection for attaching to a tab. Something like this:

RootActor
|-- ChildProcessActor1
|   `-- ChromeDebuggerActor
|-- ChildProcessActor2
|   `-- ChromeDebuggerActor
|-- ChildProcessActor3
|   `-- ChromeDebuggerActor
|-- ChromeDebuggerActor
|-- TabActor1
|-- TabActor2
|-- TabActor3
`-- TabActor4
Attached patch patch example (obsolete) — — Splinter Review 
I agree with Panos and comment 57.

And as I said in bugs (bug 1027310 at least) and IRL in london,
I see a simplier way to offer chrome debugging of tabs...
It's not perfect and again, ignore processes.
Instead it is dead simple and would allow offering debugging in a matter of days.

I don't see much value in involving Browser toolbox to debug child processes.
(Yes I hate having more than one window!)
The tab toolbox are a very good spot to expose the chrome of child processes.
(we don't need to run a distinct window/process!!)
I can also see the point about having a fine tuned lower level browser toolbox with more features.
But do we have ressources to work on it? Is it even worth? Do we need more?
And yes my approach can be misleading, as if we share same process between two tabs,
the ressources are going to be duplicated between toolboxes, but is that so bad?

The only challenge I see in this approach is on how to expose this feature
in the toolbox. We could hack into the frame switching UI...
But implementation details make it hard to toggle the chrome mode
without reopening a brand new toolbox in chrome mode.

Also, I see no value in the ChromeDebuggerActor, we can just get rid of it.
Having a new actor just for that introduce more complexity than value.
And it appear to be useless given that makeDebugger has way more control
on which ressources we are tracking!

I'm pretty convinced such approach, even if not perfect would
offer way enough debugging for e10s and I'm not sure platform devs will
ask for much more.

To summarize what this patch does:
- Get rid of ChromeDebuggerActor
  (Appear to be useless)
- Accept a "chrome" option on tab actor `attach` request.
  If true, makeDebugger returns a chrome debugger.
  (Using ChromeDebuggerActor isn't enough, the important thing is to tweak makeDebugger)
- Read "devtools.chrome.enabled" during target creation and pass its value
  for this new `chrome` argument of attach request.
  (We would just need to add a nice UI for that)
Flags: needinfo?(poirot.alex)
The problem with that approach is that chrome JS code that outlives a tab (eg browser-child.js) isn't properly debuggable because if the TabActor for the tab that happens to be selected shuts down, then its children also shut down (such as the chrome-scoped ThreadActor née ChromeDebuggerActor).
Assignee: ally → poirot.alex
Attached patch ContentProcessActor patch (obsolete) — — Splinter Review 
Ok, so here is a tentative patch to implement a pattern similar to what has been described in comment 58.

It is working quite well after a lot of debugging.
*But* e10s API, message manager API are not really designed for per process abstraction,
so there is some issues in tracking processes and I have to broadcast some messages to all processes.
And I haven't been able to know if the content process is running or not,
that to figure out if we can open a toolbox or not.
So, in its current state, the code just silently fails opening a toolbox and do nothing
if no tab is running OOP (for example, if you only have about:sessionrestore opened).

Otherwise there is some unexpected issues with Debugger API -or- ThreadActor.
If you set a breakpoint in a frame script, it will only break existing frame script,
if you open a new tab, the breakpoint won't stop in newly loaded scripts.

Feedback and tests are highly welcomed!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=46d9ca59897c
Attachment #8508835 - Attachment is obsolete: true
Comment on attachment 8513860 [details] [diff] [review]
ContentProcessActor patch

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

I think the message manager APIs are working well here. It looks like what you're missing is a way to choose which process to debug. I don't think that's something the platform should provide. Instead, I assume you'll need a menu where the user can select which process. You could use broadcast a message to collect all the processes, and perhaps they could reply with their set of tabs. Then the user could pick which one seems right.

Were there any other message manager issues you ran into?

::: browser/base/content/content.js
@@ +733,5 @@
>    }
>  });
> +
> +// load devtools component on-demand
> +cpmm.addMessageListener("Devtools:InitDebuggerServer", function(message) {

This code should probably go in ContentProcessSingleton.js.

::: toolkit/devtools/server/content-server.jsm
@@ +14,5 @@
> +this.EXPORTED_SYMBOLS = ["init"];
> +
> +let started = false;
> +
> +function init(msg, tabChildGlobal) {

The tabChildGlobal parameter seems unused (both here and in ChildProcessActor). Remove?
bill, here is some random questions/needs I had while writing this patch:

- in the content process, is there any way to list all tab child globals or content windows?
window-mediator doesn't throw, but doesn't enumerate any window either :/
I can listen for some events to watch for new content windows being created, but that doesn't help listing existing ones, when we connect to the process.

- in the content process, do we have access to a unique ID for the process, like childID or something?
From the parent we have a childID on the frame loader, but we don't have access to the frame loader from the content process... I could sendSyncMessage to fetch it from the parent, but that sounds so sad to not have the child id from the child!

- parent process message manager appears to be quite good to implement this feature, but it is also frustrating to be limited to only message passing API. We can iterate over all message managers via childCount and getChildAt(), but given one message manager, we can't differenciate managers for documents being OOP or not. I can broadcast a message to all of them to figure this out, but that's really not handy... whereas the platform code surely knows if that remote or not. That would be really great to be able to differentiate OOP vs non-OOP somehow, with QueryInterface or an explicit attribute. (That would help me listing existing content processes and today, it would help me telling the user if we can spawn a content toolbox or not)

- finally, I had issues in choosing in which context, the console should be executed into. You may want to evaluate against the current tab child global (topmost, visible tab), or content window? But I didn't do that because it window-mediator doesn't work, and even if we can do that with various messages between parent and child, it sounds quite challenging to implement correctly. So I ended up crafting an empty chrome Sandbox, where you have to use Components.* to do something cool. We may keep this sandbox and expose some cool helpers to ease fetching tabchild globals/windows.
Attached patch ContentProcessActor patch v2 (obsolete) — — Splinter Review 
Cleaned up. Addressed first feedback from billm.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=81a26305f71f

This patch introduce a new menuitem in developer menu "Browser content toolbox".
If you happen to have the content process running, it will open a toolbox.
(otherwise it will silently do nothing)
This toolbox only exposes webconsole, debugger and scratchpad. Debugger is displayed by default.
The webconsole default context is an empty chrome sandbox.
(We could easily expose some helpers in it, or choose another default context?)

Given that this patch is based on comment 58, the toolbox doesn't track any document,
but just a process and there isn't any meaningfull unique and stable document/window/JS context.
The main usage would be to use the debugger.

This patch seems to work quite well now. The only issue left I'm seeing is the debugger
not setting breakpoint on frame scripts loaded after we set the breakpoint.

But I'm still not ultimately confident this is the best thing we could come up with.
If you want to debug content document and use an inspector, you will have to play with multiple toolboxes.
I agree this patch looks scientifically more correct regarding actor hierarchy.
But it may just be easier to tweak/hack TabActor to not die when its top-level document get destroyed.
Reusing tab actor has the benefit to come with many more feature (all tools and frame switching).
For example, I would like the root actor to inherit TabActor
or expose one in order to support frame switching in browser toolbox.
The tab actor in a content process would be really similar to this.
It would immediatly support frame switching and allow you to switch to any other document in the content process,
while seeing chrome ressources in debugger.
About frame scripts, here is my findings, but someone with more Debugger/ThreadActor knowledges should take a look at that as I'm stuck on this.

Breakpoints don't work on frame script if the bp is set before the frame script is loaded.
(load a first tab, open debugger, set a bp in a frame script, open another tab, the bp won't break on the second tab whereas it does on the first)

After some discussions with ejpbruel, it looks like there might be something wrong with Debugger.onNewScript.
onNewScript is never called for frame script. I can see it called for some JSMs and content ressources.
The only way to reach frame scripts is Debugger.findScript. Such thing explains why the bp wouldn't be set when a new frame script is loaded. We are used to restore/apply bp on new scripts via the onNewScript callback.
I verified that even when calling Debugger.addDebugee on the frame script global, onNewScript isn't called.

I've also been told that the fact that frame script are more or less eval-ed might be an issue.
But I'm not sure it is that special. To start with, frame scripts do have an URL and aren't just anonymous scripts. Here is the special trick frame scripts are using in order to share the global without sharing the scope (more info in bug 673569):
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFrameMessageManager.cpp#1464

Another important point is that frame scripts instanciates multiple scripts with the same URL. I don't really know how it can mess up breakpoints?

I tried to workaround this issue without much success. But it seems to confirm that onNewScript is broken.
I have been able to break in new frame script by using Debugger.findScripts() and then restoring breakpoints.
*But* I had to hack this condition (by removing it): !bp.actor.scripts.length
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#2353
By removing it, I also end up with an unexpected amount of breakpoint on the same line, so it break manyyy times.
James, I have been told by Eddy that your work on Debugger.Source may help... any random idea? Is you patch ready somewhere to see if that automagically fixes that?

Jim, I thought you may also have some random guesses?
Flags: needinfo?(jlong)
Flags: needinfo?(jimb)
Attachment #8513860 - Attachment is obsolete: true
Attachment #8505179 - Attachment is obsolete: true
Attachment #8506342 - Attachment is obsolete: true
A followup of bug 1088247, we don't need all these target.isAddon everywhere.
Checking for actor available is, most of the time, enough.
But not always, so that I had to introduce target.isContentProcess :/
(In reply to Alexandre Poirot [:ochameau] from comment #66)
> James, I have been told by Eddy that your work on Debugger.Source may
> help... any random idea? Is you patch ready somewhere to see if that
> automagically fixes that?

Yep, there's big patch over at https://bugzilla.mozilla.org/show_bug.cgi?id=905700 that works. I haven't rebased it in a week or so though and think there's been a bunch of changes to tests that will conflict. I'm working on rebasing it today, so you can try it tomorrow if you'd like (or see if you get lucky today).

If the origin of the script is indeed `eval`, the frontend will never be notified of it, so it might fix the issue.
Flags: needinfo?(jlong)
(In reply to James Long (:jlongster) from comment #68)
> If the origin of the script is indeed `eval`, the frontend will never be
> notified of it, so it might fix the issue.

Frame scripts are evaluated with an URL, like modules evaled in sandboxes.
Attached patch ContentProcessActor v3 (obsolete) — — Splinter Review 
With a test.
Attachment #8514227 - Attachment is obsolete: true
(In reply to Alexandre Poirot [:ochameau] from comment #69)
> (In reply to James Long (:jlongster) from comment #68)
> > If the origin of the script is indeed `eval`, the frontend will never be
> > notified of it, so it might fix the issue.
> 
> Frame scripts are evaluated with an URL, like modules evaled in sandboxes.

Still might be worth trying it with my patch, it's generally a lot more accurate with sources. Currently rebasing it and the conflicts don't look too bad, should be done soon.
Bill, I have some question for you (or any other e10s expert looking at this bug!) in comment 63.
Flags: needinfo?(wmccloskey)
Comment on attachment 8514371 [details] [diff] [review]
Enable tools if related actor is available - v1

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

This patch removes the various target.isAddon being useless given that checking for the actor availability is enough.
And also continue the work being done in bug 1088247, by always checking for actor/trait instead of checking for target kind.
This patch also introduce the new checks needed for content process target, where we can't just check for trait/actor.
(If you have better idea on how to check for netmon/picker... I would really like to avoid such isAddon/isContentProcess/isSomething on targets!!)
Attachment #8514371 - Flags: review?(jryans)
Attached patch ContentProcessActor v4 (obsolete) — — Splinter Review 
Rebased after various merge conflicts.

I would like to continue tweaking this patch after some more feedback from
e10s expert, but I would like to also ear from devtools elders ;)
Feedback is welcomed from anyone!
I came up with a UX/UI that was easy to implement,
we may have a better way to expose this feature within existing firefox UI.

Note that this patch comes with 2 strings, the menuitem label and the window title.
Attachment #8514383 - Attachment is obsolete: true
Attachment #8515995 - Flags: feedback?(past)
Comment on attachment 8514371 [details] [diff] [review]
Enable tools if related actor is available - v1

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

We could add more traits to each actor that can be used as a target... then ContentProcessActor could say whether it specifically has a network monitor support, is highlightable, similar to |target.activeTab.traits.frames| today.  What do think about this?  (Probably best in a follow-up, but it would need to land the same release as this content process support to make much sense.)  The awkward part of course is that no older versions have such traits at all.  But it could help for the future at least.

I think you've done the best you can with the system we have today.
Attachment #8514371 - Flags: review?(jryans) → review+
Comment on attachment 8515995 [details] [diff] [review]
ContentProcessActor v4

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

::: browser/devtools/framework/target.js
@@ +349,5 @@
>      return !this.window;
>    },
>  
> +  get isContentProcess() {
> +    return !!(this._form && this._form.isContentProcess);

You could also check if the actor ID starts with "processActor".  If you prefer the boolean flag since it is more explicit, that seems fine too.
(In reply to Alexandre Poirot [:ochameau] from comment #63)
> - in the content process, is there any way to list all tab child globals or
> content windows?
> window-mediator doesn't throw, but doesn't enumerate any window either :/
> I can listen for some events to watch for new content windows being created,
> but that doesn't help listing existing ones, when we connect to the process.

I'd don't think there's any way to do that besides manually keeping the list yourself.

> - in the content process, do we have access to a unique ID for the process,
> like childID or something?
> From the parent we have a childID on the frame loader, but we don't have
> access to the frame loader from the content process... I could
> sendSyncMessage to fetch it from the parent, but that sounds so sad to not
> have the child id from the child!

There's no way to do this that I know of. I'd like to stay away from IDs though. There's usually a better way to do things.

> - parent process message manager appears to be quite good to implement this
> feature, but it is also frustrating to be limited to only message passing
> API. We can iterate over all message managers via childCount and
> getChildAt(), but given one message manager, we can't differenciate managers
> for documents being OOP or not. I can broadcast a message to all of them to
> figure this out, but that's really not handy... whereas the platform code
> surely knows if that remote or not. That would be really great to be able to
> differentiate OOP vs non-OOP somehow, with QueryInterface or an explicit
> attribute. (That would help me listing existing content processes and today,
> it would help me telling the user if we can spawn a content toolbox or not)

I can see that managing the list of tabs in a given process might be annoying. One idea would be to add a method to a <browser>'s message manager that would return the corresponding process message manager. Then you could iterate over tabs and group those that have the same process message manager. You could also exclude tabs where !browser.isRemoteBrowser. Would this help you? I don't think it would be too hard to implement.

> - finally, I had issues in choosing in which context, the console should be
> executed into. You may want to evaluate against the current tab child global
> (topmost, visible tab), or content window? But I didn't do that because it
> window-mediator doesn't work, and even if we can do that with various
> messages between parent and child, it sounds quite challenging to implement
> correctly. So I ended up crafting an empty chrome Sandbox, where you have to
> use Components.* to do something cool. We may keep this sandbox and expose
> some cool helpers to ease fetching tabchild globals/windows.

This seems fine.
Flags: needinfo?(wmccloskey)
Depends on: 1094203
Comment on attachment 8514371 [details] [diff] [review]
Enable tools if related actor is available - v1

Moved to bug 1094203.
Attachment #8514371 - Attachment is obsolete: true
Attached patch ContentProcessActor v5 (obsolete) — — Splinter Review 
Rebased, with some cleanup.
Feedback still welcomed!
Attachment #8515995 - Attachment is obsolete: true
Attachment #8515995 - Flags: feedback?(past)
Attachment #8517502 - Flags: feedback?(past)
Attachment #8517502 - Flags: feedback?(jryans)
(In reply to Bill McCloskey (:billm) from comment #77)
> (In reply to Alexandre Poirot [:ochameau] from comment #63)
> > - in the content process, is there any way to list all tab child globals or
> > content windows?
> > window-mediator doesn't throw, but doesn't enumerate any window either :/
> > I can listen for some events to watch for new content windows being created,
> > but that doesn't help listing existing ones, when we connect to the process.
> 
> I'd don't think there's any way to do that besides manually keeping the list
> yourself.

This is really missing, not only for this patch, only for another feature we have that allows to debug any frame living in the process of an app.
We can't keep a list as devtools codebase is loaded on-demand, and frames/windows/tabchilds can be created before devtools code kicks ine.
Do you think we could tweak the platform to expose all frames/windows/tabchilds?
Would it be easier to ensure that window-mediator works, or come up with something new and specific to TabChild API?


> I can see that managing the list of tabs in a given process might be
> annoying. One idea would be to add a method to a <browser>'s message manager
> that would return the corresponding process message manager. Then you could
> iterate over tabs and group those that have the same process message
> manager. You could also exclude tabs where !browser.isRemoteBrowser. Would
> this help you? I don't think it would be too hard to implement.

I think that would help doing more things and especially better handle more than just one content process. But in my case, what would help is figuring out which of the parent process message manager childs is the parent process message manager. (Parent process message manager always has one child, the message manager for tabs living in the parent process.) I need to distinguish it as I can't open a content toolbox for it.
Comment on attachment 8517502 [details] [diff] [review]
ContentProcessActor v5

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

The overall functionality seems nice!  The debugger finds browser-child.js, and I can debug it.

But what about add-ons?  My dev profile has some add-ons installed, but I did not notice any of their scripts in this debugger.  Maybe I don't have the right kind of add-on to test this?

See my RDP comments, I believe that is main issue with the current approach.

::: browser/devtools/framework/gDevTools.jsm
@@ +706,5 @@
> +    }
> +
> +    let transport = DebuggerServer.connectPipe();
> +    let conn = transport._serverConnection;
> +    let client = new DebuggerClient(transport);

This is kind of an odd dance to do here.

It seems that the current version has no way to get the content-process actor via an RDP request, right?  Isn't that something we want?

Comment 58 suggests that the content process actors would be children of the root actor, and I interpreted that to mean you would see them in the "listTabs" reply, possibly in some new array next to tabs.

If that is a bad idea for some reason, we could make this similar to getting an app actor, where we send a "getAppActor" message.

One way or another though, we should use the RDP to get the actor (that message handler would run |DebuggerServer.connectToContent|) instead of requiring local access to |DebuggerServer.connectToContent|.

::: toolkit/components/processsingleton/ContentProcessSingleton.js
@@ +24,5 @@
>      switch (topic) {
>      case "app-startup": {
>        Services.obs.addObserver(this, "console-api-log-event", false);
>        Services.obs.addObserver(this, "xpcom-shutdown", false);
> +      cpmm.addMessageListener("Devtools:InitDebuggerServer", this);

Nit: DevTools

@@ +56,5 @@
>  
>      case "xpcom-shutdown":
>        Services.obs.removeObserver(this, "console-api-log-event");
>        Services.obs.removeObserver(this, "xpcom-shutdown");
> +      cpmm.removeMessageListener("Devtools:InitDebuggerServer", this);

Nit: DevTools

::: toolkit/devtools/server/actors/child-process.js
@@ +23,5 @@
> +    findDebuggees: dbg => dbg.findAllGlobals(),
> +    shouldAddNewGlobalAsDebuggee: global => true
> +  });
> +
> +  // Bind the console to the current tab document

I don't think you are actually using the current tab, right?  It's just the empty sandbox you make below.

::: toolkit/devtools/server/content-server.jsm
@@ +24,5 @@
> +  // Init a custom, invisible DebuggerServer, in order to not pollute
> +  // the debugger with all devtools modules, nor break the debugger itself with using it
> +  // in the same process.
> +  let devtools = new DevToolsLoader();
> +  devtools.invisibleToDebugger = true;

You could also opt to refine the loader's condition[1].  But maybe it's less mysterious to just set it here.

[1]: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/Loader.jsm#423
Attachment #8517502 - Flags: feedback?(jryans)
It looks to me like almost every use of js::CloneScript (including JS::CloneAndExecuteScript and js::ExecuteInGlobalAndReturnScript, which nsFrameScriptExecutor::LoadFrameScriptInternal uses) fails to call Debugger's onNewScript hook to report the new script to the debugger.

I would definitely expect this to break breakpoints.
Flags: needinfo?(jimb)
Alex, is the onNewScript breakage still a problem?
Flags: needinfo?(poirot.alex)
As I'm reading this bug, there seem to be two essential questions:

- How should the various actors manage their Debuggers' globals? It seems that the debuggee global tracking techniques we've got at hand weren't correct for e10s chrome. It seems like that's been taken care of, with billm's help?

- How can Debugger find all the scripts it needs? I think can see how to fix onNewScript (and catch future such bugs with assertions).
(In reply to Jim Blandy :jimb from comment #83)
> Alex, is the onNewScript breakage still a problem?

Yes!

(In reply to Jim Blandy :jimb from comment #84)
> - How should the various actors manage their Debuggers' globals? It seems
> that the debuggee global tracking techniques we've got at hand weren't
> correct for e10s chrome. It seems like that's been taken care of, with
> billm's help?

Actually it works as-is with something very simple:
Dbg.onNewGlobal = g => dbg.addDebugee(g);

> 
> - How can Debugger find all the scripts it needs? I think can see how to fix
> onNewScript (and catch future such bugs with assertions).

Finding script at a given point in time works fine with dbg.findScript(). (That explain why breakpoints works on new resources if we remove and readd them)
That's really just about onNewScript issue... It prevents applying breakpoint on new script correctly.
If you have a quick patch I can easily confirm you of it really fix that issue.
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #85)
> Actually it works as-is with something very simple:
> Dbg.onNewGlobal = g => dbg.addDebugee(g);

Delightful! That's how it was supposed to work...


> That's really just about onNewScript issue... It prevents applying
> breakpoint on new script correctly.

Okay. I'm working on this.

Thank you for answering so late!
(In reply to Jim Blandy :jimb from comment #82)
> It looks to me like almost every use of js::CloneScript (including
> JS::CloneAndExecuteScript and js::ExecuteInGlobalAndReturnScript, which
> nsFrameScriptExecutor::LoadFrameScriptInternal uses) fails to call
> Debugger's onNewScript hook to report the new script to the debugger.
> 
> I would definitely expect this to break breakpoints.

I bet this is the cause behind a number of long-standing bugs in the debugger. Great catch Jim!
Use RDP from end to end, localize the UI (2 strings: window title, and alert message when there is no content process running).

Faster reviewer wins ;-) [But I wish both of you guys could provide feedback on this patch!]
Attachment #8517502 - Attachment is obsolete: true
Attachment #8517502 - Flags: feedback?(past)
Attachment #8518191 - Flags: review?(past)
Attachment #8518191 - Flags: review?(jryans)
(In reply to J. Ryan Stinnett [:jryans] from comment #81)
> Comment on attachment 8517502 [details] [diff] [review]
> But what about add-ons?  My dev profile has some add-ons installed, but I
> did not notice any of their scripts in this debugger.  Maybe I don't have
> the right kind of add-on to test this?

You need to have addons with frame scripts...
For example a random one:
https://addons.mozilla.org/fr/firefox/addon/cutyfox-url-shortener-googl-is/?src=search
You should see some js files in chrome://cutyfox/something...


> ::: toolkit/devtools/server/actors/child-process.js
> @@ +23,5 @@
> > +    findDebuggees: dbg => dbg.findAllGlobals(),
> > +    shouldAddNewGlobalAsDebuggee: global => true
> > +  });
> > +
> > +  // Bind the console to the current tab document
> 
> I don't think you are actually using the current tab, right?  It's just the
> empty sandbox you make below.

Right, forgot to update the comment.

> 
> ::: toolkit/devtools/server/content-server.jsm
> @@ +24,5 @@
> > +  // Init a custom, invisible DebuggerServer, in order to not pollute
> > +  // the debugger with all devtools modules, nor break the debugger itself with using it
> > +  // in the same process.
> > +  let devtools = new DevToolsLoader();
> > +  devtools.invisibleToDebugger = true;
> 
> You could also opt to refine the loader's condition[1].  But maybe it's less
> mysterious to just set it here.

I haven't really understood why we don't always set it to invisible so I prefered to only set it here...
Here's a shell test case that shows one instance of the onNewScript problem.


// Debugger should be notified of scripts created with ExecuteInGlobalAndReturnScope.

var g = newGlobal();
var dbg = new Debugger;
var DOg = dbg.addDebuggee(g);
var log = '';

dbg.onNewScript = function (outerEvalScript) {
  log += 'o';
  assertEq(outerEvalScript.url, "splunge.js");

  dbg.onNewScript = function (innerEvalScript) {
    log += 'i';
    assertEq(innerEvalScript.url, "splunge.js");
    assertEq(innerEvalScript.source.introductionScript, outerEvalScript);
    innerEvalScript.setBreakpoint(0, {
      hit(frame) {
        log += 'b';
        assertEq(frame.script, innerEvalScript);
      }
    });
  };
};

dbg.onDebuggerStatement = function (frame) {
  log += 'd';
};

assertEq(log, '');
var evalScope = g.evaluate('evalReturningScope("debugger; // nee");',
                           { fileName: "splunge.js" });
assertEq(log, 'oibd');
For the missing onNewScript notifications, I think this should catch the two paths I was able to find that clone scripts but don't report them to Debugger. It includes tests and some new support functions.
Attachment #8518342 - Flags: review?(wmccloskey)
Comment on attachment 8518342 [details] [diff] [review]
Report cloned scripts to Debugger.

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

Thanks very much.

::: js/src/builtin/Eval.cpp
@@ +477,5 @@
>      MOZ_ASSERT(global->is<GlobalObject>());
>  
>      RootedScript script(cx, scriptArg);
>      if (script->compartment() != cx->compartment()) {
>          script = CloneScript(cx, NullPtr(), NullPtr(), script);

It just occurred to me that maybe we could add a required parameter to CloneScript saying whether the script should be tracked by the debugger. That way any new callers would be more likely to be aware of the need.

::: js/src/builtin/TestingFunctions.cpp
@@ +2066,5 @@
> +        if (global)
> +            mac.emplace(cx, global);
> +
> +        // Use whatever global we've selected for the execution.
> +        global = JS::CurrentGlobalOrNull(cx);

It might be a little clearer to move this line up to the else branch of |if (global)| above. Then you could convert Maybe<AutoCompartment> to just AutoCompartment IIUC.
Attachment #8518342 - Flags: review?(wmccloskey) → review+
I've removed the Maybe; it is clearer without, and Maybe was only visible due to the unified build anyway.

I'm going to leave out the CloneScript argument. The code needed to report a new script is simplified by bug 1095145, and I think bug 1095159 (assert on executing unreported scripts) will do a better job of alerting people to the presence of a problem in the future.

New try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=952022e085c9
Attachment #8518342 - Attachment is obsolete: true
I've got a fix for the breakpoints-that-don't-get-hit issue in bug 1095216.
Attachment #8518564 - Flags: checkin+
Comment on attachment 8518191 [details] [diff] [review]
ContentProcessactor v6

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

Just a few nits, looks great!

I was instructed by Joe to land this myself, assuming you got r+.  I will now fix the nits and land it.

::: browser/devtools/framework/gDevTools.jsm
@@ +31,5 @@
>  const MAX_ORDINAL = 99;
>  
> +const bundle = Services.strings.createBundle("chrome://browser/locale/devtools/toolbox.properties");
> +
> +

Nit: extra line

@@ +707,5 @@
> +    let transport = DebuggerServer.connectPipe();
> +    let client = new DebuggerClient(transport);
> +
> +    let deferred = promise.defer();
> +    client.connect((aType, aTraits) => {

Unused params

::: toolkit/devtools/server/actors/child-process.js
@@ +49,5 @@
> +  get window() {
> +    return this._consoleScope;
> +  },
> +
> +  form: function BAA_form() {

Nit: Remove function name

@@ +74,5 @@
> +      },
> +    };
> +  },
> +
> +  disconnect: function BAA_disconnect() {

Nit: Remove function name

::: toolkit/devtools/server/main.js
@@ +727,5 @@
> +    let prefix = aConnection.allocID("content-process");
> +    let actor, childTransport;
> +
> +    aMm.addMessageListener("debug:content-process-actor", function listener(msg) {
> +      // Arbitrary choose the first content process to reply

Nit: Arbitrarily

::: toolkit/devtools/server/tests/mochitest/test_attachProcess.html
@@ +1,4 @@
> +<SDOCTYPv HTM.>
> +<html>
> +<!--
> +Bug 966991 - Test DebuggerServer.attachProcess

Nit: Wrong bug number
Attachment #8518191 - Flags: review?(past)
Attachment #8518191 - Flags: review?(jryans)
Attachment #8518191 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f108844f33ac
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
I assume we'll want to document this for add-on devs.
Keywords: dev-doc-needed
Thanks Ryan for fixing and landing the patch!
I've written a page for this: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Debugging_frame_scripts

I also made a couple of short videos:
https://dl.dropboxusercontent.com/u/91815060/browser-content-toolbox-open.mp4
https://dl.dropboxusercontent.com/u/91815060/browser-content-toolbox-using.mp4
Please let me know if they are OK. I could add highlighting and zooming and stuff to pick out e.g. menu items being selected, if you think it would help.

I could also add intro/outtro screens, although not sure what to use there, and suggestions welcome. There is some MDN branding I could use if we can't think of anything else.
Flags: needinfo?(jryans)
Flags: needinfo?(cpeterson)
(In reply to Will Bamberg [:wbamberg] from comment #105)
> I've written a page for this:
> https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/
> Debugging_frame_scripts
> 
> I also made a couple of short videos:
> https://dl.dropboxusercontent.com/u/91815060/browser-content-toolbox-open.mp4
> https://dl.dropboxusercontent.com/u/91815060/browser-content-toolbox-using.
> mp4
> Please let me know if they are OK. I could add highlighting and zooming and
> stuff to pick out e.g. menu items being selected, if you think it would help.
> 
> I could also add intro/outtro screens, although not sure what to use there,
> and suggestions welcome. There is some MDN branding I could use if we can't
> think of anything else.

Sorry, in case it's not obvious: the plan would be to embed these videos in the page, once we're happy with them.
(In reply to Will Bamberg [:wbamberg] from comment #105)
> I've written a page for this:
> https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/
> Debugging_frame_scripts
> 
> I also made a couple of short videos:
> https://dl.dropboxusercontent.com/u/91815060/browser-content-toolbox-open.mp4
> https://dl.dropboxusercontent.com/u/91815060/browser-content-toolbox-using.
> mp4
> Please let me know if they are OK. I could add highlighting and zooming and
> stuff to pick out e.g. menu items being selected, if you think it would help.

Great, this all looks good to me!  Thanks Will.

> I could also add intro/outtro screens, although not sure what to use there,
> and suggestions welcome. There is some MDN branding I could use if we can't
> think of anything else.

The videos seem fine to me as-is... though maybe we should have some kind of fancy Dev Edition intro these days.  Anyway, I'm getting off topic. :)
Flags: needinfo?(jryans)
Will, your videos look good. I don't think you need to change anything. :)
Flags: needinfo?(cpeterson)
Thanks! I've embedded the vids and am marking as ddc. I know it's gross that they are Flash, and am working on this.
Keywords: dev-doc-needed → dev-doc-complete
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: