738480 - Debugger.prototype.findScripts does not find non-compileAndGo scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

It does not find them because the current code searches for scripts with the right global. I think compartment-per-global will clear this right up.

Comment 1

[Jim Blandy :jimb]

Adding [chrome-debug] to whiteboard; although this isn't chrome-specific, chrome debugging does need it.

The patch we whipped up is part of changeset ff20735ac16c in the remote-debug repository.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 4

[Jim Blandy :jimb]

Comment on attachment 626422 [details] [diff] [review]
v1

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

::: js/src/vm/Debugger.cpp
@@ +2187,5 @@
>       * if no error occurs, false if an error occurs.
>       */
> +    bool consider(JSScript *script, AutoScriptVector *vector) {
> +        GlobalObject *global = script->getGlobalObjectOrNull();
> +        if (global && !globals.has(global))

Could we have a one-line comment here explaining that non-CNG scripts can't be associated with any global, so we're just including them all?

Comment 5

[Jason Orendorff [:jorendorff]]

I don't have time to write a one-line comment, so I added a three-line comment

https://hg.mozilla.org/integration/mozilla-inbound/rev/58ca5bf07352

Comment 6

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/58ca5bf07352 - bustage a la https://tbpl.mozilla.org/php/getParsedLog.php?id=11995902&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=11996820&tree=Mozilla-Inbound

Comment 7

[Jason Orendorff [:jorendorff]]

I refreshed this and ran it by Try server, but it has some failures in debugger tests.

Comment 8

[Jim Blandy :jimb]

This patch was needed to see all the scripts we expected while debugging chrome code; it landed, but bounced. Panos, this might still be affecting our chrome debugging.

The patch includes a test, so we can see for sure.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Resetting component & flags that I presume were accidentally changed. This patch seems slightly bitrotted:

Debugger.cpp:2348:40: error: no member named 'getGlobalObjectOrNull' in 'JSScript'

I wanted to verify if this patch would fix some missing scripts I get with chrome debugging.

Comment 10

[Jim Blandy :jimb]

Attachment: Simplify Debugger::ScriptQuery after CPG. Fix missing compile-and-go scripts.

Since compartment-per-global, this code can be much simplified, and fix this bug in the process.

Will request review if try approves.

Comment 11

[Jim Blandy :jimb]

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

Comment 12

[Jim Blandy :jimb]

Seems to cause a bunch of timeouts in the debugger tests:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bfcache.js | Test timed out

and a bunch like this:

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/devtools/debugger/tests/unit/test_dbgactor.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | chrome://global/content/devtools/dbg-script-actors.js:1056 | TypeError: aScript.url is null - See following stack:

Comment 13

[Jim Blandy :jimb]

Seems like the debugger wasn't expecting to get any scripts that didn't have URLs; it does now.

Fresh try: https://tbpl.mozilla.org/?tree=Try&rev=d86bf86be673

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] from comment #9)
> I wanted to verify if this patch would fix some missing scripts I get with
> chrome debugging.

It does indeed!

(In reply to Jim Blandy :jimb from comment #13)
> Seems like the debugger wasn't expecting to get any scripts that didn't have
> URLs; it does now.

After we get Debugger.Source and are able to display the script contents without using the URL to fetch them, will it make sense to allow scripts without URLs again? Under what circumstances eval scripts get no URL? Using eval() in a page makes the script inherit the page URL, so I haven't come across this case before.

Comment 15

[Jim Blandy :jimb]

Still some interesting regressions here; for example:
https://tbpl.mozilla.org/php/getParsedLog.php?id=16149680&tree=Try&full=1

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jim Blandy :jimb from comment #15)
> Still some interesting regressions here; for example:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=16149680&tree=Try&full=1

Only the createRemote leak is new, the others are bug 753225 and bug 797336.

Comment 17

[Jim Blandy :jimb]

Luke, you'll enjoy this patch, I think.

The try run looks good; there is a debugger-related failure, but I think that's a known bug (starred).

Comment 18

[Jim Blandy :jimb]

Panos, for what it's worth, the stack traces printed after the "###!!! ASSERTION" (NS_ASSERTION) are produced assuming that we've got a frame pointer, which we don't; they're always bogus.

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 671586 [details] [diff] [review]
Simplify Debugger::ScriptQuery after CPG. Fix missing compile-and-go scripts.

cpg ftw!

Comment 20

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bea5e6ee825

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bea5e6ee825