Bug 157058 – Restrict the availability of some JSC options to local debug builds only.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Patch coming.

[Mark Lam]

<rdar://problem/25577473>

[Mark Lam]

Created attachment 277437 [details]
proposed patch.

[Filip Pizlo]

Comment on attachment 277437 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=277437&action=review

> Source/JavaScriptCore/runtime/Options.cpp:58
> +#if NDEBUG

I'm really not a fan of this policy.  A lot of bugs require a release build to repro.

[Brent Fulgham]

Could this be handled as a runtime flag?

[Mark Lam]

Comment on attachment 277437 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=277437&action=review

>> Source/JavaScriptCore/runtime/Options.cpp:58
>> +#if NDEBUG
> 
> I'm really not a fan of this policy.  A lot of bugs require a release build to repro.

I think you missed the part where allowRestrictedOptions() only applies to options with "Restricted" availability.  For now, the only options with "Restricted" availability are JSC_functionOverrides and JSC_useDollarVM.  If we need those in a release build, we can change this function and rebuild (which requires a rebuild of 1 file only).  All other options are "Normal" and are not gated by this allowRestrictedOptions() function.

[Mark Lam]

(In reply to comment #4)
> Could this be handled as a runtime flag?

This is not an option (pardon the pun).  All JSC options are runtime flags.  The whole point of this patch is that there are some "Restricted" options that we don't want to be runtime flags.  Hence, it doesn't make sense to use a runtime flag to disable those JSC runtime options / flags.

[Mark Lam]

Comment on attachment 277437 [details]
proposed patch.

I need to revise this patch to fix the broken debug build, and some non-ascii chars in the ChangeLog.

[Mark Lam]

Created attachment 277441 [details]
proposed patch with ChangeLog and debug build fix.

[Oliver Hunt]

Comment on attachment 277441 [details]
proposed patch with ChangeLog and debug build fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=277441&action=review

> Source/JavaScriptCore/runtime/Options.cpp:62
> +#ifdef NDEBUG
> +    return false;
> +#else
> +    return true;
> +#endif

Consider doing an internal test. Something like:
#if PLATFORM(OSX)
return !csr_check(CSR_ALLOW_APPLE_INTERNAL); // CSR check
#elif PLATFORM(IOS)
return MGGetBoolAnswer(kMGQAppleInternalInstallCapability); // Check with mobile gestalt
#endif

[Geoffrey Garen]

Comment on attachment 277441 [details]
proposed patch with ChangeLog and debug build fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=277441&action=review

r=me

>> Source/JavaScriptCore/runtime/Options.cpp:62
>> +#endif
> 
> Consider doing an internal test. Something like:
> #if PLATFORM(OSX)
> return !csr_check(CSR_ALLOW_APPLE_INTERNAL); // CSR check
> #elif PLATFORM(IOS)
> return MGGetBoolAnswer(kMGQAppleInternalInstallCapability); // Check with mobile gestalt
> #endif

I think the debug check is enough. We want non-Apple-internal engineers to be able to debug too.

[Geoffrey Garen]

> > Source/JavaScriptCore/runtime/Options.cpp:58
> > +#if NDEBUG
> 
> I'm really not a fan of this policy.  A lot of bugs require a release build
> to repro.

The plan for using these features in a release build is to edit this function and recompile. Since it's in a .cpp file, it's a single translation unit to recompile.

[Mark Lam]

Thanks for the review.  Landed in r200136: <http://trac.webkit.org/r200136>.