I just noticed this on my 2013-12-05 Nightly on OS X... STR: 1) Open the Browser Toolbox (assuming you've enabled chrome and remote debugging) 2) Choose the Debugger pane, and pick some arbitrary point to set a breakpoint that you can trigger. I chose the Developer Tools widget's onViewShowing method in CustomizableWidgets.jsm. 3) Hit the breakpoint (in my case, I clicked on the Developer Tools widget) 4) In the Browser Debugger, click "Step over" to execute the next line of the script ER: Should execute the next line and break. AR: We seem to continue instead.

this might affect all remote debugging.

I think the actual results is "broken browser and debugger". I've experienced this a couple times and have had to quit (maybe even kill?) firefox.

I think I'm also experiencing this problem remote debugging Fennec chrome (on the lastest version of Fennec nightly). I'm seeing these errors in the browser console: this._contentWorker is null worker.js:549 notify event 'newSource' threw an exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.hostPort]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://browser/content/devtools/debugger-panes.js :: SourceUtils.trimUrl :: line 1229" data: no] DevToolsUtils.js:51 DebuggerClient.requester callback threw an exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.hostPort]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://browser/content/devtools/debugger-panes.js :: SourceUtils.trimUrl :: line 1229" data: no] DevToolsUtils.js:51 Also, it appears like everything just dies when we hit the breakpoint I set. The resume/step buttons are unresponsive, and eventually Fennec dies with an ANR.

(In reply to Nick Fitzgerald [:fitzgen] from comment #2) > I think the actual results is "broken browser and debugger". I've > experienced this a couple times and have had to quit (maybe even kill?) > firefox. I've had this happen a bunch of times, too. :-(

Going to see if I can get a regression window for this.

This regression was caused by https://hg.mozilla.org/mozilla-central/rev/ef84114446cc from bug 941012. Here's the STR I used while bisecting: 1. Go to mozilla.org 2. Open the (regular) debugger in that tab 3. Start the browser debugger / toolbox (this patch landed very close to this change) 4. In the browser debugger / toolbox, place a breakpoint just inside BreakpointStore.addBreakpoint 5. In the normal debugger, open some script from the page, such as tabzilla.js, and add a new breakpoint 6. In the browser debugger, you should now have hit your breakpoint 7. In the browser debugger, try to step over. Alex, any ideas on the cause here? That changeset involves many complexities. Perhaps this change prevents ToolboxProcess.jsm from getting a fresh DebuggerServer instance? Though it starts a new loader, so I would think it would get it's own instance still.

This is problematic enough that I think we should try to fix this on Aurora, too, if at all possible.

Alex, I tried the strategy you suggested to separate out the loader instances, but it hasn't fixed the behavior. The attached test verifies that we're now getting separate loaders (which was not happening before this patch), at least. I also verified that we get two separate compartments for server/main.js on about:memory. Not sure what to try next... I am hoping we'll come up with some new ideas tomorrow.

Comment on attachment 8348404 [details] [diff] [review] WIP v1 Review of attachment 8348404 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good. If I understand the following code and comment correctly, and especially if the comment is right about the fact that this.require may be undefined in all cases; It means that we were using the global loader to load protocol.js modules! https://hg.mozilla.org/mozilla-central/rev/ef84114446cc#l3.29 With my patch, we are now using current "require", that should always be available, even at top level. This require uses main.js loader, so if we manage to have distinct loader for main.js, we will now load new distinct protocol.js actor modules. Honestly, I'm just trying to describe what actually used to happen and what happens now. I have no clear idea what should be the various loaders behavior. I'd expect to load distinct module instances for everything (main.js, actors, ...) in case of chrome debugging. If that's not expected, that would be worth understanding why we have to instance protocol.js actor only once.

Chrome debugging expects all server code to be in the same compartment, otherwise stepping will pause inside debugger server code. This is why we were using loadSubscript to load actors and the promise library.

Oh right, that's assumption is completely broken then, each SDK module spawn its own Sandbox, and since the "Compartment Per Global" move, we can no longer share compartment across sandboxes. But we can share the same Zone, would that be enough? I hope so, otherwise it will bring a fundamental issue in using SDK module loader...

Alex, it's looking like we've got a path forward now with "invisibleToDebugger". However, implementing it will require various changes in different areas (XPConnect, Addon SDK, and Dev Tools), so it may take some time to land all the pieces. Does it make sense to back out the original changeset that caused the regression for now, until we have all these pieces landed? I would likely just back it out of 29, so that Fennec / Firefox devs are no longer impacted.

I'm fine with a backout as soon as we ensure that we gets something working on b2g after the backout.

Let's start by backing out the regressing changeset. I've verified locally that this fixes chrome debugging for Desktop, Android, and B2G continues to work too. Try: https://tbpl.mozilla.org/?tree=Try&rev=85d1256296c3

Try again for bc: https://tbpl.mozilla.org/?tree=Try&rev=f5acfd3a5e1b

Make the compartment option "invisibleToDebugger" available when creating a sandbox. In a later patch, I will make use of this in sandboxes that the add-on SDK's loader creates.

Can you explain why we want this? In general, invisibleToDebugger was intended only for scopes whose existence was an implementation detail of the embedding. I'm skeptical that it's actually something we want for scopes where we run script.

(In reply to Bobby Holley (:bholley) from comment #17) > Can you explain why we want this? In general, invisibleToDebugger was > intended only for scopes whose existence was an implementation detail of the > embedding. I'm skeptical that it's actually something we want for scopes > where we run script. For chrome debugging, the debugger gets confused if its own scripts live in multiple compartments, as it has no way to know what code is part of the debugger and what code is being debugged. I believe by default it only "ignores" the one compartment it started in. In the past, this did not matter, because we simply loaded everything into one compartment. However, a recent change was made that loads more of the Dev Tools server (which makes use of debugger instances) via the add-on SDK's loader, which creates a new compartment (via sandboxes) for each module that you tell it to load. This change to using the loader also improves compatibility with B2G, so I am reluctant to just revert it and stop there. To get chrome debugging working again while also making use of the add-on SDK's loader here (as we already do in much of the rest of the Dev Tools), I'd like to use this flag to mark all such Dev Tools compartments invisible so that chrome debugger doesn't get lost trying to debug itself (which causes it to behave erratically, as described in comment #0). If it still seems bad, please let me know if any alternatives come to mind!

Hm, ok. I'll defer to jimb on whether this is the right solution.

Landed part 0 (backing out the original regressing changeset) on fx-team: https://hg.mozilla.org/integration/fx-team/rev/89c1a04bede1 Once this makes it's way to m-c, chrome debugging for desktop & android should work again while we continue figuring out how to move forward here before reapplying this change.

Comment on attachment 8349772 [details] [diff] [review] Part 1: Expose invisibleToDebugger as a sandbox flag Canceling review to get this out of my queue. Please reflag me if jimb approves.

jryans and I discussed this tactic on IRC, and it seemed okay to me. I don't see anything structural about the invisibleToDebugger flag that would make it problematic to expand its use in this way. In the absence of better metadata on globals (as would be provided by bug 801084, say), this is probably a reasonable immediate solution.

Comment on attachment 8349772 [details] [diff] [review] Part 1: Expose invisibleToDebugger as a sandbox flag Review of attachment 8349772 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/tests/chrome/test_evalInSandbox.xul @@ +157,5 @@ > + try { > + let dbg = new Debugger(); > + let sandbox = new Cu.Sandbox(this, { invisibleToDebugger: false }); > + dbg.addDebuggee(sandbox); > + ok(true, "debugger couldn't add visible value"); This should be "could add visible value" @@ +168,5 @@ > + let sandbox = new Cu.Sandbox(this, { invisibleToDebugger: true }); > + dbg.addDebuggee(sandbox); > + ok(false, "debugger added invisible value"); > + } catch(e) { > + ok(true, "debugger added invisible value"); This should be "did not add invisible value"

Updated test messages from review comments. Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=dc63be8c7e89 Landed: https://hg.mozilla.org/integration/fx-team/rev/eb908c8e68eb

this is on central. Fixed?

(In reply to Rob Campbell [:rc] (:robcee) from comment #26) > this is on central. Fixed? No, more parts to come, which I plan to work on today. However, the regression is already fixed, since the bad change was undone as part 0. The rest of the work here is about relanding it safely.

woo!

* Ensures you get a new add-on SDK loader instance for each Dev Tools loader instance * Sets the invisibleToDebugger flag as needed (when we are doing chrome debugging)

This just the inverse of part 0, so it re-lands the patch from bug 941012 without modification, since it is now safe to do so with parts 1 - 3 in place. Figured it would be good for you to look over, just in case.

Comment on attachment 8355753 [details] Part 2: Expose invisibleToDebugger via the loader Looks good!

Commits pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/fc244acbeef73850cb8ff1ab7f60d8955e3bd6a6 Bug 946813 - Part 2: Expose invisibleToDebugger via the loader. r=jsantell https://github.com/mozilla/addon-sdk/commit/843dd486b54b4d1465f44248f07f18f48218057a Merge pull request #1332 from jryans/loader-itd Bug 946813 - Part 2: Expose invisibleToDebugger via the loader. r=@jsantell

Comment on attachment 8356229 [details] [diff] [review] Part 3: Use independent loaders, mark them invisible Review of attachment 8356229 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! I haven't tested this due to extra complexity of needing to sync with the addon sdk changes as well, but I assume you verified that both chrome and content debugging still work for fennec and b2g. ::: toolkit/devtools/Loader.jsm @@ -223,3 @@ > > DevToolsLoader.prototype = { > - _provider: null, I think it would make things slightly faster if you still initialized _provider to null in the constructor, so that the object shape wouldn't have to change down the road. ::: toolkit/devtools/tests/unit/test_independent_loaders.js @@ +3,5 @@ > + > +const { DevToolsLoader } = > + Cu.import("resource://gre/modules/devtools/Loader.jsm", {}); > + > +function run_test() { Could you add a brief comment about the purpose of this test please? ::: toolkit/devtools/tests/unit/xpcshell.ini @@ +2,5 @@ > head = head_devtools.js > tail = > > +[test_independent_loaders.js] > +[test_invisible_loader.js] Where does test_invisible_loader.js come from? I don't see it in this patch.

Carrying over Panos' r+ from attachment 8356229 [details] [diff] [review]. (In reply to Panos Astithas [:past] from comment #34) > Looks good to me! I haven't tested this due to extra complexity of needing > to sync with the addon sdk changes as well, but I assume you verified that > both chrome and content debugging still work for fennec and b2g. Yes, I've tested this on desktop, Fennec, and B2G. > > ::: toolkit/devtools/Loader.jsm > @@ -223,3 @@ > > > > DevToolsLoader.prototype = { > > - _provider: null, > > I think it would make things slightly faster if you still initialized > _provider to null in the constructor, so that the object shape wouldn't have > to change down the road. Makes sense, added it back. > ::: toolkit/devtools/tests/unit/test_independent_loaders.js > @@ +3,5 @@ > > + > > +const { DevToolsLoader } = > > + Cu.import("resource://gre/modules/devtools/Loader.jsm", {}); > > + > > +function run_test() { > > Could you add a brief comment about the purpose of this test please? Added. > ::: toolkit/devtools/tests/unit/xpcshell.ini > @@ +2,5 @@ > > head = head_devtools.js > > tail = > > > > +[test_independent_loaders.js] > > +[test_invisible_loader.js] > > Where does test_invisible_loader.js come from? I don't see it in this patch. Sigh... Should make hg warn when I leave out files or something... Anyway, it's there now.

Now waiting for part 2 (from addon SDK) to land in m-c. I hear this happens on Thursdays.

Tracking for 28 and as per comment 7 let's get this on Aurora as soon as possible.

Add-on SDK changes have been uplifted, so parts 3 and 4 can be landed now.

https://hg.mozilla.org/integration/fx-team/rev/e4a784a2c735 https://hg.mozilla.org/integration/fx-team/rev/937f571f4655

Backed out in https://hg.mozilla.org/integration/fx-team/rev/c12ab8a45990 and https://hg.mozilla.org/integration/fx-team/rev/9d410546acc3 for cross platform bustage in m-oth and xpcshell tests: https://tbpl.mozilla.org/php/getParsedLog.php?id=32841008&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=32840838&tree=Fx-Team

Missed a call to bind. I was so sure I had done a try run for this... :( Anyway, here we go: https://tbpl.mozilla.org/?tree=Try&rev=265aa41f3d5d

Looks like all issues are now resolved. Try: https://tbpl.mozilla.org/?tree=Try&rev=d7682b293880

https://hg.mozilla.org/integration/fx-team/rev/f5255c1b202f https://hg.mozilla.org/integration/fx-team/rev/d2c4548ed792

https://hg.mozilla.org/mozilla-central/rev/f5255c1b202f https://hg.mozilla.org/mozilla-central/rev/d2c4548ed792

Comment on attachment 8350446 [details] [diff] [review] Part 1 (v2): Expose invisibleToDebugger as a sandbox flag [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 941012 User impact if declined: Browser toolbox can't step through code. Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): Low String or IDL/UUID changes made by this patch: None

[Approval Request Comment] Bug caused by (feature/regressing bug #): bug 941012 User impact if declined: Browser toolbox can't step through code. Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): Low String or IDL/UUID changes made by this patch: None This patch will not be checked in directly, but will instead go through the add-on SDK's process for landing in Aurora. This attachment is only for approval.

Part 1: https://hg.mozilla.org/releases/mozilla-aurora/rev/7044a1139697 Now working with add-on SDK team to land part 2 for Aurora.

(In reply to J. Ryan Stinnett [:jryans] from comment #47) > Now working with add-on SDK team to land part 2 for Aurora. FWIW, I'm going to guess the answer is "just land the patch directly on Aurora." We typically don't do wholesale SDK updates on Aurora.

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #48) > (In reply to J. Ryan Stinnett [:jryans] from comment #47) > > Now working with add-on SDK team to land part 2 for Aurora. > > FWIW, I'm going to guess the answer is "just land the patch directly on > Aurora." We typically don't do wholesale SDK updates on Aurora. Others seem to approve of this plan: https://hg.mozilla.org/releases/mozilla-aurora/rev/76f4520b267d

Comment on attachment 8358787 [details] [diff] [review] Part 3 (v4): Use independent loaders, mark them invisible [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 941012 User impact if declined: Browser toolbox can't step through code. Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): Low String or IDL/UUID changes made by this patch: None

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #48) > (In reply to J. Ryan Stinnett [:jryans] from comment #47) > > Now working with add-on SDK team to land part 2 for Aurora. > > FWIW, I'm going to guess the answer is "just land the patch directly on > Aurora." We typically don't do wholesale SDK updates on Aurora. And we shouldn't but in this case we're primarily impacting the developers and their ability to debug so we will prioritize doing what it takes to fix this regression before Beta.

Part 3: https://hg.mozilla.org/releases/mozilla-aurora/rev/9b91c7e0aa11

Part 3 backed out from Aurora for xpcshell crashes. https://hg.mozilla.org/releases/mozilla-aurora/rev/e9358bde4d0f https://tbpl.mozilla.org/php/getParsedLog.php?id=33187475&tree=Mozilla-Aurora

It appears Aurora likes this version of part 3 better: https://tbpl.mozilla.org/?tree=Try&rev=461d3a6220cd Aside from B2G debug emulator issues, which seem unrelated, this looks better. Carrying over r+ and a+. Landing on Aurora soon.

Part 3: https://hg.mozilla.org/releases/mozilla-aurora/rev/009513120418

I reproduced the initial issue on the Nightly build from 2013-12-05. Verified as fixed on latest Aurora 28.0a2 (20140120004001) and latest Nightly 29.0a1 (20140120030202) under Win 7 64-bit, Ubuntu 32-bit and Mac OSX 10.6.8.