Bug 1100580 part 2. Get rid of EvaluateOptions::needResult, since we can use JS::CompileOptions::noScriptRval (with the opposite meaning, but same default behavior) for this purpose. r=bholley
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 18 Nov 2014 11:01:09 -0500
changeset 240618 e6fdb771bce0482f65b7058d35706fb6e8df4350
parent 240617 c46d86623e15265b09054a385211de53d37973b4
child 240619 52f500342c8835e97098fa4bb3e3483c69aec6eb
push id4311
push userraliiev@mozilla.com
push dateMon, 12 Jan 2015 19:37:41 +0000
treeherdermozilla-beta@150c9fed433b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1100580
milestone36.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1100580 part 2. Get rid of EvaluateOptions::needResult, since we can use JS::CompileOptions::noScriptRval (with the opposite meaning, but same default behavior) for this purpose. r=bholley
dom/base/nsJSUtils.cpp
dom/base/nsJSUtils.h
dom/base/nsScriptLoader.cpp
--- a/dom/base/nsJSUtils.cpp
+++ b/dom/base/nsJSUtils.cpp
@@ -187,30 +187,32 @@ nsJSUtils::EvaluateString(JSContext* aCx
                           JS::MutableHandle<JS::Value> aRetValue,
                           void **aOffThreadToken)
 {
   PROFILER_LABEL("nsJSUtils", "EvaluateString",
     js::ProfileEntry::Category::JS);
 
   MOZ_ASSERT_IF(aCompileOptions.versionSet,
                 aCompileOptions.version != JSVERSION_UNKNOWN);
-  MOZ_ASSERT_IF(aEvaluateOptions.coerceToString, aEvaluateOptions.needResult);
-  MOZ_ASSERT_IF(!aEvaluateOptions.reportUncaught, aEvaluateOptions.needResult);
+  MOZ_ASSERT_IF(aEvaluateOptions.coerceToString, !aCompileOptions.noScriptRval);
+  MOZ_ASSERT_IF(!aEvaluateOptions.reportUncaught, !aCompileOptions.noScriptRval);
+  // Note that the above assert means that if aCompileOptions.noScriptRval then
+  // also aEvaluateOptions.reportUncaught.
   MOZ_ASSERT(aCx == nsContentUtils::GetCurrentJSContext());
   MOZ_ASSERT(aSrcBuf.get());
   MOZ_ASSERT(js::GetGlobalForObjectCrossCompartment(aEvaluationGlobal) ==
              aEvaluationGlobal);
-  MOZ_ASSERT_IF(aOffThreadToken, !aEvaluateOptions.needResult);
+  MOZ_ASSERT_IF(aOffThreadToken, aCompileOptions.noScriptRval);
 
   // Unfortunately, the JS engine actually compiles scripts with a return value
   // in a different, less efficient way.  Furthermore, it can't JIT them in many
   // cases.  So we need to be explicitly told whether the caller cares about the
   // return value.  Callers can do this by calling the other overload of
-  // EvaluateString() which calls this function with aEvaluateOptions.needResult
-  // set to false.
+  // EvaluateString() which calls this function with
+  // aCompileOptions.noScriptRval set to true.
   aRetValue.setUndefined();
 
   nsresult rv = NS_OK;
 
   nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
   NS_ENSURE_TRUE(ssm->ScriptAllowed(aEvaluationGlobal), NS_OK);
 
   mozilla::Maybe<AutoDontReportUncaught> dontReport;
@@ -246,17 +248,17 @@ nsJSUtils::EvaluateString(JSContext* aCx
         script(aCx, JS::FinishOffThreadScript(aCx, JS_GetRuntime(aCx), *aOffThreadToken));
       *aOffThreadToken = nullptr; // Mark the token as having been finished.
       if (script) {
         ok = JS_ExecuteScript(aCx, scopeChain, script);
       } else {
         ok = false;
       }
     } else if (ok) {
-      if (aEvaluateOptions.needResult) {
+      if (!aCompileOptions.noScriptRval) {
         ok = JS::Evaluate(aCx, scopeChain, aCompileOptions, aSrcBuf, aRetValue);
       } else {
         ok = JS::Evaluate(aCx, scopeChain, aCompileOptions, aSrcBuf);
       }
     }
 
     if (ok && aEvaluateOptions.coerceToString && !aRetValue.isUndefined()) {
       JS::Rooted<JS::Value> value(aCx, aRetValue);
@@ -264,38 +266,35 @@ nsJSUtils::EvaluateString(JSContext* aCx
       ok = !!str;
       aRetValue.set(ok ? JS::StringValue(str) : JS::UndefinedValue());
     }
   }
 
   if (!ok) {
     if (aEvaluateOptions.reportUncaught) {
       ReportPendingException(aCx);
-      if (aEvaluateOptions.needResult) {
+      if (!aCompileOptions.noScriptRval) {
         aRetValue.setUndefined();
       }
     } else {
       rv = JS_IsExceptionPending(aCx) ? NS_ERROR_FAILURE
                                       : NS_ERROR_OUT_OF_MEMORY;
       JS::Rooted<JS::Value> exn(aCx);
       JS_GetPendingException(aCx, &exn);
-      if (aEvaluateOptions.needResult) {
-        aRetValue.set(exn);
-      }
+      MOZ_ASSERT(!aCompileOptions.noScriptRval); // we asserted this on entry
+      aRetValue.set(exn);
       JS_ClearPendingException(aCx);
     }
   }
 
   // Wrap the return value into whatever compartment aCx was in.
-  if (aEvaluateOptions.needResult) {
-    JS::Rooted<JS::Value> v(aCx, aRetValue);
-    if (!JS_WrapValue(aCx, &v)) {
+  if (!aCompileOptions.noScriptRval) {
+    if (!JS_WrapValue(aCx, aRetValue)) {
       return NS_ERROR_OUT_OF_MEMORY;
     }
-    aRetValue.set(v);
   }
   return rv;
 }
 
 nsresult
 nsJSUtils::EvaluateString(JSContext* aCx,
                           JS::SourceBufferHolder& aSrcBuf,
                           JS::Handle<JSObject*> aEvaluationGlobal,
@@ -309,31 +308,31 @@ nsJSUtils::EvaluateString(JSContext* aCx
 
 nsresult
 nsJSUtils::EvaluateString(JSContext* aCx,
                           const nsAString& aScript,
                           JS::Handle<JSObject*> aEvaluationGlobal,
                           JS::CompileOptions& aCompileOptions)
 {
   EvaluateOptions options(aCx);
-  options.setNeedResult(false);
+  aCompileOptions.setNoScriptRval(true);
   JS::RootedValue unused(aCx);
   return EvaluateString(aCx, aScript, aEvaluationGlobal, aCompileOptions,
                         options, &unused);
 }
 
 nsresult
 nsJSUtils::EvaluateString(JSContext* aCx,
                           JS::SourceBufferHolder& aSrcBuf,
                           JS::Handle<JSObject*> aEvaluationGlobal,
                           JS::CompileOptions& aCompileOptions,
                           void **aOffThreadToken)
 {
   EvaluateOptions options(aCx);
-  options.setNeedResult(false);
+  aCompileOptions.setNoScriptRval(true);
   JS::RootedValue unused(aCx);
   return EvaluateString(aCx, aSrcBuf, aEvaluationGlobal, aCompileOptions,
                         options, &unused, aOffThreadToken);
 }
 
 /* static */
 bool
 nsJSUtils::GetScopeChainForElement(JSContext* aCx,
--- a/dom/base/nsJSUtils.h
+++ b/dom/base/nsJSUtils.h
@@ -63,40 +63,33 @@ public:
                                   uint32_t aArgCount,
                                   const char** aArgArray,
                                   const nsAString& aBody,
                                   JSObject** aFunctionObject);
 
   struct MOZ_STACK_CLASS EvaluateOptions {
     bool coerceToString;
     bool reportUncaught;
-    bool needResult;
     JS::AutoObjectVector scopeChain;
 
     explicit EvaluateOptions(JSContext* cx)
       : coerceToString(false)
       , reportUncaught(true)
-      , needResult(true)
       , scopeChain(cx)
     {}
 
     EvaluateOptions& setCoerceToString(bool aCoerce) {
       coerceToString = aCoerce;
       return *this;
     }
 
     EvaluateOptions& setReportUncaught(bool aReport) {
       reportUncaught = aReport;
       return *this;
     }
-
-    EvaluateOptions& setNeedResult(bool aNeedResult) {
-      needResult = aNeedResult;
-      return *this;
-    }
   };
 
   // aEvaluationGlobal is the global to evaluate in.  The return value
   // will then be wrapped back into the compartment aCx is in when
   // this function is called.
   static nsresult EvaluateString(JSContext* aCx,
                                  const nsAString& aScript,
                                  JS::Handle<JSObject*> aEvaluationGlobal,
--- a/dom/base/nsScriptLoader.cpp
+++ b/dom/base/nsScriptLoader.cpp
@@ -1043,16 +1043,19 @@ nsScriptLoader::FillCompileOptionsForReq
   // It's very important to use aRequest->mURI, not the final URI of the channel
   // aRequest ended up getting script data from, as the script filename.
   nsContentUtils::GetWrapperSafeScriptFilename(mDocument, aRequest->mURI, aRequest->mURL);
 
   aOptions->setIntroductionType("scriptElement");
   aOptions->setFileAndLine(aRequest->mURL.get(), aRequest->mLineNo);
   aOptions->setVersion(JSVersion(aRequest->mJSVersion));
   aOptions->setCompileAndGo(JS_IsGlobalObject(aScopeChain));
+  // We only need the setNoScriptRval bit when compiling off-thread here, since
+  // otherwise nsJSUtils::EvaluateString will set it up for us.
+  aOptions->setNoScriptRval(true);
   if (aRequest->mHasSourceMapURL) {
     aOptions->setSourceMapURL(aRequest->mSourceMapURL.get());
   }
   if (aRequest->mOriginPrincipal) {
     nsIPrincipal* scriptPrin = nsContentUtils::ObjectPrincipal(aScopeChain);
     bool subsumes = scriptPrin->Subsumes(aRequest->mOriginPrincipal);
     aOptions->setMutedErrors(!subsumes);
   }