772119 - expose source mapped sources over the remote debugging protocol

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

If we let the server fetch the source map for us and return the JSON blob that SourceMapConsumer accepts, then we can localize the logic for fetching a source map to one location. This will help with integrating source maps in to the webconsole, as well as integrating with the debugger.

This contrasts with simply returning the URL for the source map, which will force the debugger and the webconsole to implement the fetching logic.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Nick Fitzgerald :fitzgen from comment #1)
> If we let the server fetch the source map for us and return the JSON blob
> that SourceMapConsumer accepts, then we can localize the logic for fetching
> a source map to one location. This will help with integrating source maps in
> to the webconsole, as well as integrating with the debugger.
> 
> This contrasts with simply returning the URL for the source map, which will
> force the debugger and the webconsole to implement the fetching logic.

This would also alleviate the user-agent filtering problem described in bug 771597 comment 4.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #2)
> This would also alleviate the user-agent filtering problem described in bug
> 771597 comment 4.

Well, it would alleviate the issue for the source map itself, but not the sources linked to from the source map.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Nick Fitzgerald :fitzgen from comment #3)
> (In reply to Panos Astithas [:past] from comment #2)
> > This would also alleviate the user-agent filtering problem described in bug
> > 771597 comment 4.
> 
> Well, it would alleviate the issue for the source map itself, but not the
> sources linked to from the source map.

Yes, that is true.

Comment 5

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald :fitzgen from comment #1)
> If we let the server fetch the source map for us and return the JSON blob
> that SourceMapConsumer accepts, then we can localize the logic for fetching
> a source map to one location. This will help with integrating source maps in
> to the webconsole, as well as integrating with the debugger.

Just so I can follow the conversation:

In the comment I quoted, you're arguing for having the debug server fetch and parse the source map.

In bug 771597 comment 4, you're arguing for having the debug client fetch and parse the source code.

Right?

> This contrasts with simply returning the URL for the source map, which will
> force the debugger and the webconsole to implement the fetching logic.

I don't think this argument really applies; the debugger UI and webconsole can happily share a JSM that does the work. It's all one project.

Is it okay to have (say) a phone debuggee fetching resources like source maps and unminified source code?

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jim Blandy :jimb from comment #5)
> Just so I can follow the conversation:
> 
> In the comment I quoted, you're arguing for having the debug server fetch
> and parse the source map.
> 
> In bug 771597 comment 4, you're arguing for having the debug client fetch
> and parse the source code.
> 
> Right?

I'm bouncing back and forth :) I can still be convinced either way, but I am starting to lean back towards having the server fetch these things.


> > This contrasts with simply returning the URL for the source map, which will
> > force the debugger and the webconsole to implement the fetching logic.
> 
> I don't think this argument really applies; the debugger UI and webconsole
> can happily share a JSM that does the work. It's all one project.

True, indeed.

> Is it okay to have (say) a phone debuggee fetching resources like source
> maps and unminified source code?

I am ignorant of any reasons why this wouldn't be ok.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

It seems that everyone I have talked to lately supports having the server in charge of fetching source maps and the original sources pointed to from the source maps. In light of that, should there even be a "sourceMap" property on the "scripts" and "newScript" packets? Instead, should we just extend the protocol to support fetching these original sources on demand?

If you guys agree, should I just hijack this bug, or close it and create another?

Comment 8

[Jim Blandy :jimb]

Yes, let's shoot for having the server handle sourcemaps itself, and instead extend the protocol to retrieve sources. Hijacking this bug is fine.

Comment 9

[Rob Campbell [:rc] (:robcee)]

Yup. Agree fully.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

We already have bug 755661 for that.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Hmm, I was probably a little too hasty. You probably want a bug for the actual source map fetching that will result in scripts to be transferred via the support added in bug 755661.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #11)
> Hmm, I was probably a little too hasty. You probably want a bug for the
> actual source map fetching that will result in scripts to be transferred via
> the support added in bug 755661.

Exactly.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(Oops, didn't mean to change the status to NEW, but REOPENED isn't appearing in the dropdown anymore...)

Jim also mentioned in IRC that we don't necessarily need to wait on Debugger.Source before adding at least the source mapped source fetching support to the protocol: the clients don't care what APIs were used to create the packet they receive, just that they get the packet. So as long as the protocol is a sane abstraction, we can always change how we implement the protocol on the server side once Debugger.Source comes.

With that in mind, does anyone have any proposals on what the protocol for grabbing sources should look like? Would we want to just send the whole source along with the "scripts" and "newScript" packets? Or would we want to have the client explicitly request the source for a specific script?

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Nick Fitzgerald :fitzgen from comment #13)
> With that in mind, does anyone have any proposals on what the protocol for
> grabbing sources should look like? Would we want to just send the whole
> source along with the "scripts" and "newScript" packets? Or would we want to
> have the client explicitly request the source for a specific script?

The current thinking in bug 755661 is to add a "source" property to "newScript" and "scripts" response packets and also to avoid sending the script source with every such packet. The latter can be done with either a new "getSource" request to the thread actor, or by putting sources in the above packets as Long Strings (https://wiki.mozilla.org/Remote_Debugging_Protocol#Long_Strings), which would require bug 694539. Long strings are transferred in chunks and we can limit the initial chunk size to zero, for scripts that aren't yet shown in the editor.

Comment 15

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #14)
> The current thinking in bug 755661 is to add a "source" property to
> "newScript" and "scripts" response packets and also to avoid sending the
> script source with every such packet. The latter can be done with either a
> new "getSource" request to the thread actor, or by putting sources in the
> above packets as Long Strings
> (https://wiki.mozilla.org/Remote_Debugging_Protocol#Long_Strings), which
> would require bug 694539. Long strings are transferred in chunks and we can
> limit the initial chunk size to zero, for scripts that aren't yet shown in
> the editor.

Using the long string mechanism for sources seems like a great idea.

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

filter on chocolate

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

CC'ing Joe Walker because I plan on using Promise.jsm in the patch for this bug, which would require moving it to toolkit/devtools so we don't break Thunderbird (like we did with Require.jsm).

Anything I should be aware of here, Joe?

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Move Promise.jsm to toolkit/devtools

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Related to part 1: https://github.com/mozilla/gcli/pull/59

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 v6 patch for that bug soon.

I am not sure if the loading text is set in the right place.

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Move Promise.jsm to toolkit/devtools, v2

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Fetch sources from the server, send them over the protocol, v1

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ignore everything from comment 20 through this comment, wrong bug >_<

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Splitting out the source mapping of frame locations to bug 840684 since it is the trickier part and I would like to land the basic support as soon as possible.

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: wip

Rebased the WIP patch I have going and uploading it so that Panos can take a look.

Comment 27

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v1

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

Comment 28

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 725080 [details] [diff] [review]
v1

Known issues with this patch:

* setting breakpoints is wocky

* the green current location stepping arrow thingy is missing

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #28)
> * setting breakpoints is wocky

I found a bug in the way this patch modifies breakpoints. It sets the |actualLocation| packet property even when the line number is not changed, which confuses the frontend and causes it to delete the breakpoint.

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v2

Rebased on top of Eddy's patch, which has fixed the disappearing breakpoint issue. General wonkiness is still there though.

Comment 31

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hey Panos, I'm getting two test failures using your new rebased patch, but the logs don't really tell me anything. Here are the failures:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug740825_conditional-breakpoints-01.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_source_maps-01.js | Test timed out


Will look into it, but if you know what's up, let me know.

Comment 32

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v3

All tests passing again, and expanded the test to make sure that `actualLocation` works correctly.

Now need to make sure that the stepper works right and add tests for that.

Comment 33

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v4

OMG OMG OMG OMG OMG OMG

Caret works. Added caret positioning to test.

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

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 726989 [details] [diff] [review]
v4

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

Needs rebasing due to bug 852860.

Comment 35

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v5

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

Rebased!

Comment 36

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 727289 [details] [diff] [review]
v5

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

Nothing really major, looks good.
I think that the reason your tests fail on B2G may be that they use app://foo/bar URLs there.

::: browser/devtools/debugger/test/Makefile.in
@@ +96,5 @@
>  	browser_dbg_bfcache.js \
>  	browser_dbg_progress-listener-bug.js \
>  	browser_dbg_chrome-debugging.js \
> +	browser_dbg_source_maps-01.js \
> +	$(filter disabled-for-intermittent-failures--bug-753225, browser_dbg_createRemote.js) \

You are adding a duplicate entry for the createRemote test here.

::: browser/devtools/debugger/test/browser_dbg_bug731394_editor-contextmenu.js
@@ +66,5 @@
>  
>      is(scripts.itemCount, 2,
>        "Found the expected number of scripts.");
>  
> +    dump("\n\n\n\n\n\n\n\n\n\n\n" + editor.getText() + "\n\n\n\n\n\n\n\n\n\n\n\n");

A couple of forgotten dump() calls here and below.

::: browser/devtools/debugger/test/browser_dbg_source_maps-01.js
@@ +95,5 @@
> +      waitForCaretPos(4, testStepping);
> +    });
> +
> +    // This will cause the breakpoint to be hit, and put us back in the paused
> +    // stated.

Typo.

::: toolkit/devtools/debugger/server/dbg-browser-actors.js
@@ +48,5 @@
>        from: "root",
>        applicationType: "browser",
>        traits: {
> +        sources: true,
> +        sourceMaps: true,

Do we use this new trait anywhere in the code? We shouldn't add it if we don't have any use for it.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +211,5 @@
>      this._state = "attached";
>  
> +    this._options = {
> +      useSourceMaps: false
> +    };

The constructor seems like a better fit for this, no?

@@ +1960,5 @@
>     */
>    hit: function BA_hit(aFrame) {
>      // TODO: add the rest of the breakpoints on that line (bug 676602).
>      let reason = { type: "breakpoint", actors: [ this.actorID ] };
> +    this.threadActor._pauseAndRespond(aFrame, reason, function (aPacket) {

_pauseAndRespond and _nest return resumption values, so you should still return the value of _pauseAndRespond here.

@@ +2317,5 @@
> +  /**
> +   * Add a source to the current set of sources.
> +   *
> +   * Right now this takes a URL, but in the future it should
> +   * take a Debugger.Source. See bug XXXXXX.

Don't forget to file that bug.

@@ +2337,5 @@
> +    this._sourceActors[aURL] = actor;
> +    try {
> +      this._onNewSource(actor);
> +    } catch (e) {
> +      Cu.reportError(e);

dumpn() too, please.

@@ +2356,5 @@
> +        return [
> +          this.source(s) for (s of aSourceMap.sources)
> +        ];
> +      }.bind(this), function (e) {
> +        Cu.reportError(e);

Here, too.

@@ +2452,5 @@
> +        }.bind(this));
> +    }
> +
> +    // No source map
> +    return resolve({

Could you add that comment in getOriginalLocation, too?

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +35,2 @@
>      dump("DBG-SERVER: " + str + "\n");
> +//  }

Uncomment these.
How come you don't toggle the pref while testing instead?

::: toolkit/devtools/debugger/tests/unit/test_profiler_actor.js
@@ +161,2 @@
>          do_check_true(Profiler.IsActive());
> +        finishClient(client);

finishClient will also call do_test_finished(), potentially racing DebuggerServer._connectionClosed above. You could just use client.close() if you must do some cleanup here :-)

::: toolkit/devtools/debugger/tests/unit/test_sourcemaps-03.js
@@ +46,5 @@
> +      do_check_eq(aResponse.actualLocation.line, 4);
> +      do_check_eq(aResponse.actualLocation.url,
> +                  "http://example.com/www/js/" + aName + ".js");
> +
> +      // The eval will cause us to resume, then we get and unsolicited pause

Typo.

Comment 37

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #36)
> Comment on attachment 727289 [details] [diff] [review]
> v5
> 
> Review of attachment 727289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nothing really major, looks good.
> I think that the reason your tests fail on B2G may be that they use
> app://foo/bar URLs there.

I don't follow. All of the tests should still specify http://example.com/... urls because they are being source mapped, and using Cu.evalInSandbox. Not sure what is going on here, please enlighten me as to how to fix this.

> ::: toolkit/devtools/debugger/server/dbg-browser-actors.js
> @@ +48,5 @@
> >        from: "root",
> >        applicationType: "browser",
> >        traits: {
> > +        sources: true,
> > +        sourceMaps: true,
> 
> Do we use this new trait anywhere in the code? We shouldn't add it if we
> don't have any use for it.

I think we discussed this at the work week, but it will allow anyone who wants to use our RDP to know whether the server supports sourceMaps or not and whether the `useSourceMaps` flag will be used when they attach to a thread.

> ::: toolkit/devtools/debugger/server/dbg-server.js
> @@ +35,2 @@
> >      dump("DBG-SERVER: " + str + "\n");
> > +//  }
> 
> Uncomment these.
> How come you don't toggle the pref while testing instead?

Interesting, these aren't commented out in my git branch. I wonder if I uploaded an old patch? But that doesn't make sense either since the patch was rebased on the no-more-script-cache patch...

Anyways, I don't toggle the pref because I can never remember or find the file where to do it so that it happens in time.

*shrug*

> 
> ::: toolkit/devtools/debugger/tests/unit/test_profiler_actor.js
> @@ +161,2 @@
> >          do_check_true(Profiler.IsActive());
> > +        finishClient(client);
> 
> finishClient will also call do_test_finished(), potentially racing
> DebuggerServer._connectionClosed above. You could just use client.close() if
> you must do some cleanup here :-)

Word. This test was consistently failing for me without the changes, but client.close() works now for some reason.

Comment 38

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v5.1

Updated with requested changes from review.

Comment 39

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 727928 [details] [diff] [review]
v5.1

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #37)
> (In reply to Panos Astithas [:past] from comment #36)
> > Nothing really major, looks good.
> > I think that the reason your tests fail on B2G may be that they use
> > app://foo/bar URLs there.
> 
> I don't follow. All of the tests should still specify http://example.com/...
> urls because they are being source mapped, and using Cu.evalInSandbox. Not
> sure what is going on here, please enlighten me as to how to fix this.

My hunch was probably wrong, I think app://foo/bar URLs are only used by packaged apps, not regular pages.

You will probably have to run the tests locally in order to send a SIGINT to stop the stuck test and peek at the log. The test harness currently sends a SIGKILL on timeout which doesn't produce any useful output log. I would really like us to fix the test harness problem (bug 853787), but until then, running the tests locally is your best bet.

> > ::: toolkit/devtools/debugger/server/dbg-browser-actors.js
> > @@ +48,5 @@
> > >        from: "root",
> > >        applicationType: "browser",
> > >        traits: {
> > > +        sources: true,
> > > +        sourceMaps: true,
> > 
> > Do we use this new trait anywhere in the code? We shouldn't add it if we
> > don't have any use for it.
> 
> I think we discussed this at the work week, but it will allow anyone who
> wants to use our RDP to know whether the server supports sourceMaps or not
> and whether the `useSourceMaps` flag will be used when they attach to a
> thread.

What I'm arguing is, since this is not a distinction that our frontend is finding necessary to make, why do we believe that others will?

The current behavior of the frontend does not change depending on whether the backend is serving original or generated data. As a matter of fact, the only way the frontend has to know whether the server complied with a request to switch sources, is to compare the |frames| response before and after a |reconfigure| request, right? Actually, the mere presence of a |reconfigure| packet handler indicates sourcemaps support. Therefore I don't see any reason why anyone will ever have to consult a sourceMaps trait on startup. Can you think of any?

Comment 40

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

So I can't get the SourceMap.jsm to work on B2G. The source map xpcshell tests are all failing for me in B2G as well. On central. Why haven't there been automated bugs filed? I can't find any the source map tests on tbpl which is weird.

In the process of trying to fix this,

* I made sure that all the modules were using |this.EXPORTED_SYMBOLS| and not |let EXPORTED_SYMBOLS|. (Currently, source map's Utils.jsm still uses |let| but even with the changes it doesn't work).

* I verified that the JSM works in normal Firefox with the |jsloader.reuseGlobal| config option set

The results is that when I import the source map module, I get an object with the exported keys set on it, but the value is always undefined.

Help :(

Comment 41

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #39)
> The current behavior of the frontend does not change depending on whether
> the backend is serving original or generated data. As a matter of fact, the
> only way the frontend has to know whether the server complied with a request
> to switch sources, is to compare the |frames| response before and after a
> |reconfigure| request, right? Actually, the mere presence of a |reconfigure|
> packet handler indicates sourcemaps support. Therefore I don't see any
> reason why anyone will ever have to consult a sourceMaps trait on startup.
> Can you think of any?

There are many details here that I don't understand, but in general, clients are supposed to discover features by just trying them and seeing if they work, when that's practical. The "unrecognizedPacketType" error ought to be delivered reliably.

I can imagine situations where this won't be adequate. For example, if we were careless enough to add a new property to an existing request, where the request has side effects, it wouldn't be safe for a client that was unsure whether the server understands the property to simply send the request with the property set, and hope the effect is correct. In that situation, there would need to be some way to find out in advance whether the property was supported. (But ideally we wouldn't extend the protocol this way in the first place...)

But I don't think that's the case here, right?

Comment 42

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #40)
> So I can't get the SourceMap.jsm to work on B2G. The source map xpcshell
> tests are all failing for me in B2G as well. On central. Why haven't there
> been automated bugs filed? I can't find any the source map tests on tbpl
> which is weird.

Which tests are failing on m-c? The source map tests are all in this uncommitted patch, right?

There are some debugger xpcshell tests disabled on B2G, see the dependencies of bug 820380. The rest are usually green, see here for example:

https://tbpl.mozilla.org/php/getParsedLog.php?id=21145529&tree=Firefox&full=1


> In the process of trying to fix this,
> 
> * I made sure that all the modules were using |this.EXPORTED_SYMBOLS| and
> not |let EXPORTED_SYMBOLS|. (Currently, source map's Utils.jsm still uses
> |let| but even with the changes it doesn't work).
> 
> * I verified that the JSM works in normal Firefox with the
> |jsloader.reuseGlobal| config option set
> 
> The results is that when I import the source map module, I get an object
> with the exported keys set on it, but the value is always undefined.
> 
> Help :(

Could you get the tests to run on b2g locally? If so, what did the log from an interrupted test look like?

Comment 43

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jim Blandy :jimb from comment #41)
> (In reply to Panos Astithas [:past] from comment #39)
> > The current behavior of the frontend does not change depending on whether
> > the backend is serving original or generated data. As a matter of fact, the
> > only way the frontend has to know whether the server complied with a request
> > to switch sources, is to compare the |frames| response before and after a
> > |reconfigure| request, right? Actually, the mere presence of a |reconfigure|
> > packet handler indicates sourcemaps support. Therefore I don't see any
> > reason why anyone will ever have to consult a sourceMaps trait on startup.
> > Can you think of any?
> 
> There are many details here that I don't understand, but in general, clients
> are supposed to discover features by just trying them and seeing if they
> work, when that's practical. The "unrecognizedPacketType" error ought to be
> delivered reliably.
> 
> I can imagine situations where this won't be adequate. For example, if we
> were careless enough to add a new property to an existing request, where the
> request has side effects, it wouldn't be safe for a client that was unsure
> whether the server understands the property to simply send the request with
> the property set, and hope the effect is correct. In that situation, there
> would need to be some way to find out in advance whether the property was
> supported. (But ideally we wouldn't extend the protocol this way in the
> first place...)
> 
> But I don't think that's the case here, right?

Good point. A |reconfigure| request only clears the source cache in the server, which will be recreated when needed, so I think that doesn't qualify as a client-visible side-effect, right?

I suppose you could argue that "reconfigure" is too broad and perhaps in the future it will be used to modify a thread actor in ways that do produce side-effects. In that case I think that we should reconsider a sourceMaps trait at that hypothetical point in the future, and not architect for use cases that may never materialize.

Comment 44

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v6

Tests are passing on b2g locally!

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

* Removed the sourceMaps trait from the hello packet.

* Updated the source map lib from master. Now works on B2G, has support for composing source maps, etc. Would have needed to get all the updates for bug 849069, so I figured it wasn't worth pulling out just the B2G stuff.

Comment 45

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 730321 [details] [diff] [review]
v6

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

It needs a rebase, and apart from a few nits, this is ready to land.

::: toolkit/devtools/debugger/server/dbg-browser-actors.js
@@ +47,5 @@
>      return {
>        from: "root",
>        applicationType: "browser",
>        traits: {
> +        sources: true,

Nit: unnecessary change.

::: toolkit/devtools/sourcemap/SourceMap.jsm
@@ +996,5 @@
> +      }
> +    };
> +
> +  /**
> +   * Applies a SourceMap for a source file to the SourceMap.

This is slightly confusing, see if you can reword it a bit.

::: toolkit/devtools/sourcemap/tests/unit/Utils.jsm
@@ +77,5 @@
>   * Copyright 2011 Mozilla Foundation and contributors
>   * Licensed under the New BSD license. See LICENSE or:
>   * http://opensource.org/licenses/BSD-3-Clause
>   */
> +define('test/source-map/util', ['require', 'exports', 'module' ,  'lib/source-map/util'], function(require, exports, module) {

Nit: ETOOMANYSPACES

Comment 46

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v6.1

Fixed nits, except whitespace. I posit that it doesn't matter since that JSM is generated from the mozilla/source-map repo and that line in particular is not part of the original sources, but is generated just to make sure that we get all the exports and stuff.

Comment 47

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 48

[Panos Astithas (he/him) [:past] (please ni?)]

Backed out on suspicion of general breakage:
https://hg.mozilla.org/integration/fx-team/rev/54ff299711ab

Comment 49

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v6.2

Rebased.

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

Comment 50

[Panos Astithas (he/him) [:past] (please ni?)]

I don't think the try failures should be attributed to bug 852512. That orange had only appeared once without your patch, and it included a single failure, not a multitude of failures like in this case. Also, browser_toolbox_window_title_changes.js is the first test to fail in your case, so it is very likely that the failure in browser_inspector_bug_650804_search.js is fallout from that one. We can't land a patch that turns a test permaorange (never mind multiple tests), unless the test itself is deemed buggy.

Comment 51

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 732046 [details] [diff] [review]
v6.2

Unfortunately, I don't know what could be causing these oranges, aside from a suspicion that the leak I've been fighting in bug 849071 may be caused by this patch, even though I couldn't reproduce it locally. Maybe this patch changes the timing of protocol events/responses enough to break tests that expect things to be really snappy.

Comment 52

[Girish Sharma [:Optimizer]]

The inspector search related failures will only happen if the toolbox is left undocked, which is actually due to browser_toolbox_window_title_changes.js timing out and thus in turn not being able to dock the toolbox back. If you fix the browser_toolbox_window_title_changes.js failure, search failure will not happen. Or otherwise, fix the browser_inspector_bug_650804_search.js by synthesizing the keys on toolbox window, but browser_toolbox_window_title_changes.js will still remain.

Comment 53

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

After bisecting while applying my patch and testing browser_toolbox_window_title_changes.js at each step, the first bad commit that I get is

d72e5febb76f2198d49134692f0070658b5dd0b8 is the first bad commit
commit d72e5febb76f2198d49134692f0070658b5dd0b8
Author: Victor Porof <[e-mail]>
Date:   Thu Mar 28 10:30:37 2013 +0200

    Bug 854185 - Frontend shouldn't mix promises, event listeners and callbacks on initialization/destruction, r=past

:040000 040000 bd3caafdadfa5bd2b9c01d59c91f5b174170466e a8a1da338bd4fe6fc84e1282cc52ecb214ba8a33 M	browser

Will look into that commit more tomorrow.

Comment 54

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hey Victor, can you give a look at this when you get a chance and see if there is anything about the combination of our patches that might be causing this test failure? Thanks :)

Comment 55

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v7

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

So closing the toolbox was causing a disconnect to happen (which in turn resumes the thread) right before we would receive a "resume" packet. By adding a state check at the start of TA_onResume and returning an error packet if the state wasn't "paused" I have all the tests passing locally.

Victor and I on discussed what might be causing the change in order of events such that the above situation started happening, and we concluded that it was probably related to something that was asynchronous becoming synchronous what with all the things becoming promises.

Comment 56

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 734055 [details] [diff] [review]
v7

Hmm, I would expect that returning a (different) error message would still cause tests to fail, but the change is reasonable, so let's find out!

Comment 57

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v7.1

Had to rebase this patch for bug 849069, would be awesome if someone could land this for me so I don't have to keep rebasing :)

The try push for that bug includes this patch: https://tbpl.mozilla.org/?tree=Try&rev=3ddd5d28881d

Comment 58

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 59

[Panos Astithas (he/him) [:past] (please ni?)]

Backed out on suspicion of mochitest-chrome leaks:
https://hg.mozilla.org/integration/fx-team/rev/8a0fcda5b4eb

Comment 60

[Panos Astithas (he/him) [:past] (please ni?)]

Note that the leaks appeared in a mochitest-chrome test run, which crashes in an unrelated test for me locally. This should be enough to reproduce though:

mach mochitest-chrome browser/

Comment 61

[Panos Astithas (he/him) [:past] (please ni?)]

I should add that the only mochitest-chrome tests we currently have are for the web console protocol bits, so this leak is not caused by frontend code (which is to be expected from this patch). It might be helpful if we could pinpoint whether the leak is in the client or server code.

Comment 62

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v8

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

The leak was caused by having attaching "all" onto "Promise" instead of just having it be its own function.

Comment 63

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 737847 [details] [diff] [review]
v8

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

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +28,5 @@
>  const { defer, resolve, reject } = Promise;
> +let promisedArray = Promise.promised(Array);
> +function resolveAll(aPromises) {
> +  return promisedArray.apply(null, aPromises);
> +};

We can take this as it is, but did you try just using the now exported Promise.all instead of patching it from the outside like in the previous patch?

Comment 64

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] from comment #63)
> We can take this as it is, but did you try just using the now exported
> Promise.all instead of patching it from the outside like in the previous
> patch?

I was curious so I did a try push:

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

Comment 65

[Panos Astithas (he/him) [:past] (please ni?)]

Relanded, with fingers crossed:
https://hg.mozilla.org/integration/fx-team/rev/70caf37070d3

Comment 66

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] from comment #64)
> (In reply to Panos Astithas [:past] from comment #63)
> > We can take this as it is, but did you try just using the now exported
> > Promise.all instead of patching it from the outside like in the previous
> > patch?
> 
> I was curious so I did a try push:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=bf22677d74a1

Green on try, but not urgent, so I filed bug 862360 for that.

Comment 67

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #63)
> Comment on attachment 737847 [details] [diff] [review]
> v8
> 
> Review of attachment 737847 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/server/dbg-server.js
> @@ +28,5 @@
> >  const { defer, resolve, reject } = Promise;
> > +let promisedArray = Promise.promised(Array);
> > +function resolveAll(aPromises) {
> > +  return promisedArray.apply(null, aPromises);
> > +};
> 
> We can take this as it is, but did you try just using the now exported
> Promise.all instead of patching it from the outside like in the previous
> patch?

Wasn't aware that Promise.all was available now. Probably related to the reason why everything was leaking if we overwrote it.

Comment 68

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/70caf37070d3