758617 - Crash in js::StackIter::settleOnNewState()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

STR:

1) Visit http://htmlpad.org/debugger/
2) Open the debugger
3) Click on the 'Click me!' button
4) Edit the value of variable 'a' in the variable pane to asdf (no quotes!)
5) Boom.

Full log:
http://past.pastebin.mozilla.org/1649938

Top of the stack:

js::StackIter::settleOnNewState() + 205
js::StackIter::operator++() + 237
_ZL14InitExnPrivateP9JSContextN2JS6HandleIP8JSObjectEENS2_IP8JSStringEES8_jP13JSErrorReporti + 1265
js_ErrorToException(JSContext*, char const*, JSErrorReport*, JSErrorFormatString const* (*)(void*, char const*, unsigned int), void*) + 913
_ZL11ReportErrorP9JSContextPKcP13JSErrorReportPFPK19JSErrorFormatStringPvS2_jES8_ + 109
js_ReportErrorNumberVA(JSContext*, unsigned int, JSErrorFormatString const* (*)(void*, char const*, unsigned int), void*, unsigned int, int, __va_list_tag*) + 265
+ 200
js::NameOperation(JSContext*, unsigned char*, JS::Value*) + 1170
js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) + 4014
js::RunScript(JSContext*, JSScript*, js::StackFrame*) + 957
js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) + 1298
js::EvaluateInEnv(JSContext*, JS::Handle<JSObject*>, js::StackFrame*, unsigned short const*, unsigned int, char const*, unsigned int, JS::Value*) + 675
_ZL17DebuggerFrameEvalP9JSContextjPN2JS5ValueE16EvalBindingsMode + 1030
js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) + 907

Comment 1

[Luke Wagner [:luke]]

Ah, this seems to be fallout from allowing eval-in-frame to eval in a frame in a different context.  I'll try to whip up a fix.

Comment 2

[Luke Wagner [:luke]]

Attachment: fix and test

Delightfully simple fix.  I audited the rest of the uses of prevInContext and they seem to be fine.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 628115 [details] [diff] [review]
fix and test

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

r=me.

StackIter's comment should point out that it's really just following the prev() chain, and in corner cases like eval-in-frame that can get weird. I think StackIter::settleOnNewState() should have a comment somewhere that tells what it's for. (In general I think API comments, documenting contracts, are very often worth it. We don't do enough of those.)

::: js/src/jit-test/tests/debug/cross-context-3.js
@@ +4,5 @@
> +var dbg = new Debugger(g);
> +var hits = 0;
> +dbg.onDebuggerStatement = function (frame1) {
> +    dbg.onDebuggerStatement = function (frame2) {
> +        print(frame1.eval("a").throw);

Assert something instead of printing it?

Comment 4

[Luke Wagner [:luke]]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> (In general I think API comments, documenting
> contracts, are very often worth it. We don't do enough of those.)

Madness

Comment 5

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02e49bc6ead9

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/02e49bc6ead9