switch console to use client-side source map service

RESOLVED FIXED in Firefox 55

Status

P1
normal
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
We want to switch the console to use the new client-side source map code.
(Assignee)

Comment 1

2 years ago
A few notes on what I've learned so far:

The actual console changes are simple.
Most of the adaptation is in the Frame component.
However, the only place that we get a sourceMapURL is the source actor.
So, we need some kind of intermediary for this.
(Assignee)

Updated

2 years ago
Blocks: 1349354
(Assignee)

Comment 2

2 years ago
The Frame component is also used in the memory and performance tools, but they don't
pass in a source map service.  However they may still need updates if we change the
shape of location objects passed in to Frame.  (Not sure yet if this is the plan.)
(Assignee)

Comment 3

2 years ago
There is no source actor if there is no source.
This can happen if the page calls console.log and then the source is GC'd.
So, for example, if you open the console after the page is loaded, we'll be unable to source map
some things.

One fix for this would be to fetch the original resource ourselves and parse it to get the
source map url.
(Assignee)

Comment 4

2 years ago
Another idea is to change Console.webidl and Console.cpp to put the sourceMapURL into
the ConsoleStackEntry.  I'm going to investigate this a bit more because it seems promising.
status-firefox55: --- → affected
status-firefox57: affected → ---
(Assignee)

Comment 5

2 years ago
The Console.webidl change seems doable, but somewhat complicated; and it affects memory use.
So I am going to defer it to a separate bug.
(Assignee)

Updated

2 years ago
Depends on: 1360554
(Assignee)

Comment 6

2 years ago
My patch does the mapping ok, but with webpack I see an original URL like "webpack:///./f2.js".
Clicking on this in the console doesn't do anything useful.
(Assignee)

Comment 7

2 years ago
Probably what needs to happen is that we should have a special case when clicking on any source-mapped link.
(Assignee)

Comment 8

2 years ago
devtools-source-map doesn't emit any sort of notification when applySourceMap is called.
Until this is done, the console will not notice when pretty-printing is done in the debugger.
I think I will handle this in a follow-up bug.
(Assignee)

Updated

2 years ago
Blocks: 1317962
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8863416 [details]
Bug 1359144 - use client-side source map service in console;

https://reviewboard.mozilla.org/r/135176/#review138144

Overall, this looks reasonable to me!  I was curious about the naming issues, so I'd like to hear more about that before marking r+.

::: devtools/client/framework/source-map-url-service.js:80
(Diff revision 1)
> +  if (!urlInfo) {
> +    return null;
> +  }
> +  // Call getOriginalURLs to make sure the source map has been
> +  // fetched.  We don't actually need the result of this though.
> +  await this._sourceMapService.getOriginalURLs(urlInfo);

If something goes wrong (bad SM URL, invalid SM syntax, etc.), this will return `false`, perhaps that's useful to test for to catch the error case?

I was about to say you could add the first of many source map diagnostic logs here (to debug broken maps), but this is probably too high level?  At this spot, all you can probably say is "something is wrong!".  (Though, even that is probably better than silence!)

::: devtools/client/framework/toolbox.js:561
(Diff revision 1)
>  
> +  /**
> +   * Clients wishing to use source maps but that want the toolbox to
> +   * track the source actor mapping can use this source map service.
> +   */
> +  get sourceMapURLService() {

The overall design here seems reasonable (a service to track the URLs, and then call into other modules), but the naming does worry me a bit...  The difference between `sourceMapService` and `sourceMapURLService` doesn't seem very obvious at all.

But...  since we want to share `sourceMapService` with the debugger, we don't want to just drop it from the toolbox...

Any thoughts on better naming to reduce confusion here?

::: devtools/client/shared/components/test/mochitest/test_frame_01.html:290
(Diff revision 1)
>        line: "1",
>        shouldLink: true,
>        tooltip: "View source in Debugger → http://www.cnn.com/:1",
>      });
>  
> +    const resolvedLocation = {

Does the console have any other source map tests, or is the only one?  Not sure what the right level of test coverage is, since we eventually call down to mozilla/source-map which is fairly well tested, and the debugger also has some high level tests...  So maybe the console doesn't have to have an extensive suite of its own...
Attachment #8863416 - Flags: review?(jryans)
(Assignee)

Comment 13

2 years ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)

> I was about to say you could add the first of many source map diagnostic
> logs here (to debug broken maps), but this is probably too high level?

It's a good idea, but I'd rather defer the error-reporting issue to a separate bug,
I suppose bug 1345533.  Thinking about why I want to do this, I suppose partly
it's to make progress piecemeal, and also partly to attack the reporting problem
once I have a better summary view of the various users of the service.

> The overall design here seems reasonable (a service to track the URLs, and
> then call into other modules), but the naming does worry me a bit...  The
> difference between `sourceMapService` and `sourceMapURLService` doesn't seem
> very obvious at all.
> 
> But...  since we want to share `sourceMapService` with the debugger, we
> don't want to just drop it from the toolbox...
> 
> Any thoughts on better naming to reduce confusion here?

Yeah, this name is kind of terrible with the other service still around.  I don't
think my naming skills are all that great, but I did think about this a bit, and
couldn't find another obviously better name.  Then I talked myself into this name
because we're going to remove source-map-service in bug 1349354. 

> Does the console have any other source map tests, or is the only one?  Not
> sure what the right level of test coverage is, since we eventually call down
> to mozilla/source-map which is fairly well tested, and the debugger also has
> some high level tests...  So maybe the console doesn't have to have an
> extensive suite of its own...

This might be the only one.  I was just trying to make sure the new code was hit,
but that's pretty lame really.  I will see if I can come up with a bit more, and 
also it seems to me that I could unit test the url service.
(In reply to Tom Tromey :tromey from comment #13)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> > The overall design here seems reasonable (a service to track the URLs, and
> > then call into other modules), but the naming does worry me a bit...  The
> > difference between `sourceMapService` and `sourceMapURLService` doesn't seem
> > very obvious at all.
> > 
> > But...  since we want to share `sourceMapService` with the debugger, we
> > don't want to just drop it from the toolbox...
> > 
> > Any thoughts on better naming to reduce confusion here?
> 
> Yeah, this name is kind of terrible with the other service still around.  I
> don't
> think my naming skills are all that great, but I did think about this a bit,
> and
> couldn't find another obviously better name.  Then I talked myself into this
> name
> because we're going to remove source-map-service in bug 1349354. 

Okay, well, maybe we could at least add a few more comments around the toolbox getters for the two services to explain one is a lower level service used by new debugger vs. the higher level service used by the console?

Anyway, seems like you've thought it through, so I'll mark r+.

Comment 15

2 years ago
mozreview-review
Comment on attachment 8863416 [details]
Bug 1359144 - use client-side source map service in console;

https://reviewboard.mozilla.org/r/135176/#review138164
Attachment #8863416 - Flags: review+
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
Comment on attachment 8863416 [details]
Bug 1359144 - use client-side source map service in console;

Re-requesting review.  I added some comments and I adapted one of the
server-side tests to use the new service.  (The other server-side test
will have to wait for a later bug.)
Attachment #8863416 - Flags: review+ → review?(jryans)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8863416 [details]
Bug 1359144 - use client-side source map service in console;

https://reviewboard.mozilla.org/r/135176/#review138648

Thanks, this looks good to me!
Attachment #8863416 - Flags: review?(jryans) → review+

Comment 19

2 years ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5a0c4a83b72
use client-side source map service in console; r=jryans

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d5a0c4a83b72
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Updated

2 years ago
Blocks: 1289570
(Assignee)

Updated

2 years ago
Blocks: 1297498
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1309493
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1365579

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.