[jsdbg2] Debugger fails to ignore self-hosted scripts

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimb, Assigned: shu)

Tracking

unspecified
mozilla30
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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
$
(Reporter)

Updated

5 years ago
Blocks: 967234
(Reporter)

Updated

5 years ago
Assignee: nobody → shu
(Assignee)

Comment 1

5 years ago
Created attachment 8373738 [details] [diff] [review]
Observe observance rules in Debugger more systematically.
Attachment #8373738 - Flags: review?(jimb)
(Reporter)

Comment 2

5 years ago
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...
(Reporter)

Updated

5 years ago
Flags: needinfo?(shu)
(Assignee)

Comment 3

5 years ago
(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.
Flags: needinfo?(shu)
(Assignee)

Comment 4

5 years ago
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
(Reporter)

Updated

5 years ago
Attachment #8373738 - Flags: review?(jimb) → review+
https://hg.mozilla.org/mozilla-central/rev/12f47ee14e2c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.