1224622 - inconsistent naming of debugger eval scripts

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[James Long (:jlongster)]

If you eval code with `frame.eval`, it looks like we get a new source with a URL of `null` but an `introductionType` of "debugger eval". Looks like that is set here: https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#6661

However, if we eval code simple inside the global object without specifying a URL (like the console does), we get a new source with a URL of "debugger eval code": https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#6827

Is it intentional to separate these kinds of evals? We need to check for both in the debugger to ignore them, and it would be nice to make it more consistent.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

D.F.p.eval should use the same URL as D.O.p.evalInGlobal. They should also have the same introduction types.

However, we definitely need to ensure that evals originating from the Debugger API can be differentiated from someRandomGlobal.eval("..."). I suppose the introductionType ensures that.

Comment 2

[James Long (:jlongster)]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #1)
> However, we definitely need to ensure that evals originating from the
> Debugger API can be differentiated from someRandomGlobal.eval("..."). I
> suppose the introductionType ensures that.

`someRandomGlobal.eval("...")` is different from `D.O.p.evalInGlobal` right? The former always has a URL of `null` but the latter (as well as `D.F.p.eval`) should always have a URL of "debugger eval code".

In that case do we even need the `introductionType`?

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

The URL doesn't tell you what kind of source you're looking at, the introductionType does. You can infer for some types of sources in some environments by looking at the URL but it isn't bullet proof.

Comment 4

[James Long (:jlongster)]

Oh I didn't mean do away with `introductionType` entirely, I just meant set it to anything with debugger eval scripts. I don't really mind either away, as long I can just do `url === "debugger eval code"` and all internal evals are safely ignored I'm good.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Consistently name sources evaluated by the Debugger API

This commit makes `introductionType === "debugger eval"` the one source of truth
for whether a source was introduced by a debugger eval or not. Debugger
evaluated sources are now consistent with "normally" evaluated sources in that
both now have null urls. Additionally, it adds a test to ensure that
executeInGlobal[WithBindings] and eval[WithBindings] all create sources with the
same url and introductionType.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Use introductionType == "debugger eval" to determine sources introduced by a debugger eval

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86a46da27905

Comment 8

[James Long (:jlongster)]

Comment on attachment 8687336 [details] [diff] [review]
Part 1: Use introductionType == "debugger eval" to determine sources introduced by a debugger eval

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

Nice, now we don't even need to check for that special string.

::: devtools/server/actors/script.js
@@ +3764,5 @@
>      }
>  
>      return source.displayURL;
>    }
> +  else if (source.introductionType === "debugger eval") {

I don't think we even need this branch anymore. We had to force the URL to be null, but now it's null by default so we can remove this `else if` and just return `source.url` below.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8687335 [details] [diff] [review]
Part 0: Consistently name sources evaluated by the Debugger API

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

Cool