Mike Conley (:mconley) (:⚙️) Reporter
	Description • 11 years ago

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.

	Rob Campbell [:rc] (:robcee)
	Comment 1 • 11 years ago

this might affect all remote debugging.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 2 • 11 years ago

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.

	:Margaret Leibovic
	Comment 3 • 11 years ago

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.

	:Gijs (he/him)
	Comment 4 • 11 years ago

(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. :-(

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 5 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 6 • 11 years ago

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.

	:Gijs (he/him)
	Comment 7 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 8 • 11 years ago

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.

	Alexandre Poirot [:ochameau]
	Comment 9 • 11 years ago

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.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 10 • 11 years ago

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.

	Alexandre Poirot [:ochameau]
	Comment 11 • 11 years ago

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...

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 12 • 11 years ago

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.

	Alexandre Poirot [:ochameau]
	Comment 13 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 14 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 15 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 16 • 11 years ago

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.

	Bobby Holley (:bholley)
	Comment 17 • 11 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 18 • 11 years ago

(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!

	Bobby Holley (:bholley)
	Comment 19 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 20 • 11 years ago

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.

	Bobby Holley (:bholley)
	Comment 21 • 11 years ago

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.

	Jim Blandy :jimb
	Comment 22 • 11 years ago

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.

	Bobby Holley (:bholley)
	Comment 23 • 11 years ago

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"

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 24 • 11 years ago

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

	Carsten Book [:Tomcat]
	Comment 25 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/eb908c8e68eb

	Rob Campbell [:rc] (:robcee)
	Comment 26 • 11 years ago

this is on central. Fixed?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 27 • 11 years ago

(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.

	Rob Campbell [:rc] (:robcee)
	Comment 29 • 11 years ago

woo!

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 30 • 11 years ago

* 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)

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 31 • 11 years ago

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.

	Jordan Santell [:jsantell] [@jsantell]
	Comment 32 • 11 years ago

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

Looks good!

	[github robot]
	Comment 33 • 11 years ago

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

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 34 • 11 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 35 • 11 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 36 • 11 years ago

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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 37 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 38 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 11 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 40 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 41 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 42 • 11 years ago

Looks like all issues are now resolved.

Try: https://tbpl.mozilla.org/?tree=Try&rev=d7682b293880

	Ryan VanderMeulen [:RyanVM]
	Comment 43 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 45 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 46 • 11 years ago

[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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 47 • 11 years ago

Part 1: https://hg.mozilla.org/releases/mozilla-aurora/rev/7044a1139697

Now working with add-on SDK team to land part 2 for Aurora.

	Ryan VanderMeulen [:RyanVM]
	Comment 48 • 11 years ago

(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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 49 • 11 years ago

(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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 50 • 11 years ago

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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 51 • 11 years ago

(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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 52 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 53 • 11 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 54 • 11 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 55 • 11 years ago

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

	Petruta Horea [:phorea], Desktop QA
	Comment 56 • 11 years ago

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.