1007164 - Assertion failure: v.isBoolean(), at js/src/vm/Interpreter.cpp:836

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: Crash dump

Slightly modified STR from bug 1006922:

1. Go to http://todomvc.com/architecture-examples/backbone/
2. Add a breakpoint on line 221 of backbone.js (the line should read " var listeningTo = ...")
3. Reload the page
4. Wait for the breakpoint to hit
5. Switch to the web console
6. Type "inspect(method)" (without quotes)
7. Press enter -> crash.

The value that I'm trying to inspect has been optimized-out by the engine. Top of the stack:

0  js::TypeOfValue(JS::Value const&) + 272 (Interpreter.cpp:836)
1  js::TypeOfOperation(JS::Value const&, JSRuntime*) + 25 (Interpreter-inl.h:453)
2  Interpret(JSContext*, js::RunState&) + 44384 (Interpreter.cpp:2394)
3  js::RunScript(JSContext*, js::RunState&) + 505 (Interpreter.cpp:422)
4  js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) + 1491 (Interpreter.cpp:494)
5  js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) + 834 (Interpreter.cpp:531)
6  js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) + 316 (jsproxy.cpp:448)
7  js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) + 572 (jswrapper.cpp:464)
8  js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) + 385 (jsproxy.cpp:2659)
9  js::proxy_Call(JSContext*, unsigned int, JS::Value*) + 248 (jsproxy.cpp:3062)
10 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 165 (jscntxtinlines.h:239)
11 js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) + 978 (Interpreter.cpp:468)
12 Interpret(JSContext*, js::RunState&) + 53803 (Interpreter.cpp:2620)
13 js::RunScript(JSContext*, js::RunState&) + 505 (Interpreter.cpp:422)

Comment 1

[Jim Blandy :jimb]

So, this is basically my fault: Shu relied on me for design guidance when writing the optimized-out value support, and wrote things up as I suggested; but the approach we took was just wrong. We need to rejigger the API a bit.

Fortunately, I think it's not too bad.

The central problem is that DebugScopeProxy reports optimized-out values by returning magic values from the proxy's get and getOwnPropertyDescriptor hooks; but since Debugger.Frame.prototype.eval (which is what the web console evaluation in comment 0 uses) uses a DebugScopeObject, which are DebugScopeProxy-driven proxies, as the environment for the evaluation, those magic values are immediately visible to the evaluated JavaScript code.

Because DebugScopeObjects were created to serve exactly this role, it's completely inappropriate for DebugScopeProxy's hooks to return magic values, as they do in the design Shu and I put together. That needs to change.

First, we need to decide what *should* happen when code passed to D.F.p.eval tries to access a variable whose value we cannot obtain. Shu and I discussed this, and we think the most developer-friendly behavior is for the eval to throw an exception. All developer tools must already be prepared for eval to throw, since even evaluating the simplest expressions can cause almost anything to run, if there are 'with' clauses with hostile objects, or weird getters / setters on 'window', etc. The developer needs to know that the tool was unable to evaluate the expression as expected, so returning 'undefined' or the like would be unhelpful.

Given that, it seems that DebugScopeProxy::handleUnaliasedAccess should continue to return the JS_OPTIMIZED_OUT magic value, and that its callers, getOwnPropertyDescriptor and get in the same class, should check for JS_OPTIMIZED_OUT and throw an exception. DebugScopeProxy::checkForMissingArguments provides some precedent here.

The next question is, how can Debugger.Environment.prototype.getVariable continue to provide optimizedOut sentinel values? I think we should add a new method to detect such variables to js::DebugScopeObject, whose implementation in ScopeObject.cpp can defer to an appropriate method of DebugScopeProxy that calls handleUnaliasedAccess directly, or something similar. Then, DebuggerEnv_getVariable and related calls can use that DebugScopeObject method, when the Env in question is actually a DebugScopeObject.

We have the opportunity here to clean up something else as well: many callers of D.E.p.getVariable in the devtools wrap the call in a 'try' clause, to handle errors thrown by DebugScopeProxy::checkForMissingArguments. It's always a shame to use exceptions to report situations that are rare, but must be handled by correct code anyway: you essentially force all your callers to use try blocks, which are a pain. So I would suggest changing D.E.p.getVariable to return a new sentinel value for the condition checkForMissingArguments is looking for, rather than throwing. Then, the callers in the devtools code can drop their 'try' clauses and simply check for sentinel values.

If we decide to take this route, we should not forget to file a blocking bug (not a follow-up!) to prepare the devtools for the sentinel values they'll be seeing. A follow-up bug could remove the 'try' blocks.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Throw on touching sentinel values in DebugScopeProxy by default but allow Debugger.Environment.prototype.getVariable access.

Panos, the f? is for you to test if the patch works for you, not to look over the code.

When you do inspect(method) now, it should print an error like "variable has
been optimized out". Let me know if there are still other problems with this
patch + the ones you're working on.

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8419103 [details] [diff] [review]
Throw on touching sentinel values in DebugScopeProxy by default but allow Debugger.Environment.prototype.getVariable access.

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

::: toolkit/devtools/webconsole/utils.js
@@ +1089,5 @@
>      // TODO: we should use getVariableDescriptor() here - bug 725815.
> +    let result = aObj.getVariable(aName);
> +    // FIXME: Need actual UI, bug 941287.
> +    if (result.optimizedOut || result.missingArguments)
> +      return null;

Oops this should have { } around it.

Comment 4

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

Comment on attachment 8419103 [details] [diff] [review]
Throw on touching sentinel values in DebugScopeProxy by default but allow Debugger.Environment.prototype.getVariable access.

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

Everything works great with this patch, which fixes bug 1004562 as well.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8419103 [details] [diff] [review]
Throw on touching sentinel values in DebugScopeProxy by default but allow Debugger.Environment.prototype.getVariable access.

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

Thanks for fixing this so quickly!

There's an overall convention in SpiderMonkey that I think is valuable to preserve, and which your patches often don't follow:

When checking a function call for an error return, the function call should be the condition of an 'if' whose 'then' statement contains an early 'return false/nullptr'.

Following this cliche makes code faster to read for people familiar with the SpiderMonkey code base.

::: js/src/vm/ScopeObject.cpp
@@ +1152,4 @@
>       */
> +    HandleAccessStatus handleUnaliasedAccess(JSContext *cx, Handle<DebugScopeObject*> debugScope,
> +                                             Handle<ScopeObject*> scope, jsid id, Action action,
> +                                             MutableHandleValue vp)

It's pretty well-established that we use bool for error return, and report other sorts of details via variables passed by reference. Following these conventions is very helpful for readability.

Please make this function return a bool success value, and return further details about non-erroneous completions via a variable passed by reference.

@@ -1299,2 @@
>              return false;
> -        }

This looks wrong --- checkForMissingArguments can return false on OOM, can't it? This change makes a false return value ambiguous: it's either OOM or no live frame.

@@ +1351,2 @@
>              return false;
> +        }

Once checkForMissingArguments behaves properly, this will need to change correspondingly.

@@ +1628,5 @@
>  
>  bool
> +DebugScopeObject::getMaybeSentinelValue(JSContext *cx, HandleId id, MutableHandleValue vp)
> +{
> +    MOZ_ASSERT(handler() == &DebugScopeProxy::singleton);

Do we need to assert this? The caller should have used as<DebugScopeObject> to get the properly typed pointer in the first place.

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #6)
> Comment on attachment 8419103 [details] [diff] [review]
> Throw on touching sentinel values in DebugScopeProxy by default but allow
> Debugger.Environment.prototype.getVariable access.
> 
> Review of attachment 8419103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for fixing this so quickly!
> 
> There's an overall convention in SpiderMonkey that I think is valuable to
> preserve, and which your patches often don't follow:
> 
> When checking a function call for an error return, the function call should
> be the condition of an 'if' whose 'then' statement contains an early 'return
> false/nullptr'.
> 
> Following this cliche makes code faster to read for people familiar with the
> SpiderMonkey code base.

I strongly disagree with this point. I think the existing style is bad for correctness. Threading through bool *s which the user has to remember to check is more error prone than the compiler telling you that you missed a case in switching on an enum. We also have precedent for "algebraic" return values, which I've listed below. It might be confined to the JIT, but I believe it to be a better style.

http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Ion.h?from=MethodStatus&case=true#26
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Ion.h?from=IonExecStatus#100
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.h?from=InliningDecision#636
http://dxr.mozilla.org/mozilla-central/source/js/src/jsobj.h#761

> 
> ::: js/src/vm/ScopeObject.cpp
> @@ +1152,4 @@
> >       */
> > +    HandleAccessStatus handleUnaliasedAccess(JSContext *cx, Handle<DebugScopeObject*> debugScope,
> > +                                             Handle<ScopeObject*> scope, jsid id, Action action,
> > +                                             MutableHandleValue vp)
> 
> It's pretty well-established that we use bool for error return, and report
> other sorts of details via variables passed by reference. Following these
> conventions is very helpful for readability.
> 
> Please make this function return a bool success value, and return further
> details about non-erroneous completions via a variable passed by reference.
> 
> @@ -1299,2 @@
> >              return false;
> > -        }
> 
> This looks wrong --- checkForMissingArguments can return false on OOM, can't
> it? This change makes a false return value ambiguous: it's either OOM or no
> live frame.
> 

This doesn't change the current behavior, but it seems like current behavior is broken wrt OOMs. Right now the function returns true even if ArgumentsObject::createUnexpected OOMs. I'll fix that.

> @@ +1351,2 @@
> >              return false;
> > +        }
> 
> Once checkForMissingArguments behaves properly, this will need to change
> correspondingly.
> 
> @@ +1628,5 @@
> >  
> >  bool
> > +DebugScopeObject::getMaybeSentinelValue(JSContext *cx, HandleId id, MutableHandleValue vp)
> > +{
> > +    MOZ_ASSERT(handler() == &DebugScopeProxy::singleton);
> 
> Do we need to assert this? The caller should have used as<DebugScopeObject>
> to get the properly typed pointer in the first place.

You're right, we don't need to assert this.

Comment 8

[Shu-yu Guo [:shu]]

Attachment: Throw on touching sentinel values in DebugScopeProxy by default but allow Debugger.Environment.prototype.getVariable access.

Addressed review comments except first one.

Comment 9

[Shu-yu Guo [:shu]]

Attachment: Throw on touching sentinel values in DebugScopeProxy by default but allow Debugger.Environment.prototype.getVariable access.

v3

After speaking with jorendorff and luke, I have come around on the enum issue.
C++'s stupid enum-to-integral type auto coercion makes using enum return types
no easier to statically detect incorrect error handling, and the outparams can
certainly be enums and not bools. Jim, I now agree with your argument about
consistency and have refactored the patch for these methods to return bool for
success/error.

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8419797 [details] [diff] [review]
Throw on touching sentinel values in DebugScopeProxy by default but allow Debugger.Environment.prototype.getVariable access.

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

::: js/src/vm/ScopeObject.cpp
@@ +1467,3 @@
>      }
>  
> +

A stray newline found its way in, ignore this.

Comment 11

[Jim Blandy :jimb]

Comment on attachment 8419797 [details] [diff] [review]
Throw on touching sentinel values in DebugScopeProxy by default but allow Debugger.Environment.prototype.getVariable access.

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

Excellent. Thank you very much!

::: js/src/vm/ScopeObject.cpp
@@ +1306,4 @@
>  
> +    /*
> +     * Create a missing arguments object. If the function returns true but
> +     * *maybeArgsObj is null, it means the scope is dead.

I think this means 'argsObj', not '*maybeArgsObj'.

@@ +1363,5 @@
> +            if (!argsObj) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_DEBUG_NOT_LIVE,
> +                                     "Debugger scope");
> +                return false;
> +            }

This is much nicer than before. Thanks.

@@ +1389,5 @@
>              return true;
> +          case ACCESS_GENERIC:
> +            return JS_GetOwnPropertyDescriptorById(cx, scope, id, desc);
> +          default:
> +            MOZ_ASSERT(access == ACCESS_LOST);

Ignore if you prefer, but isn't it nicely symmetrical to use 'case ACCESS_LOST:' here, and then default: MOZ_ASSUME_UNREACHABLE?

Comment 12

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #11)
> Comment on attachment 8419797 [details] [diff] [review]
> Throw on touching sentinel values in DebugScopeProxy by default but allow
> Debugger.Environment.prototype.getVariable access.
> 
> Review of attachment 8419797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent. Thank you very much!
> 
> ::: js/src/vm/ScopeObject.cpp
> @@ +1306,4 @@
> >  
> > +    /*
> > +     * Create a missing arguments object. If the function returns true but
> > +     * *maybeArgsObj is null, it means the scope is dead.
> 
> I think this means 'argsObj', not '*maybeArgsObj'.
> 
> @@ +1363,5 @@
> > +            if (!argsObj) {
> > +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_DEBUG_NOT_LIVE,
> > +                                     "Debugger scope");
> > +                return false;
> > +            }
> 
> This is much nicer than before. Thanks.
> 
> @@ +1389,5 @@
> >              return true;
> > +          case ACCESS_GENERIC:
> > +            return JS_GetOwnPropertyDescriptorById(cx, scope, id, desc);
> > +          default:
> > +            MOZ_ASSERT(access == ACCESS_LOST);
> 
> Ignore if you prefer, but isn't it nicely symmetrical to use 'case
> ACCESS_LOST:' here, and then default: MOZ_ASSUME_UNREACHABLE?

In general, yes, but I seem to remember on some compilers it still gives you pesky warnings because the compiler can't figure out that all paths return a value or something? If it doesn't warn anymore I'll make the change.

Comment 13

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1233366d80f3

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8419797 [details] [diff] [review]
Throw on touching sentinel values in DebugScopeProxy by default but allow Debugger.Environment.prototype.getVariable access.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 716647
User impact if declined: Debugger doesn't work right (eval-in-env broken)
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1233366d80f3

Comment 16

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

I'm adding a browser mochitest for the web console in bug 1004562.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Nevermind, I just got bit mixed up on the landing order with this and bug 1002456.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fcbb78e6392e