inconsistent naming of debugger eval scripts

NEW
Unassigned

Status

()

3 years ago
5 months ago

People

(Reporter: jlong, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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.
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.
(Reporter)

Comment 2

3 years ago
(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`?
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.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
(Reporter)

Comment 4

3 years ago
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.
Created attachment 8687335 [details] [diff] [review]
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.
Attachment #8687335 - Flags: review?(shu)
Created attachment 8687336 [details] [diff] [review]
Part 1: Use introductionType == "debugger eval" to determine sources introduced by a debugger eval
Attachment #8687336 - Flags: review?(jlong)
(Reporter)

Comment 8

3 years ago
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.
Attachment #8687336 - Flags: review?(jlong) → review+

Comment 9

3 years ago
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
Attachment #8687335 - Flags: review?(shu) → review+
Assignee: nfitzgerald → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.