Note: There are a few cases of duplicates in user autocompletion which are being worked on.

expose source mapped sources over the remote debugging protocol

RESOLVED FIXED in Firefox 23

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 23
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 15 obsolete attachments)

v8
142.29 KB, patch
past
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Assignee: nobody → general
Component: Developer Tools: Debugger → JavaScript Engine
Product: Firefox → Core
Assignee: general → nobody
Component: JavaScript Engine → Developer Tools: Debugger
Product: Core → Firefox
Blocks: 771597
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.
(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.
(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.
(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.
OS: Mac OS X → All
Priority: -- → P2
QA Contact: nfitzgerald
Hardware: x86 → All
Assignee: nobody → nfitzgerald
QA Contact: nfitzgerald
OS: All → Mac OS X
Priority: P2 → --
Hardware: All → x86
Summary: Add a "sourceMapURL" property to the "scripts" and "newScript" packets in the remote debugger protocol → Add a "sourceMap" property to the "scripts" and "newScript" packets in the remote debugger protocol

Comment 5

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

5 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.
Yup. Agree fully.
Summary: Add a "sourceMap" property to the "scripts" and "newScript" packets in the remote debugger protocol → Extend the remote debugger protocol to support fetching the original source of source mapped scripts
We already have bug 755661 for that.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 755661
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.
Status: RESOLVED → REOPENED
Depends on: 755661
Resolution: DUPLICATE → ---
(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.
Status: REOPENED → NEW
(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?
(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

5 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.
filter on chocolate
Priority: -- → P2
Depends on: 694539
Status: NEW → ASSIGNED
Priority: P2 → --
Priority: -- → P2
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?
Created attachment 651601 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools
Attachment #651601 - Flags: feedback?(jwalker)
Related to part 1: https://github.com/mozilla/gcli/pull/59
Attachment #651601 - Flags: feedback?(jwalker)
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.
Depends on: 783393
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
Created attachment 654017 [details] [diff] [review]
Part 1: Move Promise.jsm to toolkit/devtools, v2
Attachment #651601 - Attachment is obsolete: true
Attachment #654017 - Flags: review?(past)
Attachment #654017 - Flags: feedback?(jwalker)
Created attachment 654019 [details] [diff] [review]
Part 2: Fetch sources from the server, send them over the protocol, v1
Attachment #654019 - Flags: review?(past)
Attachment #654019 - Flags: review?(past)
Attachment #654017 - Flags: review?(past)
Attachment #654017 - Flags: feedback?(jwalker)
Ignore everything from comment 20 through this comment, wrong bug >_<
No longer depends on: 783393
Attachment #654017 - Attachment is obsolete: true
Attachment #654019 - Attachment is obsolete: true
Summary: Extend the remote debugger protocol to support fetching the original source of source mapped scripts → expose source mapped sources over the remote debugging protocol
Depends on: 795368
Blocks: 840684
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.
Depends on: 848576
Depends on: 850086
Created attachment 724709 [details] [diff] [review]
wip

Rebased the WIP patch I have going and uploading it so that Panos can take a look.
Attachment #724709 - Attachment is patch: true
Created attachment 725080 [details] [diff] [review]
v1

https://tbpl.mozilla.org/?tree=Try&rev=fbadb0859c89
Attachment #724709 - Attachment is obsolete: true
Attachment #725080 - Flags: review?(past)
Blocks: 849071
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
Attachment #725080 - Flags: review?(past)
(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.
Created attachment 725645 [details] [diff] [review]
v2

Rebased on top of Eddy's patch, which has fixed the disappearing breakpoint issue. General wonkiness is still there though.
Attachment #725080 - Attachment is obsolete: true
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.
Created attachment 726753 [details] [diff] [review]
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.
Attachment #725645 - Attachment is obsolete: true
Created attachment 726989 [details] [diff] [review]
v4

OMG OMG OMG OMG OMG OMG

Caret works. Added caret positioning to test.

https://tbpl.mozilla.org/?tree=Try&rev=579a8d7ca919
Attachment #726753 - Attachment is obsolete: true
Attachment #726989 - Flags: review?(past)
Comment on attachment 726989 [details] [diff] [review]
v4

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

Needs rebasing due to bug 852860.
Attachment #726989 - Flags: review?(past)
Depends on: 852860
Created attachment 727289 [details] [diff] [review]
v5

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

Rebased!
Attachment #726989 - Attachment is obsolete: true
Attachment #727289 - Flags: review?(past)
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.
Attachment #727289 - Flags: review?(past) → review+
(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.
Created attachment 727928 [details] [diff] [review]
v5.1

Updated with requested changes from review.
Attachment #727289 - Attachment is obsolete: true
Attachment #727928 - Flags: review?(past)
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?
Attachment #727928 - Flags: review?(past) → review+
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

4 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?
(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?
(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.
Created attachment 730321 [details] [diff] [review]
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.
Attachment #727928 - Attachment is obsolete: true
Attachment #730321 - Flags: review?(past)
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
Attachment #730321 - Flags: review?(past) → review+
Created attachment 731293 [details] [diff] [review]
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.
Attachment #730321 - Attachment is obsolete: true
Attachment #731293 - Flags: review?(past)
Attachment #731293 - Flags: review?(past) → review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/a69d60d90110
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Backed out on suspicion of general breakage:
https://hg.mozilla.org/integration/fx-team/rev/54ff299711ab
Whiteboard: [fixed-in-fx-team]
Created attachment 732046 [details] [diff] [review]
v6.2

Rebased.

https://tbpl.mozilla.org/?tree=Try&rev=b2ea18b5d898
Attachment #731293 - Attachment is obsolete: true
Attachment #732046 - Flags: review?(past)
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 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.
Attachment #732046 - Flags: review?(past)
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.
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.
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 :)
Flags: needinfo?(vporof)
Created attachment 734055 [details] [diff] [review]
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.
Attachment #732046 - Attachment is obsolete: true
Attachment #734055 - Flags: review?(past)
Flags: needinfo?(vporof)
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!
Attachment #734055 - Flags: review?(past) → review+
Whiteboard: [land-in-fx-team]
Created attachment 735321 [details] [diff] [review]
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
Attachment #734055 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team] → [f
https://hg.mozilla.org/integration/fx-team/rev/eb887b962dfa
Whiteboard: [f → [fixed-in-fx-team]
Backed out on suspicion of mochitest-chrome leaks:
https://hg.mozilla.org/integration/fx-team/rev/8a0fcda5b4eb
Whiteboard: [fixed-in-fx-team]
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/
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.
Created attachment 737847 [details] [diff] [review]
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.
Attachment #735321 - Attachment is obsolete: true
Attachment #737847 - Flags: review?(past)
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?
Attachment #737847 - Flags: review?(past) → review+
(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
Relanded, with fingers crossed:
https://hg.mozilla.org/integration/fx-team/rev/70caf37070d3
Whiteboard: [fixed-in-fx-team]
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/70caf37070d3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23

Updated

4 years ago
Depends on: 883649
You need to log in before you can comment on or make changes to this bug.