Closed Bug 1447244 Opened 4 years ago Closed 3 years ago

Devtools error console link to debugger shows invalid link

Categories

(DevTools :: Console, defect, P2)

61 Branch
defect

Tracking

(firefox67 fixed)

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: joshunger1, Assigned: bhackett1024)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [good first verify] [bugday-20190327])

User Story

**Steps to reproduce**
1. Go to https://devtools-console-eval-throw-link-dbg.glitch.me/
2. Open the console
3. There should be an error message, click on its location ( script.js line 2 > eval:1:7)
4. Click on the location

**Expected results**

This opens the debugger

**Actual results**

A new tab is open `view-source:https://devtools-console-eval-throw-link-dbg.glitch.me/script.js%20line%202%20%3E%20eval`, only displaying "Not Found" in the content page.

---

Attachments

(13 files, 10 obsolete files)

49.03 KB, image/png
Details
23.79 KB, image/png
Details
50.09 KB, image/png
Details
6.32 KB, patch
loganfsmyth
: review+
Details | Diff | Splinter Review
4.84 KB, patch
loganfsmyth
: review+
Details | Diff | Splinter Review
60.46 KB, patch
Details | Diff | Splinter Review
1.85 KB, patch
jimb
: review+
Details | Diff | Splinter Review
11.92 KB, patch
jimb
: review+
Details | Diff | Splinter Review
15.87 KB, patch
jimb
: review+
Details | Diff | Splinter Review
2.32 KB, patch
jimb
: review+
Details | Diff | Splinter Review
12.69 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.42 KB, patch
loganfsmyth
: review+
Details | Diff | Splinter Review
4.94 KB, patch
loganfsmyth
: review+
Details | Diff | Splinter Review
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.
Attached image Firefox_Nightly.png
Component: Untriaged → Developer Tools: Console
Priority: -- → P2
Product: Firefox → DevTools
I can't run the provided example anymore, can you still reproduce this error?
Flags: needinfo?(joshunger1)
Attached image Firefox_Nightly.png
Flags: needinfo?(joshunger1)
The text looks better but the hyperlink loads as Not Found.
Is there a way I can reproduce this issue?
Summary: Devtools error console link to debugger shows invalid link and encoded text → Devtools error console link to debugger shows invalid link

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!

User Story: (updated)
User Story: (updated)

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.

Attached patch WIP (obsolete) — Splinter Review

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.

Assignee: nobody → bhackett1024
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch patch (obsolete) — Splinter Review

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.

Attachment #9036764 - Attachment is obsolete: true

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.

Attachment #9037032 - Flags: review?(jorendorff)

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.

Attachment #9037034 - Flags: review?(jorendorff)

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

Attachment #9037040 - Flags: review?(jorendorff)
Attached patch Part 4 - Add DebuggerSource.tag. (obsolete) — Splinter Review

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

Attachment #9037041 - Flags: review?(jorendorff)

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

Attachment #9037042 - Flags: review?(bugs)

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.

Attachment #9037043 - Flags: review?(lsmyth)

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.

Attachment #9037045 - Flags: review?(lsmyth)

Add a test for the new functionality.

Attachment #9037047 - Flags: review?(lsmyth)

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

Attachment #9037058 - Flags: review?(lsmyth)
Blocks: 1520623
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.

(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 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
Attachment #9037032 - Flags: review?(jorendorff) → review?(jimb)
Attachment #9037034 - Flags: review?(jorendorff) → review?(jimb)
Attachment #9037040 - Flags: review?(jorendorff) → review?(jimb)
Attachment #9037041 - Flags: review?(jorendorff) → review?(jimb)
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 ?
Attachment #9037042 - Flags: review?(bugs) → review-

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

Attached patch patchSplinter Review

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

Attachment #9037023 - Attachment is obsolete: true

Updated patch for s/tag/id.

Attachment #9037032 - Attachment is obsolete: true
Attachment #9037032 - Flags: review?(jimb)
Attachment #9038006 - Flags: review?(jimb)

Updated patch for s/tag/id.

Attachment #9038007 - Flags: review?(jimb)
Attachment #9037034 - Attachment is obsolete: true
Attachment #9037034 - Flags: review?(jimb)

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.

Attachment #9037040 - Attachment is obsolete: true
Attachment #9037040 - Flags: review?(jimb)
Attachment #9038008 - Flags: review?(jimb)

Updated patch for s/tag/id.

Attachment #9037041 - Attachment is obsolete: true
Attachment #9037041 - Flags: review?(jimb)
Attachment #9038009 - Flags: review?(jimb)

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.

Attachment #9037042 - Attachment is obsolete: true
Attachment #9038014 - Flags: review?(bugs)

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.

Attachment #9037043 - Attachment is obsolete: true
Attachment #9037043 - Flags: review?(lsmyth)
Attachment #9038016 - Flags: review?(lsmyth)
Attachment #9038014 - Flags: review?(bugs) → review+
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 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.
Attachment #9037045 - Flags: review?(lsmyth) → review+
Attachment #9037058 - Flags: review?(lsmyth) → review+
Attachment #9037047 - Flags: review?(lsmyth) → review+

Updated patch per comments.

Attachment #9038016 - Attachment is obsolete: true
Attachment #9038016 - Flags: review?(lsmyth)
Attachment #9039372 - Flags: review?(lsmyth)
Attachment #9039372 - Flags: review?(lsmyth) → review+
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 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
Attachment #9038006 - Flags: review?(jimb) → review+
Attachment #9038007 - Flags: review?(jimb) → review+
Attachment #9038008 - Flags: review?(jimb) → review+
Attachment #9038009 - Flags: review?(jimb) → review+

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

Attached patch followupSplinter Review

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.

Attachment #9037058 - Attachment is obsolete: true
Attachment #9045816 - Flags: review?(lsmyth)
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?
Attachment #9045816 - Flags: review?(lsmyth) → review+

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

Pushed by bhackett@mozilla.com:
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.
Depends on: 1530641
Whiteboard: [good first verify]

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

Whiteboard: [good first verify] → [good first verify] [bugday-20190327]
Duplicate of this bug: 1109612
Duplicate of this bug: 1189533
You need to log in before you can comment on or make changes to this bug.