Closed
Bug 1037675
Opened 10 years ago
Closed 10 years ago
[jsdbg2] add `source` as a query param to `findScripts` and fix `displayURL`
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: jlong, Assigned: jlong)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
The docs for the `Debugger.findScripts` method say `source` should be a query param: https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger. This isn't implemented yet. We should implement it.
I am moving the debugger server to use sources for everything instead of URLs. `eval` sources need to lookup the scripts associated with it to set breakpoints. We happen to able to get by because right now those sources do have a fake URL attached to them, which seems to work with `findScripts`. But that's an error and soon those sources should have `null` as URLs, and we won't be able to use `findScripts` until we can query on `source`.
Comment 1•10 years ago
|
||
In the meantime, you could polyfill it and it won't even be much slower because findScripts doesn't do any indexing or anything:
const origFindScripts = Debugger.prototype.findScripts;
Debugger.prototype.findScripts = function (query) {
return origFindScripts.call(this, query)
.filter(s => query.source ? s.source === query.source : true);
};
Updated•10 years ago
|
Component: Developer Tools: Debugger → JavaScript Engine
Product: Firefox → Core
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] from comment #1)
> In the meantime, you could polyfill it and it won't even be much slower
> because findScripts doesn't do any indexing or anything:
>
> const origFindScripts = Debugger.prototype.findScripts;
> Debugger.prototype.findScripts = function (query) {
> return origFindScripts.call(this, query)
> .filter(s => query.source ? s.source ===
> query.source : true);
> };
Yeah, I was going to do that manually in the code. The original call also passed line/column info: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/script.js#L1516. It looks like we figure out the offset positions later on though based on line/col, so I think it's safe just to drop the line/col from the `findScripts` query?
Comment 3•10 years ago
|
||
I think it will be faster if you leave it, since more of the filtering will be done in C++.
Assignee | ||
Comment 4•10 years ago
|
||
Unfortunately line/col require the url, but I can use that if the url exists, otherwise not, until this bug is fixed.
Assignee | ||
Comment 5•10 years ago
|
||
Hm, I think this should be high priority, actually. I tried just doing `findScripts({ innermost: true })` but got a TypeError:
TypeError: findScripts query object has 'innermost' property without both 'url' and 'line' properties
I need these queries for figuring out where the set breakpoints. Simulating the innermost I think will be difficult. I suspect it would be easy to add the `source` query param. Am I wrong? In fact I can try to do it if someone will mentor me.
Assignee | ||
Comment 6•10 years ago
|
||
This is what I have so far, but it's not working. The check in `parseQuery` that does `source.toObject().is<ScriptSourceObject>()` fails when you pass in the result of `script.source` from JS. What am I doing wrong there?
Attachment #8459853 -
Flags: review?(jimb)
Comment 7•10 years ago
|
||
Comment on attachment 8459853 [details] [diff] [review]
1037675.patch
Review of attachment 8459853 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/vm/Debugger.cpp
@@ +2585,5 @@
> + if (!JSObject::getProperty(cx, query, query, cx->names().source, &source)) {
> + return false;
> + }
> + // A source object returned from `script.source` in JS always
> + // fails the `.is<ScriptSourceObject>` check
You probably need to unwrap it, otherwise it will be a Proxy object.
Comment 8•10 years ago
|
||
Or actually, you have Debugger.Source objects, which hold a reference to the ScriptSourceObject. So you want to ensure that `source.getClass() == DebuggerSource_class`. Since there is no DebuggerSource class that inherits from JSObject, you don't get the nice is<T> and as<T> methods.
Assignee | ||
Comment 9•10 years ago
|
||
Oh, you're right! I forgot it was a Debugger.Source, not a real source object.
This patch works! The only thing I'm not sure about is that I needed to use `GetSourceReferent` but that's defined later in the file, so I needed to declare it early. Where should I put that (or how should I do it differently)?
Attachment #8459853 -
Attachment is obsolete: true
Attachment #8459853 -
Flags: review?(jimb)
Attachment #8459894 -
Flags: review?(nfitzgerald)
Comment 10•10 years ago
|
||
Comment on attachment 8459894 [details] [diff] [review]
1037675.patch
Review of attachment 8459894 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me, but I'm not a peer here. Bouncing to :jorendorff.
::: js/src/vm/Debugger.cpp
@@ +2585,5 @@
>
> + /* Check for a 'source' property */
> + if (!JSObject::getProperty(cx, query, query, cx->names().source, &source)) {
> + return false;
> + }
Style nit: SpiderMonkey drops braces on single line consequents.
@@ +2674,5 @@
> const jschar *displayURLChars;
> size_t displayURLLength;
>
> + /* If this is a source object, matching scripts will have sources
> + equal to this instance. */
Nit: "*" before "equal" to match other comments.
@@ +2809,5 @@
> }
> + if (source.isObject()) {
> + RootedScriptSource sourceObject(cx, GetSourceReferent(&source.toObject()));
> + if(sourceObject) {
> + if(sourceObject != script->sourceObject())
Nit: space after "if".
Attachment #8459894 -
Flags: review?(nfitzgerald)
Attachment #8459894 -
Flags: review?(jorendorff)
Attachment #8459894 -
Flags: feedback+
Comment 11•10 years ago
|
||
Comment on attachment 8459894 [details] [diff] [review]
1037675.patch
Review of attachment 8459894 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/vm/Debugger.cpp
@@ +2807,5 @@
> return;
> }
> }
> + if (source.isObject()) {
> + RootedScriptSource sourceObject(cx, GetSourceReferent(&source.toObject()));
Rather than saving the D.Source when validating the ScriptQuery and then converting it to a ScriptSourceObject here, why not save the ScriptSourceObject when validating and then just do the comparison here?
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> Rather than saving the D.Source when validating the ScriptQuery and then
> converting it to a ScriptSourceObject here, why not save the
> ScriptSourceObject when validating and then just do the comparison here?
I can do that. At first I was following the parsing of `displayURL` which seemed to snag the RootedValue in `parseQuery` and extract stuff from it later. But it makes sense for me to do it there.
Assignee | ||
Comment 13•10 years ago
|
||
So I tried to make the `source` property of `Debugger::ScriptQuery` a `
Assignee | ||
Comment 14•10 years ago
|
||
Doh, somehow tab->entered. So I tried to make the `source` property of `Debugger::ScriptQuery` a `RootedScriptSource`. The problem I have now is that it can be null. I initialize it in the ScriptQuery constuctor with just the context, but I don't see anywhere else that just initialized a `RootedScriptSource` with a context, they always also take a `ScriptSourceObject`. Is it possible for a `RootedScriptSource` to somehow also be null?
I tried getting the `ScriptSource` from the `RootedScriptSource` and seeing that was null (source->source()) but that call crashes now if it's not initialized with a `ScriptSource`.
Assignee | ||
Comment 15•10 years ago
|
||
After thinking about it more, `ScriptSourceObject` is an instance of `JSObject` so it can't be null. The question is if a Rooted thing can ever have a null pointer (the internal ptr seems to be set to `js::GCMethods<T>::initial()` by default, and I can't find the implementation for `ScriptSourceObject*`).
How can I check if `RootedScriptSource` has an actual source attached to it, and if there isn't an easy way, I should go back to what I did before right?
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8459894 -
Attachment is obsolete: true
Attachment #8459894 -
Flags: review?(jorendorff)
Attachment #8460051 -
Flags: review?(jorendorff)
Attachment #8460051 -
Flags: feedback+
Assignee | ||
Updated•10 years ago
|
Attachment #8460051 -
Flags: feedback+
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jlong
Comment 17•10 years ago
|
||
(In reply to James Long (:jlongster) from comment #15)
> After thinking about it more, `ScriptSourceObject` is an instance of
> `JSObject` so it can't be null. The question is if a Rooted thing can ever
> have a null pointer (the internal ptr seems to be set to
> `js::GCMethods<T>::initial()` by default, and I can't find the
> implementation for `ScriptSourceObject*`).
Yes, a Rooted can point to null.
ScriptSourceObject* is just a pointer to a ScriptSourceObject, so it can definitely be null.
> How can I check if `RootedScriptSource` has an actual source attached to it,
> and if there isn't an easy way, I should go back to what I did before right?
if (someRootedScriptSource) {
// in here you know it doesn't point to null
}
Comment 18•10 years ago
|
||
The point I was trying to make in comment 11 was that you have two phases: one for validation/initialization of ScriptQuery (which happens once) and one for matching scripts to the ScriptQuery (which happens many times.
In that version of the patch, you were repeatedly unwrapping a D.Source into a SSO in the matching phase, when it seemed to me like you should only do that the one time in the initialization phase. In the matching phase, you shouldn't need to do anymore unwrapping or value shuffling from the query object, because it should already have been taken care of in the initialization phase. All the checks for a valid SSO should happen in the intitialization phase.
Maybe I'm misunderstanding your questions?
Comment 19•10 years ago
|
||
Comment on attachment 8460051 [details] [diff] [review]
1037675.patch
Review of attachment 8460051 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/vm/Debugger.cpp
@@ +2594,5 @@
> + "not undefined nor a Debugger.Source object");
> + return false;
> + }
> + else {
> + source = GetSourceReferent(&debuggerSource.toObject());
Nit: I'd drop the `else` to have one less level of indentation.
@@ +2680,5 @@
> size_t displayURLLength;
>
> + /* If this is a source object, matching scripts will have sources
> + * equal to this instance. */
> + ScriptSourceObject* source;
This needs to be a RootedScriptSource or else the GC might move the SSO and you'll end up with a dangling pointer.
In the constructor you can just initialize it with a cx, and it will point to nullptr by default.
@@ +2811,5 @@
> if (CompareChars(s, js_strlen(s), displayURLChars, displayURLLength) != 0) {
> return;
> }
> }
> + if (source != nullptr && source != script->sourceObject())
Nit:
if (source && source != script->sourceObject())
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] from comment #18)
>
> Maybe I'm misunderstanding your questions?
Not at all, we're on the same page, I was just thinking out loud as I was trying to implement the change. I had to figure out how to handle "null", that's all.
Thanks, I didn't realize you could check rooted values like that. I'll try that and make the `ScriptSourceObject` rooted again.
Assignee | ||
Comment 21•10 years ago
|
||
Got everything working, just need proper review now.
Attachment #8460051 -
Attachment is obsolete: true
Attachment #8460051 -
Flags: review?(jorendorff)
Attachment #8460593 -
Flags: review?(jorendorff)
Assignee | ||
Updated•10 years ago
|
Blocks: dbg-source
Assignee | ||
Comment 22•10 years ago
|
||
Ok I lied, this should be the final patch. I needed to allow line, col, or innermost to be present when source is specified. Previously it only allowed queries with those params with url.
I also needed to update one of the .msg files... what's the process for that? Are those localized?
Attachment #8460593 -
Attachment is obsolete: true
Attachment #8460593 -
Flags: review?(jorendorff)
Attachment #8460898 -
Flags: review?(jorendorff)
Comment 23•10 years ago
|
||
Comment on attachment 8460898 [details] [diff] [review]
1037675.patch
Review of attachment 8460898 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with these nits picked. Thanks.
::: js/src/js.msg
@@ +342,5 @@
> MSG_DEF(JSMSG_DEBUG_NO_SCOPE_OBJECT, 289, 0, JSEXN_TYPEERR, "declarative Environments don't have binding objects")
> MSG_DEF(JSMSG_EMPTY_CONSEQUENT, 290, 0, JSEXN_SYNTAXERR, "mistyped ; after conditional?")
> MSG_DEF(JSMSG_NOT_ITERABLE, 291, 1, JSEXN_TYPEERR, "{0} is not iterable")
> +MSG_DEF(JSMSG_QUERY_LINE_WITHOUT_URL, 292, 0, JSEXN_TYPEERR, "findScripts query object has 'line' property, but no 'url' or 'source' property")
> +MSG_DEF(JSMSG_QUERY_INNERMOST_WITHOUT_LINE_URL, 293, 0, JSEXN_TYPEERR, "findScripts query object has 'innermost' property without a 'url' or 'source' and 'line' properties")
This error message confused me.
Consider:
"findScripts query object with 'innermost' property must have 'line' and either 'url' or 'source'"
or split it into two error messages:
"findScripts query object with 'innermost' property must have 'line'"
"findScripts query object with 'innermost' property must have either 'url' or 'source'"
or whatever would be better...
::: js/src/vm/Debugger.cpp
@@ +2470,5 @@
> /*
> * A class for parsing 'findScripts' query arguments and searching for
> * scripts that match the criteria they represent.
> */
> class Debugger::ScriptQuery {
Pre-existing, but please add MOZ_STACK_CLASS on this while you're in here.
Also put that { on a separate line (per SpiderMonkey house style).
@@ +2536,5 @@
> + RootedValue debuggerSource(cx);
> + if (!JSObject::getProperty(cx, query, query, cx->names().source, &debuggerSource))
> + return false;
> + if(!debuggerSource.isUndefined()) {
> + if(debuggerSource.toObject().getClass() != &DebuggerSource_class) {
Nit: leave a space between 'if' and '(' on these two lines.
debuggerSource.toObject() will assert if debuggerSource isn't an object (i.e. it's a number, string, symbol, boolean, or null).
@@ +2543,5 @@
> + "not undefined nor a Debugger.Source object");
> + return false;
> + }
> +
> + source = RootedScriptSource(cx, GetSourceReferent(&debuggerSource.toObject()));
You can assign a pointer to a Rooted:
source = GetSourceReferent(...);
@@ +2678,5 @@
> const jschar *displayURLChars;
> size_t displayURLLength;
>
> + /* If this is a source object, matching scripts will have sources
> + * equal to this instance. */
Style micro-nit: Please change this and the other multiline comments in here to the house style.
/*
* If this is ...
* equal ...
*/
Attachment #8460898 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 24•10 years ago
|
||
Ok, I've fixed everything but the following one thing. Thanks!
> debuggerSource.toObject() will assert if debuggerSource isn't an object (i.e. it's a number, string, symbol, boolean, or null).
Are you saying I should add a check like `if(!debuggerSource.isObject() ...) {}` ?
Comment 25•10 years ago
|
||
(In reply to James Long (:jlongster) from comment #24)
> Ok, I've fixed everything but the following one thing. Thanks!
>
> > debuggerSource.toObject() will assert if debuggerSource isn't an object (i.e. it's a number, string, symbol, boolean, or null).
>
> Are you saying I should add a check like `if(!debuggerSource.isObject() ...)
> {}` ?
Yes.
Assignee | ||
Updated•10 years ago
|
Summary: [jsdbg2] add `source` as a query param to `findScripts` → [jsdbg2] add `source` as a query param to `findScripts` and fix `displayURL`
Assignee | ||
Comment 26•10 years ago
|
||
I noticed that you can't also query for `displayURL` and `line` so I fixed that in this patch because it's such a small change.
Assignee | ||
Comment 27•10 years ago
|
||
Do you mind looking over this again? I rebased it and added support for a query with displayURL & line. Should be a quick review.
Attachment #8460898 -
Attachment is obsolete: true
Attachment #8476352 -
Flags: review?(jorendorff)
Comment 28•10 years ago
|
||
Comment on attachment 8476352 [details] [diff] [review]
1037675.patch
Review of attachment 8476352 [details] [diff] [review]:
-----------------------------------------------------------------
Add a test for displayURL+line, if there isn't already one.
::: js/src/js.msg
@@ +342,5 @@
> MSG_DEF(JSMSG_DEBUG_NO_SCOPE_OBJECT, 289, 0, JSEXN_TYPEERR, "declarative Environments don't have binding objects")
> MSG_DEF(JSMSG_EMPTY_CONSEQUENT, 290, 0, JSEXN_SYNTAXERR, "mistyped ; after conditional?")
> MSG_DEF(JSMSG_NOT_ITERABLE, 291, 1, JSEXN_TYPEERR, "{0} is not iterable")
> +MSG_DEF(JSMSG_QUERY_LINE_WITHOUT_URL, 292, 0, JSEXN_TYPEERR, "findScripts query object has 'line' property, but no 'url' or 'source' property")
> +MSG_DEF(JSMSG_QUERY_INNERMOST_WITHOUT_LINE_URL, 293, 0, JSEXN_TYPEERR, "findScripts query object with 'innermost' property must have 'line' and either 'url' or 'source'")
Maybe the error messages should be updated to mention displayURL?
::: js/src/vm/Debugger.cpp
@@ +2735,5 @@
> /* If this is a string, matching scripts' sources have displayURLs equal to
> * it. */
> RootedLinearString displayURLString;
>
> + /*
Please remove the space character at the end of this line.
Attachment #8476352 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 29•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a2213832f260
Several other patches there... hope those don't cause failures
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 30•10 years ago
|
||
seems the patch failed to apply:
applying 1037675.patch
patching file js/src/js.msg
Hunk #1 FAILED at 336
1 out of 1 hunks FAILED -- saving rejects to file js/src/js.msg.rej
patch failed, unable to continue (try -v)
James, could you take a look into this and maybe rebase?
Keywords: checkin-needed
Assignee | ||
Comment 31•10 years ago
|
||
Appears that I forgot to attach the updated patch. The tests were run with this patch.
Attachment #8476352 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 32•10 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 33•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•