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

Indeed I can, thanks!

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

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.

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

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.

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.

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.

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.

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

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

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

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.

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.

Add a test for the new functionality.

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

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.

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

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

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

Updated patch for s/tag/id.

Updated patch for s/tag/id.

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.

Updated patch for s/tag/id.

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.

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.

Updated patch per comments.

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

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.

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

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

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