Some of the options handled in nsJSContext::JSOptionChangedCallback, in particular the JIT options, should be set per runtime, and not per context. In particular, they would be more properly handled in a similar callback on XPCJSRuntime. However, some of the option setters, take a JSContext as argument, even though they are per JSRuntime. In particular, this is true for: - JS_SetParallelParsingEnabled - JS_SetParallelIonCompilationEnabled - JS_SetGlobalJitCompilerOption - JS_SetGlobalJitCompilerOption These functions have to be refactored or extended to take a JSRuntime first.

A part of this bug is duplicate bug909997.

I've almost got it figured out, the only thing that isn't clear to me his how we should determine whether to use the chrome or the content prefs, or whether that distinction is even relevant anymore now that these options have moved to the runtime.

Comment on attachment 8335839 [details] [diff] [review] patch Review of attachment 8335839 [details] [diff] [review]: ----------------------------------------------------------------- Ugh, I'd totally spaced on that whole chrome/content thing. I think what we should do is to use the |content| values for the RuntimeOptions, and store the |chrome| values as members of XPCJSRuntime. Then, in xpc::CreateGlobalObject, we detect if the principal we're creating with is system principal, and use the chrome overrides if that's the case. It doesn't perfectly mirror the old semantics, but those old semantics were broken anyway. And hopefully this is good enough until we get to a point where we can just eliminate this chrome/content distinction. ::: dom/base/nsJSEnvironment.cpp @@ -784,5 @@ > JS::ContextOptionsRef(cx).setExtraWarnings(true); > } > #endif > > - JS::ContextOptionsRef(cx).setWerror(Preferences::GetBool(js_werror_option_str)); Hm, this goes away but never reappears...

(In reply to Bobby Holley (:bholley) from comment #3) > Comment on attachment 8335839 [details] [diff] [review] > patch > > Review of attachment 8335839 [details] [diff] [review]: > ----------------------------------------------------------------- > > Ugh, I'd totally spaced on that whole chrome/content thing. > > I think what we should do is to use the |content| values for the > RuntimeOptions, and store the |chrome| values as members of XPCJSRuntime. > Then, in xpc::CreateGlobalObject, we detect if the principal we're creating > with is system principal, and use the chrome overrides if that's the case. > It doesn't perfectly mirror the old semantics, but those old semantics were > broken anyway. And hopefully this is good enough until we get to a point > where we can just eliminate this chrome/content distinction. > Ok, that sounds reasonable. Remind me how I detect the principal again? > ::: dom/base/nsJSEnvironment.cpp > @@ -784,5 @@ > > JS::ContextOptionsRef(cx).setExtraWarnings(true); > > } > > #endif > > > > - JS::ContextOptionsRef(cx).setWerror(Preferences::GetBool(js_werror_option_str)); > > Hm, this goes away but never reappears... Probably an oversight. I'll fix it.

(In reply to Eddy Bruel [:ejpbruel] from comment #4) > Ok, that sounds reasonable. Remind me how I detect the principal again? It's explicitly passed to xpc::CreateGlobalObject. You can check it with XPCWrapper::GetSecurityManager()->IsSystemPrincipal().

New patch ready for feedback. I just need to figure out how to obtain a reference to the XPCJSRuntime from xpc::CreateGlobalObject before its ready for review. Please make sure I got the semantics right in the case we're running in safe mode. Previously, all JIT flags were reset false in that case, so I assume we should be doing the same for the chrome overrides now.

(In reply to Eddy Bruel [:ejpbruel] from comment #6) > New patch ready for feedback. I just need to figure out how to obtain a > reference to the XPCJSRuntime from xpc::CreateGlobalObject before its ready > for review. XPCJSRuntime::Get(). Bed time now - flag me for review with that change and I'll look at it in the morning?

Comment on attachment 8335859 [details] [diff] [review] patch Review of attachment 8335859 [details] [diff] [review]: ----------------------------------------------------------------- This is looking great! Thanks for doing it. ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +44,5 @@ > > #include "GeckoProfiler.h" > #include "nsJSPrincipals.h" > +#include "nsIXULRuntime.h" > +#include "nsXPCOMCIDInternal.h" I'd probably just do nsCOMPtr<nsIXULRuntime> runtime = do_GetService("@mozilla.org/xre/runtime;1"); Instead of pulling in this nasty-looking header. @@ +2910,5 @@ > +static const char js_parallel_parsing_str[] = JS_OPTIONS_DOT_STR "parallel_parsing"; > +static const char js_parallel_ion_compilation_str[] = JS_OPTIONS_DOT_STR "ion.parallel_compilation"; > +static const char js_baseline_eager_str[] = JS_OPTIONS_DOT_STR "baselinejit.unsafe_eager_compilation"; > +static const char js_ion_eager_str[] = JS_OPTIONS_DOT_STR "ion.unsafe_eager_compilation"; > +static const char js_hardening_str[] = JS_OPTIONS_DOT_STR "jit_hardening"; Can we just inline these strings directly into the function below? Having them declared as they are seems quite silly. @@ +2915,5 @@ > + > +int > +XPCJSRuntime::JSOptionChangedCallback(const char *pref, void *data) > +{ > + XPCJSRuntime *runtime = reinterpret_cast<XPCJSRuntime *>(data); Let's just use XPCJSRuntime::Get() instead of the scary reinterpret_cast. @@ +2951,5 @@ > + useBaselineEager = false; > + useIonEager = false; > + useHardening = false; > + } > + } How about ditching this, computing safeMode once at the top of the function, and doing things like: bool useBaseline = Preferences::GetBool(...) && !safeMode; @@ +2963,5 @@ > + ::JS_SetParallelIonCompilationEnabled(rt, parallelIonCompilation); > + ::JS_SetGlobalJitCompilerOption(rt, JSJITCOMPILER_BASELINE_USECOUNT_TRIGGER, > + useBaselineEager ? 0 : -1); > + ::JS_SetGlobalJitCompilerOption(rt, JSJITCOMPILER_ION_USECOUNT_TRIGGER, > + useIonEager ? 0 : -1); Ditch the :: prefixing while you're here, please. @@ +2997,5 @@ > mJunkScope(nullptr), > + mAsyncSnowWhiteFreer(new AsyncFreeSnowWhite()), > + mUseBaselineChrome(Preferences::GetBool(js_baseline_chrome_str)), > + mUseTypeInferenceChrome(Preferences::GetBool(js_type_inference_chrome_str)), > + mUseIonChrome(Preferences::GetBool(js_ion_chrome_str)) Let's just initialize these all to false, and then invoke the callback once manually. @@ +3166,5 @@ > + mUseBaselineChrome = false; > + mUseTypeInferenceChrome = false; > + mUseIonChrome = false; > + } > + } Let's just invoke the callback manually, per above, and dispense with this. @@ +3169,5 @@ > + } > + } > + > + // Watch for the JS boolean options > + Preferences::RegisterCallback(JSOptionChangedCallback, JS_OPTIONS_DOT_STR, this); Per the corresponding in the callback definition, let's pass nullptr for the closure. ::: js/xpconnect/src/nsXPConnect.cpp @@ +434,5 @@ > > + // Check whether we're chrome > + bool chrome; > + if (NS_FAILED(XPCWrapper::GetSecurityManager()->IsSystemPrincipal(principal, &chrome))) > + return nullptr; There's an infallible version of this function without the out-param. @@ +437,5 @@ > + if (NS_FAILED(XPCWrapper::GetSecurityManager()->IsSystemPrincipal(principal, &chrome))) > + return nullptr; > + > + if (chrome) { > + // TODO: How do I get a reference to the XPCJSRuntime here? Moreover, XPCJSRuntime::Get() @@ +439,5 @@ > + > + if (chrome) { > + // TODO: How do I get a reference to the XPCJSRuntime here? Moreover, > + // rather than making the chrome overrides on the runtime public > + // members, how about making this code a public member on XPCJSRuntime? We may eventually expose xpc::CreateGlobalObject outside of the engine, so I don't want to do that. You can just declare this as a friend function of XPCJSRuntime. ::: js/xpconnect/src/xpcprivate.h @@ +563,5 @@ > > class XPCJSRuntime : public mozilla::CycleCollectedJSRuntime > { > public: > + static int JSOptionChangedCallback(const char *pref, void *data); please declare this along with the other callbacks below (see OperationCallback), and call it |ReloadPrefsCallback|, or maybe even just |ReloadPrefs|. Up to you. @@ +789,5 @@ > friend class XPCIncrementalReleaseRunnable; > + > + bool mUseBaselineChrome; > + bool mUseTypeInferenceChrome; > + bool mUseIonChrome; I think a |ForChrome| suffix would be clearer here.

Comment on attachment 8336299 [details] [diff] [review] patch with feedback addressed Review of attachment 8336299 [details] [diff] [review]: ----------------------------------------------------------------- This is great stuff - r=bholley with comments addressed. ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +2913,5 @@ > + bool useTypeInference = Preferences::GetBool(JS_OPTIONS_DOT_STR "typeinference.content") && !safeMode; > + bool useIon = Preferences::GetBool(JS_OPTIONS_DOT_STR "ion.content") && !safeMode; > + bool useAsmJS = Preferences::GetBool(JS_OPTIONS_DOT_STR "asmjs") && !safeMode; > + > + runtime->mUseBaselineForChrome = Preferences::GetBool(JS_OPTIONS_DOT_STR "baselinejit.chrome") && !safeMode; Maybe add a note here that we treat content as the default because the eventual goal is to get rid of chrome-specific jit options. ::: js/xpconnect/src/nsXPConnect.cpp @@ +417,5 @@ > #endif > > namespace xpc { > > +inline void static inline void? @@ +418,5 @@ > > namespace xpc { > > +inline void > +SetCompartmentOverrides(JS::CompartmentOptions &options) This should be called SetChromeOverrides, I'd think. @@ +442,5 @@ > JSAutoCompartment ac(cx, global); > > + // Check whether we're chrome > + bool chrome; > + if (NS_FAILED(XPCWrapper::GetSecurityManager()->IsSystemPrincipal(principal, &chrome))) As noted in the previous review comments, you should switch to the non-outparam version here, defined in the IDL. if (XPCWrapper::GetSecurityManager()->IsSystemPrincipal(principal)) { SetChromeOverrides(..); } else { ... } ::: js/xpconnect/src/xpcprivate.h @@ +564,5 @@ > +namespace xpc { > + > +void SetCompartmentOverrides(JS::CompartmentOptions &options); > + > +} // namespace xpc This is kinda gross - see my comment below. @@ +731,5 @@ > > PRTime GetWatchdogTimestamp(WatchdogTimestampCategory aCategory); > void OnAfterProcessNextEvent() { mSlowScriptCheckpoint = mozilla::TimeStamp(); } > > + friend void xpc::SetCompartmentOverrides(JS::CompartmentOptions &options); What's the advantage of doing this rather than just putting it in CreateGlobalObject and making that the friend function?

I think we should probably merge this bug into bug 939562, given that neither is correct without the other.

Merging this bug into bug 939562 as requested by Bobby