Last Comment Bug 755661 - Debugger and debuggee may see different scripts
: Debugger and debuggee may see different scripts
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: 15 Branch
: x86 Mac OS X
: P2 normal (vote)
: Firefox 18
Assigned To: Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
:
Mentors:
http://www.computerist.org/debugme.php
Depends on: 694539 savesource 783393
Blocks: 785704 786832 788585 750736 772119 784824 786834 788578 788754 791323 792524 792855
  Show dependency treegraph
 
Reported: 2012-05-16 02:47 PDT by Mark Goodwin [:mgoodwin]
Modified: 2012-10-04 08:04 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Move Promise.jsm to toolkit/devtools, v1 (1.88 KB, patch)
2012-08-21 17:07 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: review+
jwalker: review+
Details | Diff | Review
Part 2: Fetch sources from the server, send them over the protocol, v1 (16.56 KB, patch)
2012-08-21 17:07 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 2: Update protocol to fetch sources from the server side (11.63 KB, patch)
2012-08-23 18:38 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 3: Debugger frontend uses protocol for sources instead of fetching them itself (4.02 KB, patch)
2012-08-23 18:39 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 2: Interrupt and resume (9.51 KB, patch)
2012-08-28 18:14 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 3: Sources over protocol (12.02 KB, patch)
2012-08-28 18:15 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 4: Debugger frontend using the sources over protocol (3.97 KB, patch)
2012-08-28 18:16 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 2: Interrupt and resume (9.52 KB, patch)
2012-08-29 15:17 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: review+
Details | Diff | Review
Part 3: Sources over protocol (12.05 KB, patch)
2012-08-29 15:18 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: review+
Details | Diff | Review
Part 4: Debugger frontend using the sources over protocol (4.83 KB, patch)
2012-08-29 15:19 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: review+
Details | Diff | Review
Part 2: Interrupt and resume (10.28 KB, patch)
2012-09-10 10:39 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 3: Sources over protocol (11.75 KB, patch)
2012-09-10 10:40 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 4: Debugger frontend using the sources over protocol (8.16 KB, patch)
2012-09-10 10:41 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 5: Fix broken mochitests (38.97 KB, patch)
2012-09-10 10:41 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
List of failing mochitests, and notes on how/why they fail (1.28 KB, text/plain)
2012-09-10 12:30 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details
Part 2: Interrupt and resume (91.38 KB, patch)
2012-09-19 11:22 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 3: Sources over protocol (11.87 KB, patch)
2012-09-19 11:22 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 4: Debugger frontend using the sources over protocol (5.81 KB, patch)
2012-09-19 11:23 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
List of failing mochitests, and notes on how/why they fail (6.92 KB, text/plain)
2012-09-19 11:24 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details
Part 1: Move Promise.jsm to toolkit/devtools (1.41 KB, patch)
2012-09-19 19:04 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 2: Interrupt and resume (145.50 KB, patch)
2012-09-19 19:05 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 3: Sources over protocol (11.87 KB, patch)
2012-09-19 19:05 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 4: Debugger frontend using the sources over protocol (5.81 KB, patch)
2012-09-19 19:06 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
List of failing mochitests, and notes on how/why they fail (1.43 KB, text/plain)
2012-09-19 19:08 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details
Part 2: Add thread scoped long strings (7.05 KB, patch)
2012-09-26 04:07 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 3: Sources over protocol (12.54 KB, patch)
2012-09-26 04:08 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 4: Debugger frontend using the sources over protocol (6.43 KB, patch)
2012-09-26 04:09 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 1: Move Promise.jsm to toolkit/devtools (32.95 KB, patch)
2012-09-27 03:45 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 2: Add thread scoped long strings (11.26 KB, patch)
2012-09-27 03:46 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 3: Sources over protocol (22.30 KB, patch)
2012-09-27 03:47 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 4: Debugger frontend using the sources over protocol (9.28 KB, patch)
2012-09-27 03:48 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Review
Part 1: Move Promise.jsm to toolkit/devtools (23.38 KB, patch)
2012-09-27 11:28 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: review+
Details | Diff | Review
Part 2: Add thread scoped long strings (9.17 KB, patch)
2012-09-27 11:29 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: review+
Details | Diff | Review
Part 3: Sources over protocol (16.70 KB, patch)
2012-09-27 11:30 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: review+
Details | Diff | Review
Part 4: Debugger frontend using the sources over protocol (7.72 KB, patch)
2012-09-27 11:31 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: review+
Details | Diff | Review
Part 4: Debugger frontend using the sources over protocol, rebased (7.43 KB, patch)
2012-10-01 13:41 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
Part 5: Fix xpcshell test on Windows (2.49 KB, patch)
2012-10-02 08:48 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Review

Description Mark Goodwin [:mgoodwin] 2012-05-16 02:47:37 PDT
Issue:
As the debugger fetches its own copies of scripts based on URLs provided by the debuggee, there are likely scenarios in which debugger and debuggee see different things.

This could occur if content being debugged differs according to application state (e.g. login) or user agent (relatively common for mobile-aware sites).

STR:
1) Connect desktop debugger to fennec debuggee
2) Visit http://www.computerist.org/debugme.php
3) Set a breakpoint on like 7
4) click 'clickme'
5) Observe that 2+2 is 5
Comment 1 Panos Astithas [:past] 2012-05-16 03:02:04 PDT
We could fix this by making the debugger server transfer the script source along with the rest of the data in 'newScript' and 'sripts' requests. We've already suggested that this would be useful for eval() scripts, whose source is often unavailable. Jim, do you think such a protocol spec change is reasonable?
Comment 2 Panos Astithas [:past] 2012-05-16 03:09:31 PDT
I don't think this is a K9O blocker, but feel free to reinstate if you have a different opinion.
Comment 3 Jim Blandy :jimb 2012-05-16 09:25:56 PDT
I think we're going to just have the compiler retain the source code for every script it compiles. Then it'll be *accurate*.
Comment 4 Panos Astithas [:past] 2012-05-17 22:34:25 PDT
(In reply to Jim Blandy :jimb from comment #3)
> I think we're going to just have the compiler retain the source code for
> every script it compiles. Then it'll be *accurate*.

Right, and the server needs to send it through the protocol connection to the client. Can we add a simple 'source' or 'text' property to the 'newScript' packet or do you want to have the client request it separately?
Comment 5 Mark Goodwin [:mgoodwin] 2012-05-21 11:32:08 PDT
(In reply to Panos Astithas [:past] from comment #4)
> (In reply to Jim Blandy :jimb from comment #3)
> > I think we're going to just have the compiler retain the source code for
> > every script it compiles. Then it'll be *accurate*.

Marvellous

> Right, and the server needs to send it through the protocol connection to
> the client. Can we add a simple 'source' or 'text' property to the
> 'newScript' packet or do you want to have the client request it separately?

I'm not worried about the security implications of an additional property; that's fine with me.
Comment 6 Jim Blandy :jimb 2012-05-21 11:43:00 PDT
(In reply to Panos Astithas [:past] from comment #4)
> Right, and the server needs to send it through the protocol connection to
> the client. Can we add a simple 'source' or 'text' property to the
> 'newScript' packet or do you want to have the client request it separately?

I think it needs to be sent separately, perhaps incrementally; source can be *large*.
Comment 7 Jim Blandy :jimb 2012-06-25 14:35:50 PDT
Benjamin Peterson has a patch that does the hard internal work for this in bug 761723; we just need to front it via Debugger and this can be fixed.
Comment 8 Jim Blandy :jimb 2012-07-06 13:41:04 PDT
Bug 637572 comment 22 links to a proposed API for providing the source that bug 761723 saves via Debugger.
Comment 9 Panos Astithas [:past] 2012-08-03 00:57:03 PDT
*** Bug 772119 has been marked as a duplicate of this bug. ***
Comment 10 Panos Astithas [:past] 2012-08-07 06:43:36 PDT
Apparently B2G started serving their resources with app: protocol URLs, so we definitely need this for B2G.
Comment 11 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-07 14:37:09 PDT
Panos proposed that we add a "source" property to the "scripts" and "newScript" packets of the protocol which is a long string in bug 772119 comment 14. Over IRC, jimb agreed that this was a good plan.

For this reason, I have added bug 694539 as a blocker.
Comment 12 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-16 11:56:38 PDT
Panos, I am taking this bug as we discussed on IRC the other day. I think I have a lot of this bug implemented already for adding source maps to the protocol, so I should be able to bust this out fairly quickly.
Comment 13 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-21 17:06:09 PDT
Ok, I have a version 1 on this patch, which is based on top of the v5 patch for bug 783393. I plan on rebasing on top of the latest patch for that bug soon.

Also, at the time I was writing this patch, the promises from the addon sdk which will replace our Promise.jsm weren't yet integrated in to the build (see bug 756542), so I still had to use Promise.jsm. They have since been integrated in to the build process.

I made a follow up bug to replace Promise.jsm with the new standard promise module in all of devtools: https://bugzilla.mozilla.org/show_bug.cgi?id=783420

Note for reviewers: I am not sure if the loading text is set in the right place in this patch.
Comment 14 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-21 17:07:01 PDT
Created attachment 654024 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools, v1
Comment 15 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-21 17:07:42 PDT
Created attachment 654026 [details] [diff] [review]
Part 2: Fetch sources from the server, send them over the protocol, v1
Comment 16 Panos Astithas [:past] 2012-08-22 06:03:55 PDT
Comment on attachment 654024 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools, v1

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

Joe should have the final word on this, but it looks fine to me as a temporary measure. Not sure if the move to toolkit requires superreview. Rob?
Comment 17 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-22 16:59:37 PDT
Ok, so this patch is broken. We need to update the protocol some more, will attach a new patch soon.
Comment 18 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-23 18:38:20 PDT
Created attachment 654888 [details] [diff] [review]
Part 2: Update protocol to fetch sources from the server side
Comment 19 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-23 18:39:23 PDT
Created attachment 654889 [details] [diff] [review]
Part 3: Debugger frontend uses protocol for sources instead of fetching them itself
Comment 20 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-23 18:41:15 PDT
Try push: https://tbpl.mozilla.org/?tree=Try&rev=09b37a0c397b
Comment 21 Panos Astithas [:past] 2012-08-27 03:54:33 PDT
Comment on attachment 654889 [details] [diff] [review]
Part 3: Debugger frontend uses protocol for sources instead of fetching them itself

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

::: browser/devtools/debugger/debugger-controller.js
@@ +1235,2 @@
>  
> +    this.activeThread.interrupt(function (aResponse) {

I'd prefer to have the conditional interruption be a part of SourceClient.source(), just like we do in setBreakpoint. Such protocol-related concerns should be left out of the frontend controller.

@@ +1248,4 @@
>  
> +        let sourceTextClient = this.activeThread.longString(aResponse.source);
> +        let length = sourceTextClient.length;
> +        sourceTextClient.substring(0, length, function (aResponse) {

It will be interesting to see if we can reduce jank by fetching the source code in chunks, and only update the visible part of the source editor. Maybe even the visible part plus one equally-sized window above and one below, to make scrolling snappier.

Of course this is followup material.

@@ +1256,5 @@
>  
> +          this._onLoadSourceFinished(script.url,
> +                                     aResponse.substring,
> +                                     null,
> +                                     options);

You also have to resume somewhere around here.
Comment 22 Panos Astithas [:past] 2012-08-27 06:13:15 PDT
Comment on attachment 654888 [details] [diff] [review]
Part 2: Update protocol to fetch sources from the server side

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

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +931,5 @@
> +  /**
> +   * Return an instance of SourceClient for the given actor.
> +   */
> +  source: function TC_source(aActor) {
> +    return new SourceClient(aActor, this._client);

It seems redundant to have a separate client just for one request, but I'm sure it will make more sense when we get Debugger.Source to back SourceActor with this as its front object.

@@ +1083,5 @@
>  /**
> + * A SourceClient provides a way to access the source text of a script.
> + *
> + * @param aActor String
> + *        The naome of the source actor.

You either meant "name" or "Naomi" here. In the latter case, please add pics.

@@ +1084,5 @@
> + * A SourceClient provides a way to access the source text of a script.
> + *
> + * @param aActor String
> + *        The naome of the source actor.
> + * @param aClient DebuugerClient

I can't think of any witty remark for DebuugerClient.

@@ +1096,5 @@
> +SourceClient.prototype = {
> +  /**
> +   * Get a long string grip for this SourceClient's source.
> +   */
> +  source: function SC_source(aCallback) {

Like I mentioned before, add the conditional interrupt here, please.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +924,5 @@
> +  sourceGrip: function TA_sourceGrip(aScript) {
> +    // TODO: Once we have Debugger.Source, this should be replaced with a
> +    // weakmap mapping Debugger.Source instances to SourceActor instances.
> +    if (!this.threadLifetimePool.sourceActors) {
> +      this.threadLifetimePool.sourceActors = {};

Using script URLs as keys will have the issues mentioned in bug 784112. We can address this issue in that bug though, and probably when we get Debugger.Source this will no longer be a problem.

@@ +1100,5 @@
> +   * Handler for the "source" packet.
> +   */
> +  onSource: function SA_onSource(aRequest) {
> +    if (this.state !== "paused") {
> +      throw new Error("Must be paused to respond to 'source' packets.");

How come you don't use PauseScopedActor.withPaused here?

@@ +1144,5 @@
> +    } catch (e) {
> +      // XXX: In the xpcshell tests, the script url is the absolute path of the
> +      // test file, which will make a malformed URI error be thrown.
> +      url = "file://" + url;
> +      scheme = Services.io.extractScheme(url);

I'm not sure if this will work on all platforms. See getFilePath() in toolkit/devtools/debugger/tests/unit/head_dbg.js if you get failures on Windows.

We also need to test if this works for the app:// URLs in B2G.

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +22,4 @@
>  Cu.import("resource://gre/modules/jsdebugger.jsm");
>  addDebuggerToGlobal(this);
>  
> +Cu.import("resource://gre/modules/devtools/Promise.jsm");

This should be _Promise.jsm.

@@ +27,4 @@
>  function dumpn(str) {
>    if (wantLogging) {
> +    if (str.length > 500) {
> +      str = str.substr(0, 500) + "...";

I... am not sure about that. I prefer that when we ask people to turn on logging, they send us complete logs. You never know where useful hints to trace that bug could be hidden.

::: toolkit/devtools/debugger/tests/unit/test_source-01.js
@@ +5,5 @@
> +var gDebuggee;
> +var gClient;
> +var gThreadClient;
> +
> +function run_test()

A comment indicating the purpose of the text would be nice.

@@ +58,5 @@
> +          s.init(f, -1, -1, false);
> +          let ss = Cc['@mozilla.org/scriptableinputstream;1'].createInstance(Ci.nsIScriptableInputStream);
> +          ss.init(s);
> +
> +          do_check_eq(ss.read(ss.available()), aResponse.substring);

I'm not sure if this is guaranteed to always work. MDN says not to trust available() even for the size of a local file and I think it's possible for read() to return without filling the supplied buffer, even though more data is available. dcamp should know.

@@ +60,5 @@
> +          ss.init(s);
> +
> +          do_check_eq(ss.read(ss.available()), aResponse.substring);
> +
> +          ss.close();

s.close() too?
Comment 23 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-28 15:36:19 PDT
(In reply to Panos Astithas [:past] from comment #22)
> Comment on attachment 654888 [details] [diff] [review]
> Part 2: Update protocol to fetch sources from the server side
> 
> Review of attachment 654888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/dbg-client.jsm
> @@ +931,5 @@
> > +  /**
> > +   * Return an instance of SourceClient for the given actor.
> > +   */
> > +  source: function TC_source(aActor) {
> > +    return new SourceClient(aActor, this._client);
> 
> It seems redundant to have a separate client just for one request, but I'm
> sure it will make more sense when we get Debugger.Source to back SourceActor
> with this as its front object.

Yes it should make more sense when we get Debugger.Source, and also I am going to add the source map related stuff to this actor.

> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +924,5 @@
> > +  sourceGrip: function TA_sourceGrip(aScript) {
> > +    // TODO: Once we have Debugger.Source, this should be replaced with a
> > +    // weakmap mapping Debugger.Source instances to SourceActor instances.
> > +    if (!this.threadLifetimePool.sourceActors) {
> > +      this.threadLifetimePool.sourceActors = {};
> 
> Using script URLs as keys will have the issues mentioned in bug 784112. We
> can address this issue in that bug though, and probably when we get
> Debugger.Source this will no longer be a problem.

Yeah I don't see how we can do anything until we have Debugger.Source.

> 
> @@ +1100,5 @@
> > +   * Handler for the "source" packet.
> > +   */
> > +  onSource: function SA_onSource(aRequest) {
> > +    if (this.state !== "paused") {
> > +      throw new Error("Must be paused to respond to 'source' packets.");
> 
> How come you don't use PauseScopedActor.withPaused here?

Good catch.

> 
> @@ +1144,5 @@
> > +    } catch (e) {
> > +      // XXX: In the xpcshell tests, the script url is the absolute path of the
> > +      // test file, which will make a malformed URI error be thrown.
> > +      url = "file://" + url;
> > +      scheme = Services.io.extractScheme(url);
> 
> I'm not sure if this will work on all platforms. See getFilePath() in
> toolkit/devtools/debugger/tests/unit/head_dbg.js if you get failures on
> Windows.
> 
> We also need to test if this works for the app:// URLs in B2G.

This is pretty much the exact source fetching code that we currently use in the debugger controller.

Hook me up with a B2G phone and I can do a lot of testing ;)

> @@ +27,4 @@
> >  function dumpn(str) {
> >    if (wantLogging) {
> > +    if (str.length > 500) {
> > +      str = str.substr(0, 500) + "...";
> 
> I... am not sure about that. I prefer that when we ask people to turn on
> logging, they send us complete logs. You never know where useful hints to
> trace that bug could be hidden.

Sorry, this shouldn't have been included in the patch. I just did this because the debugger was like 20x slower because I was dumping to an emacs buffer.

> 
> ::: toolkit/devtools/debugger/tests/unit/test_source-01.js
> @@ +5,5 @@
> > +var gDebuggee;
> > +var gClient;
> > +var gThreadClient;
> > +
> > +function run_test()
> 
> A comment indicating the purpose of the text would be nice.
> 
> @@ +58,5 @@
> > +          s.init(f, -1, -1, false);
> > +          let ss = Cc['@mozilla.org/scriptableinputstream;1'].createInstance(Ci.nsIScriptableInputStream);
> > +          ss.init(s);
> > +
> > +          do_check_eq(ss.read(ss.available()), aResponse.substring);
> 
> I'm not sure if this is guaranteed to always work. MDN says not to trust
> available() even for the size of a local file and I think it's possible for
> read() to return without filling the supplied buffer, even though more data
> is available. dcamp should know.

I am not sure how else to test the fetching of the sources. If anyone has any better ideas, I'm all ears.

> 
> @@ +60,5 @@
> > +          ss.init(s);
> > +
> > +          do_check_eq(ss.read(ss.available()), aResponse.substring);
> > +
> > +          ss.close();
> 
> s.close() too?

I was under the impression that ss.close() would also close s, but I think that was just something I assumed.

-------------------------

Now that I have finally got a patch for the interrupt/resume concurrency bug that we were talking about on IRC, I can fix up these patches again and upload with a new (hopefully green) try push.
Comment 24 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-28 18:14:45 PDT
Created attachment 656286 [details] [diff] [review]
Part 2: Interrupt and resume
Comment 25 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-28 18:15:58 PDT
Created attachment 656288 [details] [diff] [review]
Part 3: Sources over protocol
Comment 26 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-28 18:16:44 PDT
Created attachment 656289 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Comment 27 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-28 18:21:14 PDT
Try push:
https://tbpl.mozilla.org/?tree=Try&rev=1ad166a38651

Updated the patches based on Panos' feedback.

Added interrupt/resume patch to fix concurrency bugs where there are multiple interrupts and sequences of callbacks that depend on the paused state.

The last patch (part 4, integration with the frontend) is causing mochitest failures for me locally, and I haven't been able to debug them yet. I expect that they will also show up on the try push, however the rest of the patch queue has been green in my experience.
Comment 28 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-29 12:57:53 PDT
As expected, that push was pretty orange. Just to confirm what I believe, here is a push of everything except for (part 4, integration with the frontend) and if I am correct it should be green.

https://tbpl.mozilla.org/?tree=Try&rev=dff9af9a4086
Comment 29 Panos Astithas [:past] 2012-08-29 13:05:47 PDT
Comment on attachment 656286 [details] [diff] [review]
Part 2: Interrupt and resume

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

I didn't have time to look carefully over these patches today, but I'll post a few comments I have from a quick look.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +667,5 @@
> +      this.interrupt(function (aResponse, aToken) {
> +        if (aResponse.error) {
> +          aFunction(aResponse, null);
> +        }
> +        aFunction(null, token);

I think you mean aToken here.
Comment 30 Panos Astithas [:past] 2012-08-29 13:06:20 PDT
Comment on attachment 656288 [details] [diff] [review]
Part 3: Sources over protocol

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

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +22,4 @@
>  Cu.import("resource://gre/modules/jsdebugger.jsm");
>  addDebuggerToGlobal(this);
>  
> +Cu.import("resource://gre/modules/devtools/Promise.jsm");

You still haven't changed this to _Promise.jsm. In case you were wondering why your xpcshell tests fail, this is why! :-)
Comment 31 Panos Astithas [:past] 2012-08-29 13:06:39 PDT
Comment on attachment 656289 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol

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

::: browser/devtools/debugger/debugger-controller.js
@@ +1252,5 @@
> +        }
> +
> +        this._onLoadSourceFinished(script.url,
> +                                   aResponse.substring,
> +                                   null,

From now on the client cannot receive any content-type information, since the server doesn't send it. We should probably just remove the content type handling and always use the JavaScript highlighter. This is for a followup though.
Comment 32 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-29 15:14:17 PDT
Ok, so I was doing an incremental build for a long time, and it had been working without the underscore in "_Promise.jsm" so it makes sense that xpcshell tests were failing on try and not my local machine.

So here are the latest updates to these patches with your feedback.

Locally, xpcshell tests are still all passing (although I did a complete rebuild and fixed the things you pointed out) but mochitests are still bonkers.
Comment 33 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-29 15:14:40 PDT
New try push: https://tbpl.mozilla.org/?tree=Try&rev=d1d3617b6610
Comment 34 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-29 15:17:19 PDT
Created attachment 656635 [details] [diff] [review]
Part 2: Interrupt and resume
Comment 35 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-29 15:18:26 PDT
Created attachment 656636 [details] [diff] [review]
Part 3: Sources over protocol
Comment 36 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-08-29 15:19:37 PDT
Created attachment 656637 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Comment 37 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-09-02 03:39:11 PDT
Comment on attachment 654024 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools, v1

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

Sorry that took a while, that request got lost.
Comment 38 Panos Astithas [:past] 2012-09-05 08:28:31 PDT
Comment on attachment 656637 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol

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

::: browser/devtools/debugger/debugger-controller.js
@@ +1271,4 @@
>  
> +      let sourceTextClient = this.activeThread.longString(aResponse.source);
> +      let length = sourceTextClient.length;
> +      sourceTextClient.substring(0, length, function (aResponse) {

You are throwing away the initial part of the source that was already sent by the server. I'm OK with that for a v1, but can you add a TODO comment and file a followup for optimizing it?

Also this code expects source to always be a long string, however short scripts can be more efficiently transferred as plain strings. We could optimize this in the above followup bug as well.
Comment 39 Panos Astithas [:past] 2012-09-05 08:53:00 PDT
Comment on attachment 656635 [details] [diff] [review]
Part 2: Interrupt and resume

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

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +564,5 @@
> +
> +    if (this._outstandingInterrupts.size() > 0) {
> +      if (aLimit && this._resumeLimit && aLimit.type !== this._resumeLimit.type) {
> +        throw new Error("Already have a different resume limit.");
> +      }

Since we're bailing out of the call, we need to remove the resumption callback from the list before throwing.

@@ +565,5 @@
> +    if (this._outstandingInterrupts.size() > 0) {
> +      if (aLimit && this._resumeLimit && aLimit.type !== this._resumeLimit.type) {
> +        throw new Error("Already have a different resume limit.");
> +      }
> +      this._resumeLimit = aLimit;

You don't use this._resumeLimit when sending the packet. Shouldn't you set it even when this._outstandingInterrupts.size() == 0 and use it when sending the packet?

@@ +650,1 @@
>          aOnResponse(aResponse);

You forgot to return early here.

@@ +659,5 @@
> +   *
> +   * @param aFunction
> +   *        The function we will call when we are paused.
> +   */
> +  withPause: function TC_withPause(aFunction) {

There is also TC_doInterrupted for the exact same purpose, although I haven't gotten the chance to refactor the code to use it, yet. If you prefer yours, can you please grab a few good ideas from that one (this.paused, early return on error, but perhaps not resume afterwards) and then delete it?

@@ +677,5 @@
> +   * Creates a unique token that is used to keep track of how many sequences of
> +   * callbacks expect a paused state.
> +   */
> +  _createInterruptToken: function TC__createInterruptToken() {
> +    let token = this._interruptCounter++;

I'm sure that just like me, you thought "how likely is this counter to wrap around during a browser session?" and, again just like me, you answered "highly unlikely". That's good, because our agreement boosts my confidence in this :-)
Comment 40 Panos Astithas [:past] 2012-09-05 10:12:52 PDT
Comment on attachment 656636 [details] [diff] [review]
Part 3: Sources over protocol

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

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +209,5 @@
>    "resume": "resume",
>    "scripts": "scripts",
>    "setBreakpoint": "setBreakpoint",
> +  "substring": "substring",
> +  "source": "source"

This hunk doesn't apply, so you'll need to do some minor rebasing.

@@ +1157,5 @@
> + *        The ThreadClient for the active thread.
> + * @param aClient DebuggerClient
> + *        The debugger client parent.
> + */
> +function SourceClient(aActor, aActiveThread, aClient) {

You don't need all three arguments, just aActor and aClient, since you can get to the active thread from the client. Also, put aClient first, for consistency with the rest of the constructors.

@@ +1176,5 @@
> +      }
> +
> +      let packet = {
> +        to: this._actor,
> +        type: DebugProtocolTypes.source

This hunk also fails since DebugProtocolTypes is gone now.

@@ +1181,5 @@
> +      };
> +      this._client.request(packet, function (aResponse) {
> +        aCallback(aResponse, aToken);
> +      });
> +    }.bind(this));

As a library author, I would like us to provide a simpler API to consumers, that hides the complexity of long string handling. I don't think we need to do it in this bug though.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +1190,5 @@
> +   */
> +  onSource: PauseScopedActor.withPaused(function SA_onSource(aRequest) {
> +    this
> +      ._loadSource()
> +      .chainPromise(this._threadActor.longStringGrip.bind(this._threadActor))

You should use createValueGrip here, so that short scripts get transferred as plain strings.

@@ +1229,5 @@
> +    } catch (e) {
> +      // XXX: In the xpcshell tests, the script url is the absolute path of the
> +      // test file, which will make a malformed URI error be thrown.
> +      url = "file://" + url;
> +      scheme = Services.io.extractScheme(url);

I wonder if this is the reason you got xpcshell test failures only on Windows. If that turns out to be the case, see getFilePath() in toolkit/devtools/debugger/tests/unit/head_dbg.js for some ideas.
Comment 41 Panos Astithas [:past] 2012-09-05 10:15:40 PDT
I should add that I tried these patches on my Nexus S running B2G and they result in a working debugger! W00t!

Some bugs still remain, but it could be one of the points I've mentioned above. I haven't looked into why mochitests fail, but if after fixing the things I noticed, you still have trouble debugging it, I'll gladly help if I can.
Comment 42 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-05 10:40:27 PDT
(In reply to Panos Astithas [:past] from comment #41)
> I should add that I tried these patches on my Nexus S running B2G and they
> result in a working debugger! W00t!

AWESOME!

> Some bugs still remain, but it could be one of the points I've mentioned
> above. I haven't looked into why mochitests fail, but if after fixing the
> things I noticed, you still have trouble debugging it, I'll gladly help if I
> can.

Thanks, Panos! I'll update based on your feedback and see how things go.

I think a lot of the reason the mochitests have been failing is that they just don't expect to resume so soon because they aren't acquiring interrupt tokens and once the source is loaded since no one else has a token we just resume. I have fixed about a third of the mochitests but some are proving to be more subtle and difficult than others.

I need to clean up my fixes for the mochitests a little though, by making a part 5 which is just test fixes because they are cluttering up the other patches right now.
Comment 43 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-05 12:49:46 PDT
(In reply to Panos Astithas [:past] from comment #39)
> @@ +677,5 @@
> > +   * Creates a unique token that is used to keep track of how many sequences of
> > +   * callbacks expect a paused state.
> > +   */
> > +  _createInterruptToken: function TC__createInterruptToken() {
> > +    let token = this._interruptCounter++;
> 
> I'm sure that just like me, you thought "how likely is this counter to wrap
> around during a browser session?" and, again just like me, you answered
> "highly unlikely". That's good, because our agreement boosts my confidence
> in this :-)

I am glad that we agree! If you have made this counter large enough to even be close to max int, you have *much* bigger problems.
Comment 44 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-10 10:39:30 PDT
Created attachment 659773 [details] [diff] [review]
Part 2: Interrupt and resume
Comment 45 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-10 10:40:36 PDT
Created attachment 659774 [details] [diff] [review]
Part 3: Sources over protocol
Comment 46 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-10 10:41:13 PDT
Created attachment 659775 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Comment 47 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-10 10:41:55 PDT
Created attachment 659776 [details] [diff] [review]
Part 5: Fix broken mochitests
Comment 48 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-10 12:30:32 PDT
Created attachment 659821 [details]
List of failing mochitests, and notes on how/why they fail
Comment 49 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-19 11:22:22 PDT
Created attachment 662625 [details] [diff] [review]
Part 2: Interrupt and resume

Made some changes that dcamp suggested to me in person last week.

First, abstract over tokens with continuations for all consumers of the API outside of dbg-client.jsm.

Second, when we get an unsolicited pause (like a debugger statement or resume limit), we need to immediately grab a token so that we don't accidentally resume after doing something that was scoped to the paused state only (like fetching sources). Then we send an "unsolicitedPause" event from |onPacket| which has a continuation that the debugger frontend can call when the user actually wants to step/resume/etc.

All xpcshell tests are passing, but mochitests are failing still.
Comment 50 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-19 11:22:59 PDT
Created attachment 662627 [details] [diff] [review]
Part 3: Sources over protocol
Comment 51 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-19 11:23:39 PDT
Created attachment 662631 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Comment 52 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-19 11:24:15 PDT
Created attachment 662634 [details]
List of failing mochitests, and notes on how/why they fail
Comment 53 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-19 19:04:04 PDT
Created attachment 662770 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools

Not very different, just rebased since gcli doesn't use Promise.jsm anymore.
Comment 54 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-19 19:05:06 PDT
Created attachment 662771 [details] [diff] [review]
Part 2: Interrupt and resume

Fixed a bunch of mochitests
Comment 55 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-19 19:05:35 PDT
Created attachment 662772 [details] [diff] [review]
Part 3: Sources over protocol
Comment 56 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-19 19:06:07 PDT
Created attachment 662774 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Comment 57 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-19 19:08:24 PDT
Created attachment 662775 [details]
List of failing mochitests, and notes on how/why they fail

Only one test is actually failing: browser_dbg_breakpoint-new-script.js

Haven't really dug in yet, but I have some small notes.

Other than that, there are also three tests that pass all their assertions and don't timeout or anything, but they all log the same error to the console with Cu.reportError. This is something that should probably be looked in to :P
Comment 58 Rob Campbell [:rc] (:robcee) 2012-09-20 08:19:06 PDT
(In reply to Nick Fitzgerald [:fitzgen] from comment #57)
> Created attachment 662775 [details]
> List of failing mochitests, and notes on how/why they fail
> 
> Only one test is actually failing: browser_dbg_breakpoint-new-script.js

hopefully failing because the test doesn't make sense anymore. Nice work!

> Haven't really dug in yet, but I have some small notes.
> 
> Other than that, there are also three tests that pass all their assertions
> and don't timeout or anything, but they all log the same error to the
> console with Cu.reportError. This is something that should probably be
> looked in to :P

Certainly should be looked into. Might be possible to land with this and fix the error in a followup. Very exciting!
Comment 59 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-26 04:07:45 PDT
Created attachment 664879 [details] [diff] [review]
Part 2: Add thread scoped long strings
Comment 60 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-26 04:08:25 PDT
Created attachment 664880 [details] [diff] [review]
Part 3: Sources over protocol
Comment 61 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-26 04:09:11 PDT
Created attachment 664881 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Comment 62 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-26 04:09:58 PDT
Panos, can you please do a try push as well? My account has been locked and I need to switch it back over to a non mozilla.com email address. Thanks!
Comment 63 Panos Astithas [:past] 2012-09-26 05:42:04 PDT
Try run: https://tbpl.mozilla.org/?tree=Try&rev=b30346fd26f9
Comment 64 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-27 03:45:15 PDT
Created attachment 665333 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools
Comment 65 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-27 03:46:38 PDT
Created attachment 665334 [details] [diff] [review]
Part 2: Add thread scoped long strings
Comment 66 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-27 03:47:15 PDT
Created attachment 665335 [details] [diff] [review]
Part 3: Sources over protocol
Comment 67 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-27 03:48:21 PDT
Created attachment 665336 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Comment 68 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-27 03:49:29 PDT
Ok, all the tests are passing locally, can you push to try again, Panos? Thanks.
Comment 69 Panos Astithas [:past] 2012-09-27 04:59:07 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=87ea5c82ff2e
Comment 70 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-27 11:28:35 PDT
Created attachment 665561 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools
Comment 71 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-27 11:29:31 PDT
Created attachment 665562 [details] [diff] [review]
Part 2: Add thread scoped long strings
Comment 72 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-27 11:30:24 PDT
Created attachment 665563 [details] [diff] [review]
Part 3: Sources over protocol
Comment 73 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-27 11:31:07 PDT
Created attachment 665564 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Comment 74 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-09-27 11:31:30 PDT
Sorry, same patches; no escape codes.
Comment 75 Panos Astithas [:past] 2012-09-27 11:49:48 PDT
Comment on attachment 665561 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools

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

No changes since our last review.
Comment 76 Panos Astithas [:past] 2012-09-28 08:52:21 PDT
Comment on attachment 665562 [details] [diff] [review]
Part 2: Add thread scoped long strings

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

Looks good.
Comment 77 Panos Astithas [:past] 2012-09-28 08:54:59 PDT
Comment on attachment 665563 [details] [diff] [review]
Part 3: Sources over protocol

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

No changes since last time.
Comment 78 Panos Astithas [:past] 2012-09-28 09:07:41 PDT
Comment on attachment 665564 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol

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

Looks good.
Comment 79 Eric Shepherd [:sheppy] 2012-10-01 07:19:13 PDT
Following this bug to know when these improvements arrive, so I can begin proper docs of b2g remote debugging.
Comment 80 Victor Porof [:vporof][:vp] 2012-10-01 13:41:39 PDT
Created attachment 666684 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol, rebased

Part 4 needed a rebase for whatever reason, and I needed this in my queue :)
Comment 81 Panos Astithas [:past] 2012-10-02 08:48:38 PDT
Created attachment 667016 [details] [diff] [review]
Part 5: Fix xpcshell test on Windows

This fixes the Windows-only xpcshell test failure locally. Try run: https://tbpl.mozilla.org/?tree=Try&rev=68fc434538c9
Comment 82 Panos Astithas [:past] 2012-10-03 03:02:38 PDT
I have verified that with the latest versions of these patches debugging works for both desktop b2g and my Nexus S.
Comment 84 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-10-03 11:40:06 PDT
Awesome! Thanks, Panos!

I did not know that you can have a guard in the catch clause with SpiderMonkey. That is very cool!
Comment 86 Panos Astithas [:past] 2012-10-04 08:04:57 PDT
Verified that that mgoodwin's STR from comment 0 now work as expected in the latest Fennec & Firefox nightlies.

Note You need to log in before you can comment on or make changes to this bug.