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

      No description provided.

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

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.

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

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

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

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

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

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

	Jim Blandy :jimb
	Comment 5 • 12 years ago

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

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

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

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

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?

	Jim Blandy :jimb
	Comment 8 • 12 years ago

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

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

Yup. Agree fully.

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

We already have bug 755661 for that.

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

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.

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

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

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

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

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

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

	Jim Blandy :jimb
	Comment 15 • 12 years ago

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

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

filter on chocolate

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

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?

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

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 20 • 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 v6 patch for that bug soon.

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

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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

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.

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

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.

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

OMG OMG OMG OMG OMG OMG

Caret works. Added caret positioning to test.

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

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

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

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

Needs rebasing due to bug 852860.

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

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

Rebased!

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

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.

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

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

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

Updated with requested changes from review.

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

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?

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

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

	Jim Blandy :jimb
	Comment 41 • 12 years ago

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

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

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

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

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

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

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.

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

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

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

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.

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

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

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

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

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

Rebased.

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

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

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.

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

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.

	Girish Sharma [:Optimizer]
	Comment 52 • 11 years ago

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.

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

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 <vporof@mozilla.com>
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.

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

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

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

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.

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

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!

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

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

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

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

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

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

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

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/

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

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.

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

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.

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

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?

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

(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

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

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

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

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

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

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

	Tim Taubert [:ttaubert] (inactive)
	Comment 68 • 11 years ago

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