Debugger and debuggee may see different scripts

VERIFIED FIXED in Firefox 18

Status

()

Firefox
Developer Tools: Debugger
P2
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mgoodwin, Assigned: fitzgen)

Tracking

(Blocks: 2 bugs)

15 Branch
Firefox 18
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 31 obsolete attachments)

23.38 KB, patch
past
: review+
Details | Diff | Splinter Review
9.17 KB, patch
past
: review+
Details | Diff | Splinter Review
16.70 KB, patch
past
: review+
Details | Diff | Splinter Review
7.72 KB, patch
past
: review+
Details | Diff | Splinter Review
7.43 KB, patch
Details | Diff | Splinter Review
2.49 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 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
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?
I don't think this is a K9O blocker, but feel free to reinstate if you have a different opinion.
No longer blocks: 676586

Comment 3

5 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*.
(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?
(Reporter)

Comment 5

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

Comment 6

5 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*.
Priority: -- → P2
Assignee: nobody → past
Status: NEW → ASSIGNED

Comment 7

5 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.
Depends on: 761723

Comment 8

5 years ago
Bug 637572 comment 22 links to a proposed API for providing the source that bug 761723 saves via Debugger.
Status: ASSIGNED → NEW
Priority: P2 → --
Priority: -- → P2
Status: NEW → ASSIGNED
Duplicate of this bug: 772119
Blocks: 772119
Apparently B2G started serving their resources with app: protocol URLs, so we definitely need this for B2G.
Depends on: 694539
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.
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.
Assignee: past → nfitzgerald
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.
Depends on: 783393
Created attachment 654024 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools, v1
Attachment #654024 - Flags: review?(past)
Attachment #654024 - Flags: feedback?(jwalker)
Created attachment 654026 [details] [diff] [review]
Part 2: Fetch sources from the server, send them over the protocol, v1
Attachment #654026 - Flags: review?(past)
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?
Attachment #654024 - Flags: review?(past)
Attachment #654024 - Flags: review?(jwalker)
Attachment #654024 - Flags: review+
Attachment #654024 - Flags: feedback?(jwalker)
Blocks: 784824
Ok, so this patch is broken. We need to update the protocol some more, will attach a new patch soon.
Created attachment 654888 [details] [diff] [review]
Part 2: Update protocol to fetch sources from the server side
Attachment #654026 - Attachment is obsolete: true
Attachment #654026 - Flags: review?(past)
Attachment #654888 - Flags: review?(past)
Attachment #654888 - Flags: review?(jimb)
Created attachment 654889 [details] [diff] [review]
Part 3: Debugger frontend uses protocol for sources instead of fetching them itself
Attachment #654889 - Flags: review?(past)
Try push: https://tbpl.mozilla.org/?tree=Try&rev=09b37a0c397b
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.
Attachment #654889 - Flags: review?(past)
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?
Attachment #654888 - Flags: review?(past)
(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.
Created attachment 656286 [details] [diff] [review]
Part 2: Interrupt and resume
Attachment #654888 - Attachment is obsolete: true
Attachment #654889 - Attachment is obsolete: true
Attachment #654888 - Flags: review?(jimb)
Attachment #656286 - Flags: review?(past)
Created attachment 656288 [details] [diff] [review]
Part 3: Sources over protocol
Attachment #656288 - Flags: review?(past)
Attachment #656288 - Flags: review?(jimb)
Created attachment 656289 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Attachment #656289 - Flags: review?(past)
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.
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 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 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 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.
Blocks: 786832
Blocks: 786834
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.
New try push: https://tbpl.mozilla.org/?tree=Try&rev=d1d3617b6610
Created attachment 656635 [details] [diff] [review]
Part 2: Interrupt and resume
Attachment #656286 - Attachment is obsolete: true
Attachment #656286 - Flags: review?(past)
Attachment #656635 - Flags: review?(past)
Created attachment 656636 [details] [diff] [review]
Part 3: Sources over protocol
Attachment #656288 - Attachment is obsolete: true
Attachment #656288 - Flags: review?(past)
Attachment #656288 - Flags: review?(jimb)
Attachment #656636 - Flags: review?(past)
Attachment #656636 - Flags: review?(jimb)
Created attachment 656637 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Attachment #656289 - Attachment is obsolete: true
Attachment #656289 - Flags: review?(past)
Attachment #656637 - Flags: review?(past)
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.
Attachment #654024 - Flags: review?(jwalker) → review+
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.
Attachment #656637 - Flags: review?(past) → review+
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 :-)
Attachment #656635 - Flags: review?(past) → review+
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.
Attachment #656636 - Flags: review?(past) → review+
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.
(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.
(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.
Blocks: 788754
Created attachment 659773 [details] [diff] [review]
Part 2: Interrupt and resume
Attachment #656635 - Attachment is obsolete: true
Created attachment 659774 [details] [diff] [review]
Part 3: Sources over protocol
Attachment #656636 - Attachment is obsolete: true
Attachment #656636 - Flags: review?(jimb)
Created attachment 659775 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Attachment #656637 - Attachment is obsolete: true
Created attachment 659776 [details] [diff] [review]
Part 5: Fix broken mochitests
Created attachment 659821 [details]
List of failing mochitests, and notes on how/why they fail
(Reporter)

Updated

5 years ago
Blocks: 750736
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.
Attachment #659773 - Attachment is obsolete: true
Attachment #659774 - Attachment is obsolete: true
Attachment #659775 - Attachment is obsolete: true
Attachment #659776 - Attachment is obsolete: true
Attachment #659821 - Attachment is obsolete: true
Created attachment 662627 [details] [diff] [review]
Part 3: Sources over protocol
Created attachment 662631 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Created attachment 662634 [details]
List of failing mochitests, and notes on how/why they fail
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.
Attachment #654024 - Attachment is obsolete: true
Created attachment 662771 [details] [diff] [review]
Part 2: Interrupt and resume

Fixed a bunch of mochitests
Attachment #662625 - Attachment is obsolete: true
Created attachment 662772 [details] [diff] [review]
Part 3: Sources over protocol
Attachment #662627 - Attachment is obsolete: true
Created attachment 662774 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Attachment #662631 - Attachment is obsolete: true
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
Attachment #662634 - Attachment is obsolete: true
(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!

Updated

5 years ago
Blocks: 792855
Blocks: 791323
Blocks: 785704
Blocks: 788578
Blocks: 788585

Updated

5 years ago
Blocks: 792524
Created attachment 664879 [details] [diff] [review]
Part 2: Add thread scoped long strings
Attachment #662771 - Attachment is obsolete: true
Attachment #662772 - Attachment is obsolete: true
Attachment #662774 - Attachment is obsolete: true
Attachment #662775 - Attachment is obsolete: true
Attachment #664879 - Flags: review?(past)
Created attachment 664880 [details] [diff] [review]
Part 3: Sources over protocol
Attachment #664880 - Flags: review?(past)
Created attachment 664881 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Attachment #664881 - Flags: review?(past)
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!
Try run: https://tbpl.mozilla.org/?tree=Try&rev=b30346fd26f9
Created attachment 665333 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools
Attachment #662770 - Attachment is obsolete: true
Attachment #664879 - Attachment is obsolete: true
Attachment #664880 - Attachment is obsolete: true
Attachment #664881 - Attachment is obsolete: true
Attachment #664879 - Flags: review?(past)
Attachment #664880 - Flags: review?(past)
Attachment #664881 - Flags: review?(past)
Attachment #665333 - Flags: review?(past)
Created attachment 665334 [details] [diff] [review]
Part 2: Add thread scoped long strings
Attachment #665334 - Flags: review?(past)
Created attachment 665335 [details] [diff] [review]
Part 3: Sources over protocol
Attachment #665335 - Flags: review?(past)
Created attachment 665336 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Attachment #665336 - Flags: review?(past)
Ok, all the tests are passing locally, can you push to try again, Panos? Thanks.
Try: https://tbpl.mozilla.org/?tree=Try&rev=87ea5c82ff2e
Created attachment 665561 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools
Attachment #665333 - Attachment is obsolete: true
Attachment #665333 - Flags: review?(past)
Attachment #665561 - Flags: review?(past)
Created attachment 665562 [details] [diff] [review]
Part 2: Add thread scoped long strings
Attachment #665334 - Attachment is obsolete: true
Attachment #665334 - Flags: review?(past)
Attachment #665562 - Flags: review?(past)
Created attachment 665563 [details] [diff] [review]
Part 3: Sources over protocol
Attachment #665335 - Attachment is obsolete: true
Attachment #665335 - Flags: review?(past)
Attachment #665563 - Flags: review?(past)
Created attachment 665564 [details] [diff] [review]
Part 4: Debugger frontend using the sources over protocol
Attachment #665336 - Attachment is obsolete: true
Attachment #665336 - Flags: review?(past)
Attachment #665564 - Flags: review?(past)
Sorry, same patches; no escape codes.
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.
Attachment #665561 - Flags: review?(past) → review+
Comment on attachment 665562 [details] [diff] [review]
Part 2: Add thread scoped long strings

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

Looks good.
Attachment #665562 - Flags: review?(past) → review+
Comment on attachment 665563 [details] [diff] [review]
Part 3: Sources over protocol

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

No changes since last time.
Attachment #665563 - Flags: review?(past) → review+
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.
Attachment #665564 - Flags: review?(past) → review+
Following this bug to know when these improvements arrive, so I can begin proper docs of b2g remote debugging.
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 :)
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
Attachment #667016 - Flags: review?(rcampbell)
I have verified that with the latest versions of these patches debugging works for both desktop b2g and my Nexus S.
Attachment #667016 - Flags: review?(rcampbell) → review+
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
Whiteboard: [fixed-in-fx-team]
Awesome! Thanks, Panos!

I did not know that you can have a guard in the catch clause with SpiderMonkey. That is very cool!
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Verified that that mgoodwin's STR from comment 0 now work as expected in the latest Fennec & Firefox nightlies.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.