Open
Bug 1224622
Opened 9 years ago
Updated 2 years ago
inconsistent naming of debugger eval scripts
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
NEW
People
(Reporter: jlong, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
6.80 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
8.97 KB,
patch
|
jlong
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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•9 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`?
Comment 3•9 years ago
|
||
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.
Updated•9 years ago
|
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•9 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.
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
Attachment #8687336 -
Flags: review?(jlong)
Comment 7•9 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86a46da27905
Reporter | ||
Comment 8•9 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•9 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+
Updated•6 years ago
|
Assignee: nfitzgerald → nobody
Status: ASSIGNED → NEW
Updated•4 years ago
|
Blocks: js-debugger
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•