912536 - Assertion failure: cx->compartment()->debugMode(), at js/src/vm/ScopeObject.cpp:2181

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The patch in bug 810966 exposes this bug, but I'm not sure how to trigger it without that. It probably has something to do with scope chains for functions that are not stack frames, as that is what that bug is about. Actually, I hit the assertion when inspecting the scope chain of a function from the web console or scratchpad while the debugger is not paused on a breakpoint.

I have two separate STR, that lead to the same result:

(a) apply the patch from bug 810966, uncomment line 24 in that patch's browser_webconsole_closure_inspection.js mochitest and comment out line 25 instead (25-31 actually). Running the test hits the assertion.

(b) apply the patch, open http://jsbin.com/iqijok/1 and then open the web console (or a scratchpad). Click on the 'test' button and then type the following in the web console: inspect(foobar.getAge). If you are using a scratchpad just enter "foobar.getAge" and then press Ctrl-I (Cmd-I on Mac).

In both cases the stack appears to be garbage, so I'm not pasting it here.

Comment 1

[Jim Blandy :jimb]

I wonder if this is the same bug as bug 847405.

Comment 2

[Jim Blandy :jimb]

Panos, I've attached a patch to bug 847405; could you try it out and see if it helps?

Comment 3

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

Unfortunately that patch doesn't seem to have any effect on this bug. I still get crashes in both scenarios mentioned in comment 0.

Comment 4

[Jim Blandy :jimb]

I can reproduce this:

$ cat ~/moz/foo.js
var g = newGlobal();
var dbg = new Debugger;
var gw = dbg.addDebuggee(g);

g.f = function () { }
gw.getOwnPropertyDescriptor('f').value.unwrap().environment
$ obj~/js -f ~/moz/foo.js
Assertion failure: cx->compartment()->debugMode(), at /home/jimb/moz/dbg/js/src/vm/ScopeObject.cpp:2143
Segmentation fault (core dumped)
$

Comment 5

[Jim Blandy :jimb]

Attachment: Forbid access to non-debuggee environments.

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

Comment 6

[Jim Blandy :jimb]

Panos, that patch should work. If it's not too much trouble, please give it a try.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 802744 [details] [diff] [review]
Forbid access to non-debuggee environments.

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

r=me with questions addressed.

Hmm. Is there any way to tell if frame.script and friends are going to throw before you use them? It seems a little inconvenient, as an API. Wouldn't the existing code triggering this assertion now throw instead of crashing? It seems we need a patch for that, too...

Why is it better for non-debuggee Frame.script and Frame.environment to throw than to return null?

Actually I don't see in Debugger.cpp any changes to change Frame.script or Frame.environment. Were those already working somehow?

::: js/src/jit-test/tests/debug/Frame-script-environment-nondebuggee.js
@@ +13,5 @@
> +  assertEq(frame.environment instanceof Debugger.Environment, true);
> +
> +  // If we make g no longer a debuggee, then trying to touch the frame at
> +  // all show throw.
> +  dbg.removeDebuggee(g);

I suddenly wonder what's supposed to happen if we return a weird resumption value from this point. Do we have tests for that?

::: js/src/jit-test/tests/debug/Object-script-lazy.js
@@ -4,5 @@
> -// Note that putting a compartment in debug mode automatically de-lazifies
> -// all its functions (so that findScripts works); so the only way to get a
> -// Debugger.Object whose referent is a lazy function is to get one
> -// referring to a non-debuggee function. (I don't think we should be
> -// handing out non-debuggee scripts anyway.)

I say add g1 as a debuggee at the last moment before accessing it, update the comments as appropriate, and keep the test. Now it's testing something different, but that should needs to continue working.

Comment 8

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

This patch fixes the crash for me, but I need to handle the exception now in my tests as Jason notes. What I'm mostly wondering about though is why environments from closures do not appear when inspecting a closure from the console without the debugger paused. This question is perhaps more appropriate for bug 810966, but I hope you will forgive this slight tangent.

STR:

1) visit http://jsbin.com/iqijok/1
2) open the debugger and set a breakpoint at line 31 of the HTML page
3) click the "test" button on the page
3) inspect person.getAge in the variables view and see that the "name" and "age" variables are present
4) open the web console and type: inspect(person.getAge)
5) see that inspection of the closure still works
6) type: "inspect(window.foobar.getAge)" in the console and see that another closure can be inspected
7) now go to the debugger and resume execution and return to the web console
8) type "inspect(window.foobar.getAge)" again and notice that it no longer shows the closure and the browser console shows the error "notDebuggee: cannot access the environment of this function"

It seems that when execution is not paused, the scopes of those closures are no longer observable, even though they were before. Is this because of some engine limitation or is it a bug?

Comment 9

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

Forgot to note that you need the patch from bug 810966 applied to carry these STR.

Comment 10

[Jim Blandy :jimb]

Hmm... with the web console, and the patch from bug 914405, nothing is a debuggee of the web console's Debugger, ever, any more; we can do all the inspection the web console needs (well, until now) without it. This is good for simplicity (and perf), but it does clash with this bug's notion of restricting access to non-debuggee code. But I think the fix for that should simply be for the web console to have a global object manager like the content debugger's.

(For what it's worth, those access restrictions were part of the design of Debugger from the beginning...)

Comment 11

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #7)
> Hmm. Is there any way to tell if frame.script and friends are going to throw
> before you use them? It seems a little inconvenient, as an API. Wouldn't the
> existing code triggering this assertion now throw instead of crashing? It
> seems we need a patch for that, too...

There are three separate things going on here:

- First, we're avoiding handing out non-debuggee Environments in the first place. Note that D.O.p.environment returns null if the referent is not a debuggee; it doesn't throw.

- Second, if you do get a handle on one anyhow, we've made a non-debuggee Environment sort of inert and poisoned. But note that the only way to obtain such a thing is to remove debuggees; if you're doing that, you should be cleaning up whatever displays, etc. you have that refer to that no-longer-debuggee. It seems to me that throwing is better than having every accessor return some sort of quiescent value like null or undefined; users aren't really going to check for those every time they touch an environment. A 'live' property would probably be appropriate, though.

- Third, when you remove a debuggee, we mark all of its Debugger.Frame instances as dead. That's why they throw. But again, removing a debuggee is the only way to get such a D.F.

> Why is it better for non-debuggee Frame.script and Frame.environment to
> throw than to return null?
> 
> Actually I don't see in Debugger.cpp any changes to change Frame.script or
> Frame.environment. Were those already working somehow?

Right, this is because removing a global as a debuggee causes all its D.Fs to be marked dead, as explained above.

> ::: js/src/jit-test/tests/debug/Frame-script-environment-nondebuggee.js
> @@ +13,5 @@
> > +  assertEq(frame.environment instanceof Debugger.Environment, true);
> > +
> > +  // If we make g no longer a debuggee, then trying to touch the frame at
> > +  // all show throw.
> > +  dbg.removeDebuggee(g);
> 
> I suddenly wonder what's supposed to happen if we return a weird resumption
> value from this point. Do we have tests for that?

I'll check.

> ::: js/src/jit-test/tests/debug/Object-script-lazy.js
> @@ -4,5 @@
> > -// Note that putting a compartment in debug mode automatically de-lazifies
> > -// all its functions (so that findScripts works); so the only way to get a
> > -// Debugger.Object whose referent is a lazy function is to get one
> > -// referring to a non-debuggee function. (I don't think we should be
> > -// handing out non-debuggee scripts anyway.)
> 
> I say add g1 as a debuggee at the last moment before accessing it, update
> the comments as appropriate, and keep the test. Now it's testing something
> different, but that should needs to continue working.

I'll do that.

Action items:
- Add D.E.p.live.
- Add test for forcing weird resumption values from frames that are no longer debuggee frames.
- Add test for accessing non-debuggee scripts, with timing of debuggee-ness changed.

Comment 12

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #8)
> This patch fixes the crash for me, but I need to handle the exception now in
> my tests as Jason notes.

Why is this? Unless you are fetching environments, removing debuggees, and then accessing the newly-non-debuggee environments, I *think* you shouldn't be getting any exceptions. You should just be getting 'null' from o.environment when o's referent is a non-debuggee function. Is that not the case?

> What I'm mostly wondering about though is why
> environments from closures do not appear when inspecting a closure from the
> console without the debugger paused. This question is perhaps more
> appropriate for bug 810966, but I hope you will forgive this slight tangent.

The web console no longer adds any globals as debuggees. With the addition of D.p.makeGlobalObjectReference, there was no need. However, this patch makes it significant whether something is a debuggee or not.

Note that, when the JS debugger is active, the web console uses the JS debugger's Debugger instance --- which makes those environments inspectable.

In order to inspect environments, the web console will need to be patched to track debuggees, just as the content debugger does.

Comment 13

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

Note that js.msg in this patch needs a minor rebase.

(In reply to Jim Blandy :jimb from comment #12)
> (In reply to Panos Astithas [:past] from comment #8)
> > This patch fixes the crash for me, but I need to handle the exception now in
> > my tests as Jason notes.
> 
> Why is this? Unless you are fetching environments, removing debuggees, and
> then accessing the newly-non-debuggee environments, I *think* you shouldn't
> be getting any exceptions. You should just be getting 'null' from
> o.environment when o's referent is a non-debuggee function. Is that not the
> case?

Sorry, I must have been confused by the error logged in the frontend. I don't get an exception, just a null environment as expected.

> > What I'm mostly wondering about though is why
> > environments from closures do not appear when inspecting a closure from the
> > console without the debugger paused. This question is perhaps more
> > appropriate for bug 810966, but I hope you will forgive this slight tangent.
> 
> The web console no longer adds any globals as debuggees. With the addition
> of D.p.makeGlobalObjectReference, there was no need. However, this patch
> makes it significant whether something is a debuggee or not.
> 
> Note that, when the JS debugger is active, the web console uses the JS
> debugger's Debugger instance --- which makes those environments inspectable.
> 
> In order to inspect environments, the web console will need to be patched to
> track debuggees, just as the content debugger does.

OK, that's fine. This means I have to change the web console test to inspect closures when execution is paused on a breakpoint or something, but that should be pretty straightforward.

Comment 14

[Jim Blandy :jimb]

Attachment: Test forcing resumptions in non-debuggee frames.

As requested, tests for forcing resumptions in non-debuggee frames.

Comment 15

[Jim Blandy :jimb]

Attachment: Forbid access to non-debuggee environments.

Requested changes made. I've changed some stuff and added Debugger.Environment.prototype.inspectable, so I'm asking for a fresh review.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 812739 [details] [diff] [review]
Test forcing resumptions in non-debuggee frames.

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

Thanks!

::: js/src/jit-test/tests/debug/resumption-07.js
@@ +11,5 @@
> +  return function (frame) {
> +    log += 'd';
> +    dbg.removeDebuggee(g);
> +    return resumption;
> +  }

add optional semicolon on this line if you care about such things

Comment 17

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bbcea535e8f

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 812745 [details] [diff] [review]
Forbid access to non-debuggee environments.

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

r=me, looks great.

Comment 19

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1798c8f10d4c

Comment 20

[Simon Lindholm]

FWIW, Firebug relies on the old behavior, and it'll be a sad regression for us if the "Show closures" feature of the DOM panel and the command line .% syntax requires the Script panel to be enabled. (Obviously, enabling the debugger silently is a terrible idea.) How hard would it be to make this not assert instead? Alternatively, to make debuggers with '.enabled = false' have no perf penalty?

(We'd also need to come up with some messy work-around for when the JSD-based Script panel is enabled, keeping JSD2 debuggers alive only for single closure operations, because IIRC JSD and JSD2 do not interact well performance-wise. But addDebuggee is fast with JSD enabled, so I guess that's technically doable.)

Bug 740529 is a dup of this.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3bbcea535e8f

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1798c8f10d4c

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Simon Lindholm from comment #20)
> FWIW, Firebug relies on the old behavior, and it'll be a sad regression for
> us if the "Show closures" feature of the DOM panel and the command line .%
> syntax requires the Script panel to be enabled. (Obviously, enabling the
> debugger silently is a terrible idea.) How hard would it be to make this not
> assert instead? Alternatively, to make debuggers with '.enabled = false'
> have no perf penalty?
@Jim, what do you think about this? Is it doable?

Honza

Comment 24

[Jim Blandy :jimb]

First off - we've got a bug open to bring down Debugger's performance impact, which seems to be much worse than it has any reason to be. That's bug 923724.

Is it really unacceptable to create a Debugger instance just for inspecting the closure at hand, add the debuggee, grab the data, and then remove the debuggee and drop the Debugger on the floor? Sure, you'll flush out the jit code, but that's quick to regenerate anyway.

Comment 25

[Simon Lindholm]

> First off - we've got a bug open to bring down Debugger's performance impact, which seems to be much worse than it
> has any reason to be. That's bug 923724.
Nice! Do you think it will land in 27? Will it make it possible to rid of debug GCs?

> Is it really unacceptable to create a Debugger instance just for inspecting the closure at hand, add the debuggee,
> grab the data, and then remove the debuggee and drop the Debugger on the floor?
Right now, yes. The problem is that debug GCs are slow, and navigating the DOM panel would trigger them all the time.

Comment 26

[Jim Blandy :jimb]

This will definitely not make Firefox 27. When it does, yes, it will get rid of debug GCs.

The issue is that JavaScript code compiled for non-debug compartments doesn't include the inline code we need to keep Debugger's data structures up to date when we exit block scopes and function scopes. So it's not safe to create Debugger.Environment instances referring to non-debuggee code.

A Debugger with no hooks enabled costs about a factor of four in performance: ~2x for disabling IonMonkey, and another x1.75 for debugging.

Comment 27

[Simon Lindholm]

(In reply to Jim Blandy :jimb from comment #26)
> The issue is that JavaScript code compiled for non-debug compartments
> doesn't include the inline code we need to keep Debugger's data structures
> up to date when we exit block scopes and function scopes. So it's not safe
> to create Debugger.Environment instances referring to non-debuggee code.
In which way is it unsafe? Does it point to freed memory? Is there some subcase in which it is safe, say, for interpreted code? In practice it has seemed to work.

A factor 4 worse performance isn't really acceptable to us in case the debugger is disabled.

Comment 28

[Jim Blandy :jimb]

(In reply to Simon Lindholm from comment #27)
> In which way is it unsafe? Does it point to freed memory? Is there some
> subcase in which it is safe, say, for interpreted code? In practice it has
> seemed to work.
> 
> A factor 4 worse performance isn't really acceptable to us in case the
> debugger is disabled.

Yes, it'll end up with pointers to freed memory. Yes, it's safe in the interpreter. The Debugger has no perf impact on the interpreter; it's only x4 versus IonMonkey code (full speed).

Comment 29

[Jim Blandy :jimb]

Let me talk with Luke (who designed this part) about whether there's an easy way to sacrifice accuracy for performance here.

Comment 30

[Jim Blandy :jimb]

Talking things over with Luke, it may be possible to offer read-only Environments that don't respect the identity rules.

I'm a little confused about what we're comparing with what here. Doesn't the old JSD disable IonMonkey as well? How did things perform when Firebug used JSD to inspect environments?

Comment 31

[Simon Lindholm]

(In reply to Jim Blandy :jimb from comment #30)
> Talking things over with Luke, it may be possible to offer read-only
> Environments that don't respect the identity rules.
Sounds nice. I suspect interpreter code is the most interesting for us (accessing functions within "(function() { ... })();" module patterns, and other generally heavyweight stuff), so if getting (writable?) environments for those is easier than the general JIT case it might be sufficient.

> I'm a little confused about what we're comparing with what here. Doesn't the
> old JSD disable IonMonkey as well? How did things perform when Firebug used
> JSD to inspect environments?
This isn't for use within a debugger, so JSD is irrelevant. It's for a new feature we added recently (in 1.11, as soon as the JSD2 APIs appeared - we had no JSD equivalent) that allows inspecting closures when e.g. just playing around with objects in the console, without debugger. It would be an option for us to allow the feature only with enabled Script panel (JSD); it just wouldn't be very nice.

> Yes, it'll end up with pointers to freed memory.
Just out of curiosity, what happens to those pointers when we add a debugger and then try to look at the environment? I know JIT code is thrown away, but are environments fixed up in some way? (And is it possible to do that just for a single function/environment on the fly, assuming none of the code is on the stack?)

Comment 32

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

(In reply to Simon Lindholm from comment #31)
> This isn't for use within a debugger, so JSD is irrelevant. It's for a new
> feature we added recently (in 1.11, as soon as the JSD2 APIs appeared - we
> had no JSD equivalent) that allows inspecting closures when e.g. just
> playing around with objects in the console, without debugger. It would be an
> option for us to allow the feature only with enabled Script panel (JSD); it
> just wouldn't be very nice.

FWIW that's pretty much what the Firefox devtools currently do: if the debugger is active, then inspecting objects in the console will show any closures. Otherwise no closures will be displayed.

Comment 33

[Jim Blandy :jimb]

(In reply to Simon Lindholm from comment #31)
> (In reply to Jim Blandy :jimb from comment #30)
> > Talking things over with Luke, it may be possible to offer read-only
> > Environments that don't respect the identity rules.
> Sounds nice. I suspect interpreter code is the most interesting for us
> (accessing functions within "(function() { ... })();" module patterns, and
> other generally heavyweight stuff), so if getting (writable?) environments
> for those is easier than the general JIT case it might be sufficient.

The JIT handles those cases fine, actually. "Heavyweight" code is JITted without complaint.

> > I'm a little confused about what we're comparing with what here. Doesn't the
> > old JSD disable IonMonkey as well? How did things perform when Firebug used
> > JSD to inspect environments?
> This isn't for use within a debugger, so JSD is irrelevant. It's for a new
> feature we added recently (in 1.11, as soon as the JSD2 APIs appeared - we
> had no JSD equivalent) that allows inspecting closures when e.g. just
> playing around with objects in the console, without debugger. It would be an
> option for us to allow the feature only with enabled Script panel (JSD); it
> just wouldn't be very nice.

If you're unable to take the GC hit of switching Debugger on and off, and unable to take the perf hit of turning on Debugger, then this might be the best option. You're no worse off than you were before JSD2! :)

> > Yes, it'll end up with pointers to freed memory.
> Just out of curiosity, what happens to those pointers when we add a debugger
> and then try to look at the environment? I know JIT code is thrown away, but
> are environments fixed up in some way? (And is it possible to do that just
> for a single function/environment on the fly, assuming none of the code is
> on the stack?)

Once the code is not on the stack, then none of the dangling pointer issues can occur; the problem arises when an environment that Debugger has observed *leaves* the stack, but Debugger isn't informed of that. And we can't make a global a debuggee while it's got frames on the stack. So adding debuggees is inherently safe.

Comment 34

[Simon Lindholm]

(In reply to Jim Blandy :jimb from comment #33)
> You're no worse off than you were before JSD2! :)
Well, yes, but we're still worse off than Firefox 26.

> Once the code is not on the stack, then none of the dangling pointer issues
> can occur; the problem arises when an environment that Debugger has observed
> *leaves* the stack, but Debugger isn't informed of that.
Aha! That might explain why we haven't seen any problems with this - our usage happens with either no stack from the page compartment (DOM panel inspection case) or very minimal stack (.% syntax, where in 99% of cases the only frame is from gw.evalInGlobalWithBindings). It wouldn't be possible to only poison accesses to environments currently on the stack? Or back this out until bug 923724 et. al. land?

Comment 35

[Jim Blandy :jimb]

I'm sorry --- we can't put the crashing code back in just to preserve the performance of inspecting closures.

It seems to me that this is a lower priority than other projects we have on our plate at the moment.

Comment 36

[Simon Lindholm]

Understood. We'll add a Script panel dependency for 27+ and wait for bug 923724 then. Thanks for bearing with me with all the questions.

Comment 37

[Jim Blandy :jimb]

Certainly. Bug 923724 is making good progress; I'm optimistic.

Comment 38

[Shu-yu Guo [:shu]]

(In reply to Simon Lindholm from comment #36)
> Understood. We'll add a Script panel dependency for 27+ and wait for bug
> 923724 then. Thanks for bearing with me with all the questions.

Your use case of inspecting something and dropping the Debugger on the floor will probably take longer than bug 923724, which has more or less become bug 716647.

Bug 716647 *will not* flip on Ion for debuggee code. After bug 716647 lands, there are plans to work on a different Debugger mode (let's call it inspection mode) which will have a best effort approach to not perturb engine state. In inspection mode, you will be able to read whatever data we have from frames, but no values can be written and no hooks can be set. To write values and set hooks, you will have to explicitly invalidate Ion frames and throw away Ion code.