Mark Goodwin [:mgoodwin] Reporter
	Description • 12 years ago

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

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

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?

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

I don't think this is a K9O blocker, but feel free to reinstate if you have a different opinion.

	Jim Blandy :jimb
	Comment 3 • 12 years ago

I think we're going to just have the compiler retain the source code for every script it compiles. Then it'll be *accurate*.

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

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

	Mark Goodwin [:mgoodwin] Reporter
	Comment 5 • 12 years ago

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

	Jim Blandy :jimb
	Comment 6 • 12 years ago

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

	Jim Blandy :jimb
	Comment 7 • 12 years ago

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.

	Jim Blandy :jimb
	Comment 8 • 12 years ago

Bug 637572 comment 22 links to a proposed API for providing the source that bug 761723 saves via Debugger.

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

Apparently B2G started serving their resources with app: protocol URLs, so we definitely need this for B2G.

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

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.

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

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 13 • 12 years ago

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.

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

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?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 17 • 12 years ago

Ok, so this patch is broken. We need to update the protocol some more, will attach a new patch soon.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 20 • 12 years ago

Try push: https://tbpl.mozilla.org/?tree=Try&rev=09b37a0c397b

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

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.

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

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?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 23 • 12 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 27 • 12 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 28 • 12 years ago

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

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

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.

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

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! :-)

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

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 32 • 12 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 33 • 12 years ago

New try push: https://tbpl.mozilla.org/?tree=Try&rev=d1d3617b6610

	Joe Walker [:jwalker]
	Comment 37 • 12 years ago

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.

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

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.

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

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

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

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.

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

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 42 • 12 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 43 • 12 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 49 • 12 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 53 • 12 years ago

Not very different, just rebased since gcli doesn't use Promise.jsm anymore.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 54 • 12 years ago

Fixed a bunch of mochitests

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 57 • 12 years ago

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

	Rob Campbell [:rc] (:robcee)
	Comment 58 • 12 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 62 • 12 years ago

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!

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

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 68 • 12 years ago

Ok, all the tests are passing locally, can you push to try again, Panos? Thanks.

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

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 74 • 12 years ago

Sorry, same patches; no escape codes.

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

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.

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

Comment on attachment 665562 [details] [diff] [review]
Part 2: Add thread scoped long strings

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

Looks good.

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

Comment on attachment 665563 [details] [diff] [review]
Part 3: Sources over protocol

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

No changes since last time.

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

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.

	Eric Shepherd [:sheppy]
	Comment 79 • 12 years ago

Following this bug to know when these improvements arrive, so I can begin proper docs of b2g remote debugging.

	Victor Porof [:vporof][:vp]
	Comment 80 • 12 years ago

Part 4 needed a rebase for whatever reason, and I needed this in my queue :)

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

This fixes the Windows-only xpcshell test failure locally. Try run: https://tbpl.mozilla.org/?tree=Try&rev=68fc434538c9

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

I have verified that with the latest versions of these patches debugging works for both desktop b2g and my Nexus S.

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

https://hg.mozilla.org/integration/fx-team/rev/bdf2dd34d673
https://hg.mozilla.org/integration/fx-team/rev/65ce00cfa5e2
https://hg.mozilla.org/integration/fx-team/rev/055b04db33a1
https://hg.mozilla.org/integration/fx-team/rev/5fa320921d85
https://hg.mozilla.org/integration/fx-team/rev/ac84886e4d05

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 84 • 12 years ago

Awesome! Thanks, Panos!

I did not know that you can have a guard in the catch clause with SpiderMonkey. That is very cool!

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

https://hg.mozilla.org/mozilla-central/rev/bdf2dd34d673
https://hg.mozilla.org/mozilla-central/rev/65ce00cfa5e2
https://hg.mozilla.org/mozilla-central/rev/055b04db33a1
https://hg.mozilla.org/mozilla-central/rev/5fa320921d85
https://hg.mozilla.org/mozilla-central/rev/ac84886e4d05

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

Verified that that mgoodwin's STR from comment 0 now work as expected in the latest Fennec & Firefox nightlies.