967718 - [jsdbg2] Debugger fails to ignore self-hosted scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Debugger should ignore self-hosted scripts, but we still report onEnterFrame events for them, and I guess we must be generating Debugger.Frame instances for them as well:

$ hg identify
f550b112a19b tip central
$ cat ~/moz/trace-self-hosted.js
var g = newGlobal();
var dbg = new Debugger(g);
dbg.onEnterFrame = function (frame) {
  print("> " + frame.script.url);
}

g.eval("(" + function iife() {
  [1].forEach(function noop() {});
  for (let x of [1]) {}
} + ")()");
$ obj~/js/src/js -f ~/moz/trace-self-hosted.js
> /home/jimb/moz/trace-self-hosted.js
> /home/jimb/moz/trace-self-hosted.js
> self-hosted
> /home/jimb/moz/trace-self-hosted.js
> self-hosted
> self-hosted
> self-hosted
> self-hosted
> self-hosted
> self-hosted
$

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Observe observance rules in Debugger more systematically.

Comment 2

[Jim Blandy :jimb]

Comment on attachment 8373738 [details] [diff] [review]
Observe observance rules in Debugger more systematically.

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

This all looks reasonable --- just some questions:

::: js/src/vm/Debugger.cpp
@@ -520,5 @@
>  
>      if (GlobalObject::DebuggerVector *debuggers = global->getDebuggers()) {
>          for (Debugger **p = debuggers->begin(); p != debuggers->end(); p++) {
>              Debugger *dbg = *p;
> -            JS_ASSERT(dbg->observesFrame(frame));

Since you're turning this assertion into a test --- did we have a test case that actually triggers the assertion?

@@ +3511,5 @@
>  
>  bool
>  Debugger::observesFrame(AbstractFramePtr frame) const
>  {
> +    return observesScript(frame.script());

I *think* this is okay, but:

We used to have scripts that we ran against multiple globals --- that is, script and global could be in different compartments. I believe that now we clone a script into the global's compartment before running it against that global, in which case this change is fine. But we should ask someone who knows...

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #2)
> Comment on attachment 8373738 [details] [diff] [review]
> Observe observance rules in Debugger more systematically.
> 
> Review of attachment 8373738 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This all looks reasonable --- just some questions:
> 
> ::: js/src/vm/Debugger.cpp
> @@ -520,5 @@
> >  
> >      if (GlobalObject::DebuggerVector *debuggers = global->getDebuggers()) {
> >          for (Debugger **p = debuggers->begin(); p != debuggers->end(); p++) {
> >              Debugger *dbg = *p;
> > -            JS_ASSERT(dbg->observesFrame(frame));
> 
> Since you're turning this assertion into a test --- did we have a test case
> that actually triggers the assertion?
> 

It was changed not because the assert was ever triggered, but because it's a more sensible place to test for whether or not a Debugger should ignore self-hosted scripts.

I doubt it's triggerable in the old code since it just makes sure the global is observable to the debugger.

> @@ +3511,5 @@
> >  
> >  bool
> >  Debugger::observesFrame(AbstractFramePtr frame) const
> >  {
> > +    return observesScript(frame.script());
> 
> I *think* this is okay, but:
> 
> We used to have scripts that we ran against multiple globals --- that is,
> script and global could be in different compartments. I believe that now we
> clone a script into the global's compartment before running it against that
> global, in which case this change is fine. But we should ask someone who
> knows...

Okay, I'll ask someone.

Comment 4

[Shu-yu Guo [:shu]]

For the second review point above:

17:37 < jimb> Do we ever actually execute scripts against multiple globals, or do we clone them into the global's compartment before we run them?
17:53 < billm> jimb: clone

Comment 5

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12f47ee14e2c

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12f47ee14e2c