From cccdd9fbca3bf1c929dcdcc97940bf45e1dc3731 Mon Sep 17 00:00:00 2001 From: Nathan Froyd Date: Wed, 23 Mar 2016 10:40:53 -0400 Subject: [PATCH] Bug 1177488 - use |const char*| for representing async call reasons; r=bz,fitzgen Using a simple |const char*| is more memory-efficient than allocating a JS string. We still have to allocate the JS string for passing things into JS, but ideally we will be able to move the point of allocation much closer to where it's actually needed, rather than indiscriminantly doing it all the time. --- docshell/base/nsDocShell.cpp | 3 ++- docshell/base/nsIDocShell.idl | 2 +- .../base/timeline/JavascriptTimelineMarker.h | 18 +++++++++++++----- dom/base/ScriptSettings.cpp | 6 ++---- dom/base/ScriptSettings.h | 11 ++++++++--- dom/bindings/CallbackObject.cpp | 7 +------ dom/bindings/CallbackObject.h | 1 - dom/promise/Promise.cpp | 7 +------ js/public/Debug.h | 9 +++++++-- js/src/builtin/TestingFunctions.cpp | 7 ++++++- js/src/jsapi.cpp | 5 ++--- js/src/jsapi.h | 9 +++++++-- js/src/shell/js.cpp | 4 ++-- js/src/vm/Runtime.cpp | 2 +- js/src/vm/Runtime.h | 2 +- js/src/vm/SavedStacks.cpp | 19 ++++++++++++++++++- js/src/vm/Stack-inl.h | 2 +- js/src/vm/Stack.cpp | 4 ++-- js/src/vm/Stack.h | 4 ++-- js/xpconnect/src/XPCComponents.cpp | 7 ++----- 20 files changed, 79 insertions(+), 50 deletions(-) diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index eb5bc4f0c2a4..7403357d3669 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -14108,13 +14108,14 @@ nsDocShell::GetOpener() return opener; } +// The caller owns |aAsyncCause| here. void nsDocShell::NotifyJSRunToCompletionStart(const char* aReason, const char16_t* aFunctionName, const char16_t* aFilename, const uint32_t aLineNumber, JS::Handle aAsyncStack, - JS::Handle aAsyncCause) + const char* aAsyncCause) { // If first start, mark interval start. if (mJSRunToCompletionDepth == 0) { diff --git a/docshell/base/nsIDocShell.idl b/docshell/base/nsIDocShell.idl index 3a6aa7e30698..758693668d25 100644 --- a/docshell/base/nsIDocShell.idl +++ b/docshell/base/nsIDocShell.idl @@ -1059,7 +1059,7 @@ interface nsIDocShell : nsIDocShellTreeItem in wstring fileName, in unsigned long lineNumber, in jsval asyncStack, - in jsval asyncCause); + in string asyncCause); [noscript,notxpcom,nostdcall] void notifyJSRunToCompletionStop(); /** diff --git a/docshell/base/timeline/JavascriptTimelineMarker.h b/docshell/base/timeline/JavascriptTimelineMarker.h index 0214857d39bd..47c9943a5e7f 100644 --- a/docshell/base/timeline/JavascriptTimelineMarker.h +++ b/docshell/base/timeline/JavascriptTimelineMarker.h @@ -17,23 +17,25 @@ namespace mozilla { class JavascriptTimelineMarker : public TimelineMarker { public: + // The caller owns |aAsyncCause| here, so we must copy it into a separate + // string for use later on. JavascriptTimelineMarker(const char* aReason, const char16_t* aFunctionName, const char16_t* aFileName, uint32_t aLineNumber, MarkerTracingType aTracingType, JS::Handle aAsyncStack, - JS::Handle aAsyncCause) + const char* aAsyncCause) : TimelineMarker("Javascript", aTracingType, MarkerStackRequest::NO_STACK) , mCause(NS_ConvertUTF8toUTF16(aReason)) , mFunctionName(aFunctionName) , mFileName(aFileName) , mLineNumber(aLineNumber) + , mAsyncCause(aAsyncCause) { JSContext* ctx = nsContentUtils::GetCurrentJSContext(); if (ctx) { mAsyncStack.init(ctx, aAsyncStack); - mAsyncCause.init(ctx, aAsyncCause); } } @@ -50,10 +52,16 @@ public: stackFrame.mFunctionDisplayName.Construct(mFunctionName); if (mAsyncStack.isObject() && !mAsyncStack.isNullOrUndefined() && - mAsyncCause.isString()) { + !mAsyncCause.IsEmpty()) { JS::Rooted asyncStack(aCx, mAsyncStack.toObjectOrNull()); - JS::Rooted asyncCause(aCx, mAsyncCause.toString()); JS::Rooted parentFrame(aCx); + JS::Rooted asyncCause(aCx, JS_NewUCStringCopyN(aCx, mAsyncCause.BeginReading(), + mAsyncCause.Length())); + if (!asyncCause) { + JS_ClearPendingException(aCx); + return; + } + if (!JS::CopyAsyncStack(aCx, asyncStack, asyncCause, &parentFrame, 0)) { JS_ClearPendingException(aCx); } else { @@ -78,7 +86,7 @@ private: nsString mFileName; uint32_t mLineNumber; JS::PersistentRooted mAsyncStack; - JS::PersistentRooted mAsyncCause; + NS_ConvertUTF8toUTF16 mAsyncCause; }; } // namespace mozilla diff --git a/dom/base/ScriptSettings.cpp b/dom/base/ScriptSettings.cpp index 56bae426b218..bf5378230d7f 100644 --- a/dom/base/ScriptSettings.cpp +++ b/dom/base/ScriptSettings.cpp @@ -683,7 +683,7 @@ AutoEntryScript::DocshellEntryMonitor::DocshellEntryMonitor(JSContext* aCx, void AutoEntryScript::DocshellEntryMonitor::Entry(JSContext* aCx, JSFunction* aFunction, JSScript* aScript, JS::Handle aAsyncStack, - JS::Handle aAsyncCause) + const char* aAsyncCause) { JS::Rooted rootedFunction(aCx); if (aFunction) { @@ -728,13 +728,11 @@ AutoEntryScript::DocshellEntryMonitor::Entry(JSContext* aCx, JSFunction* aFuncti const char16_t* functionNameChars = functionName.isTwoByte() ? functionName.twoByteChars() : nullptr; - JS::Rooted asyncCauseValue(aCx, aAsyncCause ? StringValue(aAsyncCause) : - JS::NullValue()); docShellForJSRunToCompletion->NotifyJSRunToCompletionStart(mReason, functionNameChars, filename.BeginReading(), lineNumber, aAsyncStack, - asyncCauseValue); + aAsyncCause); } } diff --git a/dom/base/ScriptSettings.h b/dom/base/ScriptSettings.h index 95fddc31957c..abd9cf97cc5e 100644 --- a/dom/base/ScriptSettings.h +++ b/dom/base/ScriptSettings.h @@ -356,16 +356,21 @@ private: public: DocshellEntryMonitor(JSContext* aCx, const char* aReason); + // Please note that |aAsyncCause| here is owned by the caller, and its + // lifetime must outlive the lifetime of the DocshellEntryMonitor object. + // In practice, |aAsyncCause| is identical to |aReason| passed into + // the AutoEntryScript constructor, so the lifetime requirements are + // trivially satisfied by |aReason| being a statically allocated string. void Entry(JSContext* aCx, JSFunction* aFunction, JS::Handle aAsyncStack, - JS::Handle aAsyncCause) override + const char* aAsyncCause) override { Entry(aCx, aFunction, nullptr, aAsyncStack, aAsyncCause); } void Entry(JSContext* aCx, JSScript* aScript, JS::Handle aAsyncStack, - JS::Handle aAsyncCause) override + const char* aAsyncCause) override { Entry(aCx, nullptr, aScript, aAsyncStack, aAsyncCause); } @@ -375,7 +380,7 @@ private: private: void Entry(JSContext* aCx, JSFunction* aFunction, JSScript* aScript, JS::Handle aAsyncStack, - JS::Handle aAsyncCause); + const char* aAsyncCause); const char* mReason; }; diff --git a/dom/bindings/CallbackObject.cpp b/dom/bindings/CallbackObject.cpp index 6a6f568fd7a3..2eed19d4d450 100644 --- a/dom/bindings/CallbackObject.cpp +++ b/dom/bindings/CallbackObject.cpp @@ -172,12 +172,7 @@ CallbackObject::CallSetup::CallSetup(CallbackObject* aCallback, mAsyncStack.emplace(cx, aCallback->GetCreationStack()); if (*mAsyncStack) { - mAsyncCause.emplace(cx, JS_NewStringCopyZ(cx, aExecutionReason)); - if (*mAsyncCause) { - mAsyncStackSetter.emplace(cx, *mAsyncStack, *mAsyncCause); - } else { - JS_ClearPendingException(cx); - } + mAsyncStackSetter.emplace(cx, *mAsyncStack, aExecutionReason); } // Enter the compartment of our callback, so we can actually work with it. diff --git a/dom/bindings/CallbackObject.h b/dom/bindings/CallbackObject.h index d8ca634ebccc..4825b8ad0b69 100644 --- a/dom/bindings/CallbackObject.h +++ b/dom/bindings/CallbackObject.h @@ -257,7 +257,6 @@ protected: // Members which are used to set the async stack. Maybe> mAsyncStack; - Maybe> mAsyncCause; Maybe mAsyncStackSetter; // Can't construct a JSAutoCompartment without a JSContext either. Also, diff --git a/dom/promise/Promise.cpp b/dom/promise/Promise.cpp index b68c69b4a534..889e326a7d93 100644 --- a/dom/promise/Promise.cpp +++ b/dom/promise/Promise.cpp @@ -96,16 +96,11 @@ protected: } JS::Rooted asyncStack(cx, mPromise->mAllocationStack); - JS::Rooted asyncCause(cx, JS_NewStringCopyZ(cx, "Promise")); - if (!asyncCause) { - JS_ClearPendingException(cx); - return NS_ERROR_OUT_OF_MEMORY; - } { Maybe sas; if (asyncStack) { - sas.emplace(cx, asyncStack, asyncCause); + sas.emplace(cx, asyncStack, "Promise"); } mCallback->Call(cx, value); } diff --git a/js/public/Debug.h b/js/public/Debug.h index 9fc2c03f93d9..fe1115892afb 100644 --- a/js/public/Debug.h +++ b/js/public/Debug.h @@ -351,20 +351,25 @@ class MOZ_STACK_CLASS AutoEntryMonitor { // SpiderMonkey reports the JavaScript entry points occuring within this // AutoEntryMonitor's scope to the following member functions, which the // embedding is expected to override. + // + // It is important to note that |asyncCause| is owned by the caller and its + // lifetime must outlive the lifetime of the AutoEntryMonitor object. It is + // strongly encouraged that |asyncCause| be a string constant or similar + // statically allocated string. // We have begun executing |function|. Note that |function| may not be the // actual closure we are running, but only the canonical function object to // which the script refers. virtual void Entry(JSContext* cx, JSFunction* function, HandleValue asyncStack, - HandleString asyncCause) = 0; + const char* asyncCause) = 0; // Execution has begun at the entry point of |script|, which is not a // function body. (This is probably being executed by 'eval' or some // JSAPI equivalent.) virtual void Entry(JSContext* cx, JSScript* script, HandleValue asyncStack, - HandleString asyncCause) = 0; + const char* asyncCause) = 0; // Execution of the function or script has ended. virtual void Exit(JSContext* cx) { } diff --git a/js/src/builtin/TestingFunctions.cpp b/js/src/builtin/TestingFunctions.cpp index aa5ba3c31985..6ae1e0258f09 100644 --- a/js/src/builtin/TestingFunctions.cpp +++ b/js/src/builtin/TestingFunctions.cpp @@ -1122,8 +1122,13 @@ CallFunctionWithAsyncStack(JSContext* cx, unsigned argc, Value* vp) RootedObject function(cx, &args[0].toObject()); RootedObject stack(cx, &args[1].toObject()); RootedString asyncCause(cx, args[2].toString()); + JSAutoByteString utf8Cause; + if (!utf8Cause.encodeUtf8(cx, asyncCause)) { + MOZ_ASSERT(cx->isExceptionPending()); + return false; + } - JS::AutoSetAsyncStackForNewCalls sas(cx, stack, asyncCause, + JS::AutoSetAsyncStackForNewCalls sas(cx, stack, utf8Cause.ptr(), JS::AutoSetAsyncStackForNewCalls::AsyncCallKind::EXPLICIT); return Call(cx, UndefinedHandleValue, function, JS::HandleValueArray::empty(), args.rval()); diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index 210775d3bb59..62108175b11f 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -4865,11 +4865,11 @@ JS_RestoreFrameChain(JSContext* cx) } JS::AutoSetAsyncStackForNewCalls::AutoSetAsyncStackForNewCalls( - JSContext* cx, HandleObject stack, HandleString asyncCause, + JSContext* cx, HandleObject stack, const char* asyncCause, JS::AutoSetAsyncStackForNewCalls::AsyncCallKind kind) : cx(cx), oldAsyncStack(cx, cx->runtime()->asyncStackForNewActivations), - oldAsyncCause(cx, cx->runtime()->asyncCauseForNewActivations), + oldAsyncCause(cx->runtime()->asyncCauseForNewActivations), oldAsyncCallIsExplicit(cx->runtime()->asyncCallIsExplicit) { CHECK_REQUEST(cx); @@ -4881,7 +4881,6 @@ JS::AutoSetAsyncStackForNewCalls::AutoSetAsyncStackForNewCalls( return; SavedFrame* asyncStack = &stack->as(); - MOZ_ASSERT(!asyncCause->empty()); cx->runtime()->asyncStackForNewActivations = asyncStack; cx->runtime()->asyncCauseForNewActivations = asyncCause; diff --git a/js/src/jsapi.h b/js/src/jsapi.h index 015009367005..da6660357ee3 100644 --- a/js/src/jsapi.h +++ b/js/src/jsapi.h @@ -4479,7 +4479,7 @@ class MOZ_STACK_CLASS JS_PUBLIC_API(AutoSetAsyncStackForNewCalls) { JSContext* cx; RootedObject oldAsyncStack; - RootedString oldAsyncCause; + const char* oldAsyncCause; bool oldAsyncCallIsExplicit; public: @@ -4496,8 +4496,13 @@ class MOZ_STACK_CLASS JS_PUBLIC_API(AutoSetAsyncStackForNewCalls) // ambiguous whether that would clear any scheduled async stack and make the // normal stack reappear in the new call, or just keep the async stack // already scheduled for the new call, if any. + // + // asyncCause is owned by the caller and its lifetime must outlive the + // lifetime of the AutoSetAsyncStackForNewCalls object. It is strongly + // encouraged that asyncCause be a string constant or similar statically + // allocated string. AutoSetAsyncStackForNewCalls(JSContext* cx, HandleObject stack, - HandleString asyncCause, + const char* asyncCause, AsyncCallKind kind = AsyncCallKind::IMPLICIT); ~AutoSetAsyncStackForNewCalls(); }; diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp index c6eed033b028..efb17c70b85e 100644 --- a/js/src/shell/js.cpp +++ b/js/src/shell/js.cpp @@ -4959,7 +4959,7 @@ class ShellAutoEntryMonitor : JS::dbg::AutoEntryMonitor { } void Entry(JSContext* cx, JSFunction* function, JS::HandleValue asyncStack, - JS::HandleString asyncCause) override { + const char* asyncCause) override { MOZ_ASSERT(!enteredWithoutExit); enteredWithoutExit = true; @@ -4974,7 +4974,7 @@ class ShellAutoEntryMonitor : JS::dbg::AutoEntryMonitor { } void Entry(JSContext* cx, JSScript* script, JS::HandleValue asyncStack, - JS::HandleString asyncCause) override { + const char* asyncCause) override { MOZ_ASSERT(!enteredWithoutExit); enteredWithoutExit = true; diff --git a/js/src/vm/Runtime.cpp b/js/src/vm/Runtime.cpp index 09cb89e5008f..765d21daeee4 100644 --- a/js/src/vm/Runtime.cpp +++ b/js/src/vm/Runtime.cpp @@ -138,7 +138,7 @@ JSRuntime::JSRuntime(JSRuntime* parentRuntime) profilerSampleBufferLapCount_(1), wasmActivationStack_(nullptr), asyncStackForNewActivations(this), - asyncCauseForNewActivations(this), + asyncCauseForNewActivations(nullptr), asyncCallIsExplicit(false), entryMonitor(nullptr), noExecuteDebuggerTop(nullptr), diff --git a/js/src/vm/Runtime.h b/js/src/vm/Runtime.h index e11debba30a2..4dbb89e0ac39 100644 --- a/js/src/vm/Runtime.h +++ b/js/src/vm/Runtime.h @@ -708,7 +708,7 @@ struct JSRuntime : public JS::shadow::Runtime, /* * Value of asyncCause to be attached to asyncStackForNewActivations. */ - JS::PersistentRooted asyncCauseForNewActivations; + const char* asyncCauseForNewActivations; /* * True if the async call was explicitly requested, e.g. via diff --git a/js/src/vm/SavedStacks.cpp b/js/src/vm/SavedStacks.cpp index a5320fa91d21..0f3332fb57ef 100644 --- a/js/src/vm/SavedStacks.cpp +++ b/js/src/vm/SavedStacks.cpp @@ -25,6 +25,7 @@ #include "gc/Marking.h" #include "gc/Policy.h" #include "gc/Rooting.h" +#include "js/CharacterEncoding.h" #include "js/Vector.h" #include "vm/Debugger.h" #include "vm/SavedFrame.h" @@ -1164,7 +1165,23 @@ SavedStacks::insertFrames(JSContext* cx, FrameIter& iter, MutableHandleSavedFram // youngest frame of the async stack as the parent of the oldest // frame of this activation. We still need to iterate over other // frames in this activation before reaching the oldest frame. - asyncCause = activation.asyncCause(); + AutoCompartment ac(cx, iter.compartment()); + const char* cause = activation.asyncCause(); + UTF8Chars utf8Chars(cause, strlen(cause)); + size_t twoByteCharsLen = 0; + char16_t* twoByteChars = UTF8CharsToNewTwoByteCharsZ(cx, utf8Chars, + &twoByteCharsLen).get(); + if (!twoByteChars) + return false; + + // We expect that there will be a relatively small set of + // asyncCause reasons ("setTimeout", "promise", etc.), so we + // atomize the cause here in hopes of being able to benefit + // from reuse. + asyncCause = JS_AtomizeUCStringN(cx, twoByteChars, twoByteCharsLen); + js_free(twoByteChars); + if (!asyncCause) + return false; asyncActivation = &activation; } } diff --git a/js/src/vm/Stack-inl.h b/js/src/vm/Stack-inl.h index c688bc4f7a96..d32eff6f1ec0 100644 --- a/js/src/vm/Stack-inl.h +++ b/js/src/vm/Stack-inl.h @@ -873,7 +873,7 @@ Activation::Activation(JSContext* cx, Kind kind) hideScriptedCallerCount_(0), frameCache_(cx), asyncStack_(cx, cx->runtime_->asyncStackForNewActivations), - asyncCause_(cx, cx->runtime_->asyncCauseForNewActivations), + asyncCause_(cx->runtime_->asyncCauseForNewActivations), asyncCallIsExplicit_(cx->runtime_->asyncCallIsExplicit), kind_(kind) { diff --git a/js/src/vm/Stack.cpp b/js/src/vm/Stack.cpp index ba8baca0ba26..007c6adfa689 100644 --- a/js/src/vm/Stack.cpp +++ b/js/src/vm/Stack.cpp @@ -1401,7 +1401,7 @@ ActivationEntryMonitor::ActivationEntryMonitor(JSContext* cx, InterpreterFrame* // be traced if we trigger GC here. Suppress GC to avoid this. gc::AutoSuppressGC suppressGC(cx); RootedValue stack(cx, asyncStack(cx)); - RootedString asyncCause(cx, cx->runtime()->asyncCauseForNewActivations); + const char* asyncCause = cx->runtime()->asyncCauseForNewActivations; if (entryFrame->isFunctionFrame()) entryMonitor_->Entry(cx, &entryFrame->callee(), stack, asyncCause); else @@ -1417,7 +1417,7 @@ ActivationEntryMonitor::ActivationEntryMonitor(JSContext* cx, jit::CalleeToken e // a GC to discard the code we're about to enter, so we suppress GC. gc::AutoSuppressGC suppressGC(cx); RootedValue stack(cx, asyncStack(cx)); - RootedString asyncCause(cx, cx->runtime()->asyncCauseForNewActivations); + const char* asyncCause = cx->runtime()->asyncCauseForNewActivations; if (jit::CalleeTokenIsFunction(entryToken)) entryMonitor_->Entry(cx_, jit::CalleeTokenToFunction(entryToken), stack, asyncCause); else diff --git a/js/src/vm/Stack.h b/js/src/vm/Stack.h index 7d905946c3af..8cdf22a9168d 100644 --- a/js/src/vm/Stack.h +++ b/js/src/vm/Stack.h @@ -1237,7 +1237,7 @@ class Activation Rooted asyncStack_; // Value of asyncCause to be attached to asyncStack_. - RootedString asyncCause_; + const char* asyncCause_; // True if the async call was explicitly requested, e.g. via // callFunctionWithAsyncStack. @@ -1319,7 +1319,7 @@ class Activation return asyncStack_; } - JSString* asyncCause() { + const char* asyncCause() const { return asyncCause_; } diff --git a/js/xpconnect/src/XPCComponents.cpp b/js/xpconnect/src/XPCComponents.cpp index 54e862b674a0..7e2a02a9cc30 100644 --- a/js/xpconnect/src/XPCComponents.cpp +++ b/js/xpconnect/src/XPCComponents.cpp @@ -2712,12 +2712,9 @@ nsXPCComponents_Utils::CallFunctionWithAsyncStack(HandleValue function, } JS::Rooted asyncStackObj(cx, &asyncStack.toObject()); - JS::Rooted asyncCauseString(cx, JS_NewUCStringCopyN(cx, asyncCause.BeginReading(), - asyncCause.Length())); - if (!asyncCauseString) - return NS_ERROR_OUT_OF_MEMORY; - JS::AutoSetAsyncStackForNewCalls sas(cx, asyncStackObj, asyncCauseString, + NS_ConvertUTF16toUTF8 utf8Cause(asyncCause); + JS::AutoSetAsyncStackForNewCalls sas(cx, asyncStackObj, utf8Cause.get(), JS::AutoSetAsyncStackForNewCalls::AsyncCallKind::EXPLICIT); if (!JS_CallFunctionValue(cx, nullptr, function,