From 8d090830201b0380e2a5d3497b917fb9b6a34a61 Mon Sep 17 00:00:00 2001 From: Shu-yu Guo Date: Tue, 18 Nov 2014 16:42:02 -0800 Subject: [PATCH] Bug 1100320 - Don't call onExceptionUnwind and onPop debugger hooks on over-recursion. (r=jimb) --- .../tests/debug/onExceptionUnwind-12.js | 14 +++ js/src/jsapi.cpp | 5 + js/src/jsapi.h | 2 + js/src/jscntxt.cpp | 7 +- js/src/jscntxt.h | 7 ++ js/src/jscntxtinlines.h | 2 + js/src/vm/Debugger.cpp | 93 +++++++++++-------- 7 files changed, 88 insertions(+), 42 deletions(-) create mode 100644 js/src/jit-test/tests/debug/onExceptionUnwind-12.js diff --git a/js/src/jit-test/tests/debug/onExceptionUnwind-12.js b/js/src/jit-test/tests/debug/onExceptionUnwind-12.js new file mode 100644 index 000000000000..78874ddea676 --- /dev/null +++ b/js/src/jit-test/tests/debug/onExceptionUnwind-12.js @@ -0,0 +1,14 @@ +var g = newGlobal(); +g.parent = this; +g.hits = 0; +g.eval("new Debugger(parent).onExceptionUnwind = function () { hits++; };"); +function f() { + var x = f(); +} +try { + f(); +} catch (e) { + assertEq(e instanceof InternalError, true); +} finally { + assertEq(g.hits, 0); +} diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index dfda99022e21..f5ab88f7ec6c 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -6099,6 +6099,7 @@ JS_ReportPendingException(JSContext *cx) JS::AutoSaveExceptionState::AutoSaveExceptionState(JSContext *cx) : context(cx), wasPropagatingForcedReturn(cx->propagatingForcedReturn_), + wasOverRecursed(cx->overRecursed_), wasThrowing(cx->throwing), exceptionValue(cx) { @@ -6106,6 +6107,8 @@ JS::AutoSaveExceptionState::AutoSaveExceptionState(JSContext *cx) CHECK_REQUEST(cx); if (wasPropagatingForcedReturn) cx->clearPropagatingForcedReturn(); + if (wasOverRecursed) + cx->overRecursed_ = false; if (wasThrowing) { exceptionValue = cx->unwrappedException_; cx->clearPendingException(); @@ -6116,6 +6119,7 @@ void JS::AutoSaveExceptionState::restore() { context->propagatingForcedReturn_ = wasPropagatingForcedReturn; + context->overRecursed_ = wasOverRecursed; context->throwing = wasThrowing; context->unwrappedException_ = exceptionValue; drop(); @@ -6127,6 +6131,7 @@ JS::AutoSaveExceptionState::~AutoSaveExceptionState() if (wasPropagatingForcedReturn) context->setPropagatingForcedReturn(); if (wasThrowing) { + context->overRecursed_ = wasOverRecursed; context->throwing = true; context->unwrappedException_ = exceptionValue; } diff --git a/js/src/jsapi.h b/js/src/jsapi.h index 23bcf1456673..f6a1a164ec00 100644 --- a/js/src/jsapi.h +++ b/js/src/jsapi.h @@ -4989,6 +4989,7 @@ class JS_PUBLIC_API(AutoSaveExceptionState) private: JSContext *context; bool wasPropagatingForcedReturn; + bool wasOverRecursed; bool wasThrowing; RootedValue exceptionValue; @@ -5011,6 +5012,7 @@ class JS_PUBLIC_API(AutoSaveExceptionState) */ void drop() { wasPropagatingForcedReturn = false; + wasOverRecursed = false; wasThrowing = false; exceptionValue.setUndefined(); } diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp index 17fc393628f4..210585257f5e 100644 --- a/js/src/jscntxt.cpp +++ b/js/src/jscntxt.cpp @@ -416,8 +416,10 @@ js_ReportOverRecursed(JSContext *maybecx) */ fprintf(stderr, "js_ReportOverRecursed called\n"); #endif - if (maybecx) + if (maybecx) { JS_ReportErrorNumber(maybecx, js_GetErrorMessage, nullptr, JSMSG_OVER_RECURSED); + maybecx->overRecursed_ = true; + } } void @@ -1010,6 +1012,7 @@ JSContext::JSContext(JSRuntime *rt) throwing(false), unwrappedException_(UndefinedValue()), options_(), + overRecursed_(false), propagatingForcedReturn_(false), liveVolatileJitFrameIterators_(nullptr), reportGranularity(JS_DEFAULT_JITREPORT_GRANULARITY), @@ -1042,11 +1045,13 @@ JSContext::getPendingException(MutableHandleValue rval) rval.set(unwrappedException_); if (IsAtomsCompartment(compartment())) return true; + bool wasOverRecursed = overRecursed_; clearPendingException(); if (!compartment()->wrap(this, rval)) return false; assertSameCompartment(this, rval); setPendingException(rval); + overRecursed_ = wasOverRecursed; return true; } diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h index 704767f16dce..fea21ae007fa 100644 --- a/js/src/jscntxt.h +++ b/js/src/jscntxt.h @@ -423,6 +423,7 @@ struct JSContext : public js::ExclusiveContext, friend class js::ExclusiveContext; friend class JS::AutoSaveExceptionState; friend class js::jit::DebugModeOSRVolatileJitFrameIterator; + friend void js_ReportOverRecursed(JSContext *); private: /* Exception state -- the exception member is a GC root by definition. */ @@ -432,6 +433,10 @@ struct JSContext : public js::ExclusiveContext, /* Per-context options. */ JS::ContextOptions options_; + // True if the exception currently being thrown is by result of + // js_ReportOverRecursed. See Debugger::slowPathOnExceptionUnwind. + bool overRecursed_; + // True if propagating a forced return from an interrupt handler during // debug mode. bool propagatingForcedReturn_; @@ -571,9 +576,11 @@ struct JSContext : public js::ExclusiveContext, void clearPendingException() { throwing = false; + overRecursed_ = false; unwrappedException_.setUndefined(); } + bool isThrowingOverRecursed() const { return throwing && overRecursed_; } bool isPropagatingForcedReturn() const { return propagatingForcedReturn_; } void setPropagatingForcedReturn() { propagatingForcedReturn_ = true; } void clearPropagatingForcedReturn() { propagatingForcedReturn_ = false; } diff --git a/js/src/jscntxtinlines.h b/js/src/jscntxtinlines.h index 781dff7f333a..f7d9c39bbd04 100644 --- a/js/src/jscntxtinlines.h +++ b/js/src/jscntxtinlines.h @@ -367,6 +367,8 @@ inline void JSContext::setPendingException(js::Value v) { MOZ_ASSERT(!IsPoisonedValue(v)); + // overRecursed_ is set after the fact by js_ReportOverRecursed. + this->overRecursed_ = false; this->throwing = true; this->unwrappedException_ = v; // We don't use assertSameCompartment here to allow diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp index e263cac48b0c..a9e071b20b2a 100644 --- a/js/src/vm/Debugger.cpp +++ b/js/src/vm/Debugger.cpp @@ -592,51 +592,57 @@ Debugger::slowPathOnLeaveFrame(JSContext *cx, AbstractFramePtr frame, bool frame RootedValue value(cx); Debugger::resultToCompletion(cx, frameOk, frame.returnValue(), &status, &value); - /* Build a list of the recipients. */ - AutoObjectVector frames(cx); - for (FrameRange r(frame, global); !r.empty(); r.popFront()) { - if (!frames.append(r.frontFrame())) { - cx->clearPendingException(); - return false; - } - } - - /* For each Debugger.Frame, fire its onPop handler, if any. */ - for (JSObject **p = frames.begin(); p != frames.end(); p++) { - RootedNativeObject frameobj(cx, &(*p)->as()); - Debugger *dbg = Debugger::fromChildJSObject(frameobj); - - if (dbg->enabled && - !frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER).isUndefined()) { - RootedValue handler(cx, frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER)); - - Maybe ac; - ac.emplace(cx, dbg->object); - - RootedValue completion(cx); - if (!dbg->newCompletionValue(cx, status, value, &completion)) { - status = dbg->handleUncaughtException(ac, false); - break; + // This path can be hit via unwinding the stack due to + // over-recursion. Only fire the frames' onPop handlers if we haven't + // over-recursed, because invoking more JS will only result in more + // over-recursion errors. See slowPathOnExceptionUnwind. + if (!cx->isThrowingOverRecursed()) { + /* Build a list of the recipients. */ + AutoObjectVector frames(cx); + for (FrameRange r(frame, global); !r.empty(); r.popFront()) { + if (!frames.append(r.frontFrame())) { + cx->clearPendingException(); + return false; } + } - /* Call the onPop handler. */ - RootedValue rval(cx); - bool hookOk = Invoke(cx, ObjectValue(*frameobj), handler, 1, completion.address(), - &rval); - RootedValue nextValue(cx); - JSTrapStatus nextStatus = dbg->parseResumptionValue(ac, hookOk, rval, &nextValue); + /* For each Debugger.Frame, fire its onPop handler, if any. */ + for (JSObject **p = frames.begin(); p != frames.end(); p++) { + RootedNativeObject frameobj(cx, &(*p)->as()); + Debugger *dbg = Debugger::fromChildJSObject(frameobj); - /* - * At this point, we are back in the debuggee compartment, and any error has - * been wrapped up as a completion value. - */ - MOZ_ASSERT(cx->compartment() == global->compartment()); - MOZ_ASSERT(!cx->isExceptionPending()); + if (dbg->enabled && + !frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER).isUndefined()) { + RootedValue handler(cx, frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER)); - /* JSTRAP_CONTINUE means "make no change". */ - if (nextStatus != JSTRAP_CONTINUE) { - status = nextStatus; - value = nextValue; + Maybe ac; + ac.emplace(cx, dbg->object); + + RootedValue completion(cx); + if (!dbg->newCompletionValue(cx, status, value, &completion)) { + status = dbg->handleUncaughtException(ac, false); + break; + } + + /* Call the onPop handler. */ + RootedValue rval(cx); + bool hookOk = Invoke(cx, ObjectValue(*frameobj), handler, 1, completion.address(), + &rval); + RootedValue nextValue(cx); + JSTrapStatus nextStatus = dbg->parseResumptionValue(ac, hookOk, rval, &nextValue); + + /* + * At this point, we are back in the debuggee compartment, and any error has + * been wrapped up as a completion value. + */ + MOZ_ASSERT(cx->compartment() == global->compartment()); + MOZ_ASSERT(!cx->isExceptionPending()); + + /* JSTRAP_CONTINUE means "make no change". */ + if (nextStatus != JSTRAP_CONTINUE) { + status = nextStatus; + value = nextValue; + } } } } @@ -715,6 +721,11 @@ Debugger::slowPathOnDebuggerStatement(JSContext *cx, AbstractFramePtr frame) /* static */ JSTrapStatus Debugger::slowPathOnExceptionUnwind(JSContext *cx, AbstractFramePtr frame) { + // Invoking more JS on an over-recursed stack is only going to result in + // more over-recursion errors. + if (cx->isThrowingOverRecursed()) + return JSTRAP_CONTINUE; + RootedValue rval(cx); JSTrapStatus status = dispatchHook(cx, &rval, OnExceptionUnwind, NullPtr());