1447244 - Devtools error console link to debugger shows invalid link

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Josh Unger]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180317220121

Steps to reproduce:

1. Clone https://github.com/joshunger/bugrepro
2. Edit index.jsx and add `throw new Error('foo');`
3. Yarn
4. Yarn start
5. Visit URL.
6. Open devtools and console.
7. Notice URL in error.

Comment 1

[Josh Unger]

Attachment: Firefox_Nightly.png

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

I can't run the provided example anymore, can you still reproduce this error?

Comment 3

[Josh Unger]

Attachment: Firefox_Nightly.png

Comment 4

[Josh Unger]

The text looks better but the hyperlink loads as Not Found.

Comment 5

[Josh Unger]

Attachment: http___localhost_8080_main_js_20line_204631_20_3E_20eval.png

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Is there a way I can reproduce this issue?

Comment 7

[Josh Unger]

Let me know if you can reproduce with https://bug-1447244.glitch.me/?

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Indeed I can, thanks!

Comment 9

[Josh Unger]

Excellent, I'll upload to Glitch going forward. That works well!

Comment 10

[Brian Hackett [Laid off!]]

I think the fundamental problem here is that location information associated with console messages (the location of the message and any stack trace) only refers to URLs, and not the script sources which would precisely identify the location of interest. URLs can't be used when the location is in code generated from an eval or something similar.

If we can determine the source for these locations, we can easily map them back to tabs in the debugger. Referring to the sources directly from the console messages wouldn't be good I think, because the messages can live a long time and we don't to prevent the sources themselves from being GC'ed. An alternative would be to assign a unique, process-wide ID/tag to each source, and refer to those from the message. As long as the source hasn't been GC'ed by the time the console is opened, we could map the tag back to that source.

Similar to bug 1503422, having the console or debugger open when the page is loaded will prevent sources from being GC'ed. If we find that a source in a message has already been GC'ed we could show something to the user for generated sources (make the link non-clickable, with a description when hovering?) instead of letting them click and get weird behavior.

Comment 11

[Tom Tromey :tromey]

(In reply to Brian Hackett (:bhackett) from comment #10)

An alternative would be to assign a unique, process-wide ID/tag to each source, and refer to those from the message.

This would also be useful if done for style sheets, see https://bugzilla.mozilla.org/show_bug.cgi?id=880831#c11. I think there are other dependencies of the source maps meta bug (bug 1339970) that touch on these problems as well.

Comment 12

[Brian Hackett [Laid off!]]

Attachment: WIP

WIP to add unique tags to script sources and retain them in error messages and error message stacks. The devtools server can convert the tags into actor IDs before sending them to the client, and the client can then go straight to the source to display after a link is clicked instead of just using a URL. This works for links in error message stacks in the console, but I got hung up getting this to work for the location of the error message itself. This information originates from the nsIScriptError, and when modifying the initialization methods for nsIScriptError to include a source tag I found the patch turned a lot of code pretty ugly due to having to pass yet another source-related bit of information around (there are four already). I tried creating a SourceLocation class to encapsulate this, but there are a ton of places this needs to be used and I don't think this bug is the place for a large-scale cleanup/refactoring of how the browser handles error messages.

I'm going to try a different approach where the source tag is not specified when initializing an nsIScriptError, but can be optionally specified afterwards. This should avoid having to make a lot of code changes and keep this bug more narrowly focused.

Comment 13

[Brian Hackett [Laid off!]]

Attachment: patch

Complete patch, requiring source tags to be separately specified on nsIScriptErrors to avoid many of the changes in the last patch (though lots of C++ changes are still required). I'll split this up for review shortly.

Comment 14

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Add unique tag to ScriptSource.

Add a tag to each created ScriptSource which is unique within the process. The source's tag is essentially a weak reference that can only be used in equality comparisons and is easy to pass around without getting the GC involved.

Comment 15

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Track source tag in saved frames.

Keep track of source tags in saved frames along with other location information. Devtools server code will be able to access the source tags in these saved frames directly, e.g. in stacks associated with page errors.

Comment 16

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Track source tag in JSErrorReport and ErrorObject.

Source tags need to be tracked in exception information so that they can be used when generating nsIScriptErrors (which the devtools server will then consume).

Comment 17

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Add DebuggerSource.tag.

Debugger.Source provides the tag of the underlying ScriptSource so that the devtools server can create a map from tags to sources.

Comment 18

[Brian Hackett [Laid off!]]

Attachment: Part 5 - Add source tag to nsIScriptError and ConsoleEvent.

Propagate the source tag from JSErrorReports and saved frames to nsIScriptError and webidl ConsoleEvents, for use by devtools server code.

Comment 19

[Brian Hackett [Laid off!]]

Attachment: Part 6 - Devtools server changes.

The devtools server uses DebuggerSource.tag to create a map from tags to any source actors it has created. This map is used to include source actor IDs when possible when informing the client about page errors and console API calls.

Comment 20

[Brian Hackett [Laid off!]]

Attachment: Part 7 - Devtools client changes.

The devtools client never learns about source tags, but it has to cope with the source actor IDs now being transmitted for incoming messages. This patch passes those IDs around in various places and uses them to open sources in the debugger by their ID instead of by URL if the ID is available.

Comment 21

[Brian Hackett [Laid off!]]

Attachment: Part 8 - Add new test.

Add a test for the new functionality.

Comment 22

[Brian Hackett [Laid off!]]

(In reply to Tom Tromey :tromey from comment #11)

(In reply to Brian Hackett (:bhackett) from comment #10)

An alternative would be to assign a unique, process-wide ID/tag to each source, and refer to those from the message.

This would also be useful if done for style sheets, see https://bugzilla.mozilla.org/show_bug.cgi?id=880831#c11. I think there are other dependencies of the source maps meta bug (bug 1339970) that touch on these problems as well.

Yeah, the approach in these patches is very similar to what you suggest in that comment. This bug adds the plumbing necessary for adding an identifying tag to page errors and console messages (though it is a uint32 instead of a string), which should be reusable for errors/messages related to style sheets.

Comment 23

[Brian Hackett [Laid off!]]

Attachment: Part 7.1 - Ensure that source actors are created before messages are fetched.

Oops, forgot this part. In order for the tag -> actor map in the server to work, the source actor needs to have already been created at the time we process a message that is being sent to the client.

Currently, this is not guaranteed to be the case: the console will usually request the sources for a page, but not until it has a message with a generated location which it needs to map back to an original location.

This patch ensures the above property by making sure the console client requests the sources for a page before it requests the cached messages. This reordering is rather fragile (though any breakage should be caught by the new test) though it is simple.

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 9037032 [details] [diff] [review]
Part 1 - Add unique tag to ScriptSource.

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

::: js/src/vm/JSScript.h
@@ +584,5 @@
>  
> +  // A tag for this source that is unique across the process. This can be used
> +  // to refer to this source from places that don't want to hold a strong
> +  // reference on the source itself.
> +  uint32_t tag_;

Drive-by comment: uint32_t is prone to overflow, at least considering a malicious website using Function or eval in a tight loop. Maybe safer to use uint64_t.

Comment 25

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #24)

Comment on attachment 9037032 [details] [diff] [review]
Part 1 - Add unique tag to ScriptSource.

Review of attachment 9037032 [details] [diff] [review]:

::: js/src/vm/JSScript.h
@@ +584,5 @@

• // A tag for this source that is unique across the process. This can be used
• // to refer to this source from places that don't want to hold a strong
• // reference on the source itself.
• uint32_t tag_;

Drive-by comment: uint32_t is prone to overflow, at least considering a
malicious website using Function or eval in a tight loop. Maybe safer to use
uint64_t.

Yeah, I considered this, though the only bad effects that can arise from overflow are bogus equality comparisons and incorrectly behaving devtools. When the devtools are open, all ScriptSources are kept alive by the devtools server and we'll choke long before 2^32 of them can be allocated. I don't have a strong preference, but uint32_t reduces the memory impact and can be straightforwardly converted to a JS value.

Comment 26

[Jason Orendorff [:jorendorff]]

Comment on attachment 9037032 [details] [diff] [review]
Part 1 - Add unique tag to ScriptSource.

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

jimb asked to steal these and I am ok with that

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9037042 [details] [diff] [review]
Part 5 - Add source tag to nsIScriptError and ConsoleEvent.

I'm having great trouble to understand what 'tag' means. Some magical uint32_t value.
Could you please document the added attribute in .idl files and possibly rename to something more descriptive?

Is tag more like scriptSourceId ?

Comment 28

[Brian Hackett [Laid off!]]

(In reply to Olli Pettay [:smaug] from comment #27)

Comment on attachment 9037042 [details] [diff] [review]
Part 5 - Add source tag to nsIScriptError and ConsoleEvent.

I'm having great trouble to understand what 'tag' means. Some magical
uint32_t value.
Could you please document the added attribute in .idl files and possibly
rename to something more descriptive?

Is tag more like scriptSourceId ?

Yeah, the tag is a unique ID for a script source. I'll rename it to scriptSourceId; I called it a tag instead to distinguish it from the script source IDs used in devtools code, but there is little overlap between the places where the two kinds of IDs are used so I don't think it would cause much confusion.

Comment 29

[Brian Hackett [Laid off!]]

Attachment: patch

This updated complete patch replaces 'tag' with 'id' throughout, and adds some comments and minor fixes.

Comment 30

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Add unique id to ScriptSource.

Updated patch for s/tag/id.

Comment 31

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Track source ID in saved frames.

Updated patch for s/tag/id.

Comment 32

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Track source ID in JSErrorReport and ErrorObject.

Updated patch for s/tag/id. This also fixes a bug where frame iteration scripts were not null checked --- they will be null for wasm frames.

Comment 33

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Add DebuggerSource.id

Updated patch for s/tag/id.

Comment 34

[Brian Hackett [Laid off!]]

Attachment: Part 5 - Add source ID to nsIScriptError and ConsoleEvent.

Updated patch for s/tag/id. I went with sourceId instead of scriptSourceId in IDL files, in case we use these IDs for other sources like CSS files.

Comment 35

[Brian Hackett [Laid off!]]

Attachment: Part 6 - Devtools server changes.

Updated patch for s/tag/id. Since this is the only place where the internal source IDs and the actor source IDs interact, this patch uses extra-cumbersome names like getSourceActorByInternalSourceId to avoid ambiguity / accidental misuse.

Comment 36

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9038009 [details] [diff] [review]
Part 4 - Add DebuggerSource.id

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

::: js/src/vm/Debugger.cpp
@@ +7794,5 @@
> +    ScriptSource* ss = sourceObject->source();
> +    return ss->id();
> +  }
> +  ReturnType match(Handle<WasmInstanceObject*> instanceObj) {
> +    return 0;

Is there any way to have this be consistently incrementing along with JS? It seems pretty frustrating to require verifying the source type before checking the source ID.

Comment 37

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9038016 [details] [diff] [review]
Part 6 - Devtools server changes.

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

::: devtools/server/actors/utils/TabSources.js
@@ +36,5 @@
> +  // The IDs associated with ScriptSources and available via DebuggerSource.id
> +  // are internal to this process and should not be exposed to the client. This
> +  // map associates these IDs with the corresponding source actor, provided the
> +  // source has not been GC'ed and the actor has been created.
> +  this._sourceActorsByInternalSourceId = Object.create(null);

If you're up for it, I'd use a Map instead of an object.

@@ +183,5 @@
>      return sourceActor;
>    },
>  
> +  getSourceActorByInternalSourceId: function(id) {
> +    return this._sourceActorsByInternalSourceId[id];

Same here as below

```
return this._sourceActorsByInternalSourceId[id] || null;
```

::: devtools/server/actors/webconsole.js
@@ +1473,5 @@
>    },
>  
> +  getActorIdForInternalSourceId(id) {
> +    const actor = this.parentActor.sources.getSourceActorByInternalSourceId(id);
> +    return actor ? actor.actorID : undefined;

I'd probably favor `null` as indicator of a missing result for 2 reasons:

* `undefined` values in JSON are omitted from the response entirely, making it harder to see that the ID failed to match
* `null` vs `undefined` isn't really consistent across the people I think, but I'd usually only expect a function to only return `undefined` if it _never_ returned a value, e.g. `return;` or evaluating to the end of the function, with `null` used to indicate a failure to find something that could have been returned.

Comment 38

[Brian Hackett [Laid off!]]

Attachment: Part 6 - Devtools server changes.

Updated patch per comments.

Comment 39

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9039372 [details] [diff] [review]
Part 6 - Devtools server changes.

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

::: devtools/server/actors/utils/TabSources.js
@@ +145,4 @@
>  
>      if (source) {
>        this._sourceActors.set(source, actor);
> +      if (source.id) {

I won't block on it, but I really dislike overloading `0` to mean "no id". If the source type doesn't have an ID, it feels like it should return `null` or throw or something. If the type is a number, it should just be a number, rather than giving certain digits special significance.

Comment 40

[Jim Blandy :jimb]

Comment on attachment 9038006 [details] [diff] [review]
Part 1 - Add unique id to ScriptSource.

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

::: js/src/vm/JSScript.h
@@ +587,5 @@
> +  // reference on the source itself.
> +  uint32_t id_;
> +
> +  // How many ids have been handed out to sources.
> +  static mozilla::Atomic<uint32_t> gIdCount;

nit: I don't think the 'g' prefix is SpiderMonkey practice. Other statics seem to just follow the lowerCamelCaps_ rule and nothing further. I didn't find anything specific here:

https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#Classes

Comment 41

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #25)

Yeah, I considered this, though the only bad effects that can arise from overflow are bogus equality comparisons and incorrectly behaving devtools. When the devtools are open, all ScriptSources are kept alive by the devtools server and we'll choke long before 2^32 of them can be allocated. I don't have a strong preference, but uint32_t reduces the memory impact and can be straightforwardly converted to a JS value.

OK but in that case please document this better or rename things to make it clearer. The comment now says "An id for this source that is unique across the process." but it's not necessarily unique.

Sorry to be pedantic, but even though the current uses are safe, people will use this for other things in the future and introduce security bugs.

Comment 42

[Brian Hackett [Laid off!]]

Attachment: followup

There have been a bunch of changes to how sources work in the debugger since these patches were posted, and some followup work is needed. This patch fixes a couple issues:

• We can't guarantee that the client will ask for all sources before asking for cached messages, and in any case we probably shouldn't require this. This uses a different design where the server lazily generates a map from internal source id -> debugger source, and generates source actors for those sources when needed by the console instead of requiring them to already exist.

• Some changes to the view-source code in the client are needed because the actor ID used here is no longer the ID for the source itself.

Comment 43

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9045816 [details] [diff] [review]
followup

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

::: devtools/client/shared/view-source.js
@@ +60,5 @@
> +  } else if (await toolbox.sourceMapService.hasOriginalURL(sourceURL)) {
> +    // We have seen a source map for the URL but no source. The debugger will
> +    // still be able to load the source.
> +    await toolbox.selectTool("jsdebugger", reason);
> +    dbg.selectSourceURL(sourceURL, sourceLine);

Should we be concerned about cases here where we saw the URL in `sources`, but then failed to load the file, or does `hasOriginalURL` only return true if the original code was loaded?

Comment 44

[Brian Hackett [Laid off!]]

(In reply to Logan Smyth [:loganfsmyth] from comment #43)

Comment on attachment 9045816 [details] [diff] [review]
followup

Review of attachment 9045816 [details] [diff] [review]:

::: devtools/client/shared/view-source.js
@@ +60,5 @@

• } else if (await toolbox.sourceMapService.hasOriginalURL(sourceURL)) {
• // We have seen a source map for the URL but no source. The debugger will
• // still be able to load the source.
• await toolbox.selectTool("jsdebugger", reason);
• dbg.selectSourceURL(sourceURL, sourceLine);

Should we be concerned about cases here where we saw the URL in sources,
but then failed to load the file, or does hasOriginalURL only return true
if the original code was loaded?

hasOriginalURL indicates whether the URL was found as an original URL in a source map. It will still be there if we weren't able to fetch the original URL, but in that case I don't think it matters much whether we try to open the URL in the debugger or a view-source tab. In any case, the patches here don't change the existing behavior when we make this hasOriginalURL call. We just need some extra logic to deal with the fact that sometimes we are opening things in the debugger by SourceId instead of by URL.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d1b48923635
Part 1 - Add unique id to ScriptSource, r=jimb.
https://hg.mozilla.org/integration/mozilla-inbound/rev/37828dbf6749
Part 2 - Track source ID in saved frames, r=jimb.
https://hg.mozilla.org/integration/mozilla-inbound/rev/320d791829e2
Part 3 - Track source ID in JSErrorReport and ErrorObject, r=jimb.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5adb94d8c8e0
Part 4 - Add DebuggerSource.id, r=jimb.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3538cdbdb944
Part 5 - Add source ID to nsIScriptError and ConsoleEvent, r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcd062ed8466
Part 6 - Devtools server changes for script source IDs, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/15b57a09f35f
Part 7 - Devtools client changes for script source IDs, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/72038435352d
Part 8 - Generate source actors for internal source IDs on demand, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8891e6a6c18b
Part 9 - Add test for console links based on source IDs, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7d1cc3ef70
Part 10 - Fix console stub tests.

Comment 46

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/4d1b48923635
https://hg.mozilla.org/mozilla-central/rev/37828dbf6749
https://hg.mozilla.org/mozilla-central/rev/320d791829e2
https://hg.mozilla.org/mozilla-central/rev/5adb94d8c8e0
https://hg.mozilla.org/mozilla-central/rev/3538cdbdb944
https://hg.mozilla.org/mozilla-central/rev/dcd062ed8466
https://hg.mozilla.org/mozilla-central/rev/15b57a09f35f
https://hg.mozilla.org/mozilla-central/rev/72038435352d
https://hg.mozilla.org/mozilla-central/rev/8891e6a6c18b
https://hg.mozilla.org/mozilla-central/rev/aa7d1cc3ef70

Comment 47

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 61.0a1 (2018-03-20) on Windows 7, 64 Bit.
The fix of this bug is verified with latest Beta 67.0b6!

Build ID : 20190328152334
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:67.0) Gecko/20100101 Firefox/67.0