1037675 - [jsdbg2] add `source` as a query param to `findScripts` and fix `displayURL`

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[James Long (:jlongster)]

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

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

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);
};

Comment 2

[James Long (:jlongster)]

(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

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

I think it will be faster if you leave it, since more of the filtering will be done in C++.

Comment 4

[James Long (:jlongster)]

Unfortunately line/col require the url, but I can use that if the url exists, otherwise not, until this bug is fixed.

Comment 5

[James Long (:jlongster)]

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.

Comment 6

[James Long (:jlongster)]

Attachment: 1037675.patch

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?

Comment 7

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

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

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

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.

Comment 9

[James Long (:jlongster)]

Attachment: 1037675.patch

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

Comment 10

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

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

Comment 11

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

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?

Comment 12

[James Long (:jlongster)]

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

Comment 13

[James Long (:jlongster)]

So I tried to make the `source` property of `Debugger::ScriptQuery` a `

Comment 14

[James Long (:jlongster)]

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

Comment 15

[James Long (:jlongster)]

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?

Comment 16

[James Long (:jlongster)]

Attachment: 1037675.patch

Comment 17

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

(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

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

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

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

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

Comment 20

[James Long (:jlongster)]

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

Comment 21

[James Long (:jlongster)]

Attachment: 1037675.patch

Got everything working, just need proper review now.

Comment 22

[James Long (:jlongster)]

Attachment: 1037675.patch

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?

Comment 23

[Jason Orendorff [:jorendorff]]

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 ...
     */

Comment 24

[James Long (:jlongster)]

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

[Jason Orendorff [:jorendorff]]

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

Comment 26

[James Long (:jlongster)]

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.

Comment 27

[James Long (:jlongster)]

Attachment: 1037675.patch

Do you mind looking over this again? I rebased it and added support for a query with displayURL & line. Should be a quick review.

Comment 28

[Jason Orendorff [:jorendorff]]

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.

Comment 29

[James Long (:jlongster)]

https://tbpl.mozilla.org/?tree=Try&rev=a2213832f260

Several other patches there... hope those don't cause failures

Comment 30

[Carsten Book [:Tomcat]]

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?

Comment 31

[James Long (:jlongster)]

Attachment: 1037675.patch

Appears that I forgot to attach the updated patch. The tests were run with this patch.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe3f3603f56

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ebe3f3603f56