933460 - [jsdbg2] Should be able to pass "displayUrl" parameter into findScripts queries

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Right now you can filter scripts via url/line/column/innermost, it would be awesome if we could also filter by "displayUrl" aka "//# sourceURL=<url>".

Comment 1

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

Attachment: bug-933460-filter-display-url.patch

Note the "TODO" question/comment in Debugger.cpp. Not sure if that line is needed, or if even more nullptr setting is required.

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

Comment 2

[Eddy Bruel [:ejpbruel]]

Comment on attachment 830455 [details] [diff] [review]
bug-933460-filter-display-url.patch

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

Almost good enough. Mostly small comments. If you address those I should be able to give you r+ quickly.

::: js/src/jit-test/tests/debug/Debugger-findScripts-18.js
@@ +21,5 @@
> +var gScripts = dbg.findScripts({ displayURL: "g.js" });
> +assertEq(gScripts.indexOf(gw.script) != -1, true);
> +assertEq(gScripts.indexOf(fw.script), -1);
> +assertEq(gScripts.indexOf(hw.script), -1);
> +

Please add a test with the query argument omitted.

::: js/src/vm/CommonPropertyNames.h
@@ +49,5 @@
>      macro(defineGetter, defineGetter, "__defineGetter__") \
>      macro(defineSetter, defineSetter, "__defineSetter__") \
>      macro(delete, delete_, "delete") \
>      macro(deleteProperty, deleteProperty, "deleteProperty") \
> +    macro(displayURL, displayURL, "displayURL") \

See comment on displayURL vs sourceURL below

::: js/src/vm/Debugger.cpp
@@ +2378,5 @@
>              }
>          }
>  
> +        /* Check for a 'displayURL' property. */
> +        PropertyName *displayURLName = cx->names().displayURL;

Please don't use a temporary variable for this.

@@ +2392,5 @@
>          return true;
>      }
>  
>      /* Set up this ScriptQuery appropriately for a missing query argument. */
>      bool omittedQuery() {

This function is called by Debugger::findScripts when you omit the query argument. Please add displayURL = nullptr here.

@@ +2435,5 @@
>          }
>  
> +        // TODO FITZGEN: is this necessary? Do I need to put this before every return?
> +        displayURLChars = nullptr;
> +

As far as I can tell, instances of ScriptQuery are only created by Debugger::findScripts, and ScriptQuery::findScripts is only called once for each instance. There is no need to set displayURLChars to nullptr here, since it won't ever be used again after this function returns.

@@ +2461,5 @@
>  
> +    /* If this is a string, matching scripts' sources have displayURLs equal to
> +     * it. */
> +    RootedValue displayURL;
> +

Please name this sourceURL, since that is what we use throughout the rest of the code. Either that, or you can rename all occurrences of sourceURL to displayURL. All I care about is that we're consistent. Using different names for the same concept within the same code is highly confusing. 

You can still use "displayURL" for the name of the property on the query object, if that's what you prefer.

@@ +2529,5 @@
>              if (!urlCString.encodeLatin1(cx, url.toString()))
>                  return false;
>          }
> +        if (displayURL.isString()) {
> +            displayURLChars = JS_GetStringCharsAndLength(cx, displayURL.toString(),

Since this code is internal to SpiderMonkey, you probably shouldn't use a JSAPI function for this. Use the length() and getChars() method on JSString instead.

@@ +2572,5 @@
> +                              &result)) {
> +                oom = true;
> +                return;
> +            }
> +            if (result)

result is not a bool, so please use result != 0 here instead.

Comment 3

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

Attachment: filter-display-url.patch

Updated based on review comments.

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

Comment 4

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8343380 [details] [diff] [review]
filter-display-url.patch

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

Looks good Nick! r+ with green try run and comments addressed.

::: js/src/vm/CommonPropertyNames.h
@@ +51,5 @@
>      macro(defineGetter, defineGetter, "__defineGetter__") \
>      macro(defineSetter, defineSetter, "__defineSetter__") \
>      macro(delete, delete_, "delete") \
>      macro(deleteProperty, deleteProperty, "deleteProperty") \
> +    macro(sourceURL, sourceURL, "displayURL") \

Common property names should always match their actual string, or this would lead to very confusing code. Simply change sourceURL to displayURL here.

::: js/src/vm/Debugger.cpp
@@ +2412,5 @@
>              }
>          }
>  
> +        /* Check for a 'displayURL' property. */
> +        if (!JSObject::getProperty(cx, query, query, cx->names().sourceURL, &sourceURL))

If you choose to keep the name of the property as "displayURL", that is fine. However, then you should also make that explicit by using cx->names().displayURL here.

Perhaps for good measure, you could also add a comment here explaining that what we call displayURL externally is called sourceURL internally?

Comment 5

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

Attachment: part 0: add "displayURL" filtering to findScripts

Carrying over r+ from Eddy.

Comment 6

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

Attachment: part 1: mechanical s/sourceURL/displayURL/ through out spidermonkey

This is a mechanical change, renaming all things sourceURL to displayURL inside of spidermonkey, as discussed with Eddy on irc a little while back.

Comment 7

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

Try push: https://tbpl.mozilla.org/?tree=Try&rev=8c954bfd6814

Will land if push results look good.

Comment 8

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

https://hg.mozilla.org/integration/fx-team/rev/836f2dca0336
https://hg.mozilla.org/integration/fx-team/rev/869a5af0fd20

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/836f2dca0336
https://hg.mozilla.org/mozilla-central/rev/869a5af0fd20