always expose safe functions from HermesInternal

Summary:
This diff changes the behavior of enableHermesInternal=false from hiding all
functions to hiding only the unsafe functions.

`HermesInternal.concat` is an example of safe function that template literal
relied on and exposing it fix the bug that using template literal throw
TypeError under runtime config `enableHermesInternal=false`.

Reviewed By: tmikov

Differential Revision: D23638791

fbshipit-source-id: 10d75dee1eb499d389024fba8ac76e6c6ff9a55e

lib/VM/JSLib/HermesInternal.cpp

@@ -676,17 +676,44 @@ Handle<JSObject> createHermesInternalObject(
     Runtime *runtime,
     const JSLibFlags &flags) {
   Handle<JSObject> intern = runtime->makeHandle(JSObject::create(runtime));
-  if (!flags.enableHermesInternal) {
-    JSObject::preventExtensions(*intern);
-    return intern;
-  }
 
   DefinePropertyFlags constantDPF =
       DefinePropertyFlags::getDefaultNewPropertyFlags();
   constantDPF.enumerable = 0;
   constantDPF.writable = 0;
   constantDPF.configurable = 0;
 
+  // Make a copy of the original String.prototype.concat implementation that we
+  // can use internally.
+  // TODO: we can't make HermesInternal.concat a static builtin method now
+  // because this method should be called with a meaningful `this`, but
+  // CallBuiltin instruction does not support it.
+  auto propRes = JSObject::getNamed_RJS(
+      runtime->makeHandle<JSObject>(runtime->stringPrototype),
+      runtime,
+      Predefined::getSymbolID(Predefined::concat));
+  assert(
+      propRes != ExecutionStatus::EXCEPTION && !(*propRes)->isUndefined() &&
+      "Failed to get String.prototype.concat.");
+  auto putRes = JSObject::defineOwnProperty(
+      intern,
+      runtime,
+      Predefined::getSymbolID(Predefined::concat),
+      constantDPF,
+      runtime->makeHandle(std::move(*propRes)));
+  assert(
+      putRes != ExecutionStatus::EXCEPTION && *putRes &&
+      "Failed to set HermesInternal.concat.");
+  (void)putRes;
+
+  // If `enableHermesInternal=false`, hide functions that aren't considered
+  // safe. All functions that are known to be safe should be defined above this
+  // check.
+  if (!flags.enableHermesInternal) {
+    JSObject::preventExtensions(*intern);
+    return intern;
+  }
+
   auto defineInternMethod =
       [&](Predefined::Str symID, NativeFunctionPtr func, uint8_t count = 0) {
         (void)defineMethod(
@@ -740,29 +767,6 @@ Handle<JSObject> createHermesInternalObject(
   defineInternMethodAndSymbol("getCallStack", hermesInternalGetCallStack, 0);
 #endif // HERMESVM_EXCEPTION_ON_OOM
 
-  // Make a copy of the original String.prototype.concat implementation that we
-  // can use internally.
-  // TODO: we can't make HermesInternal.concat a static builtin method now
-  // because this method should be called with a meaningful `this`, but
-  // CallBuiltin instruction does not support it.
-  auto propRes = JSObject::getNamed_RJS(
-      runtime->makeHandle<JSObject>(runtime->stringPrototype),
-      runtime,
-      Predefined::getSymbolID(Predefined::concat));
-  assert(
-      propRes != ExecutionStatus::EXCEPTION && !(*propRes)->isUndefined() &&
-      "Failed to get String.prototype.concat.");
-  auto putRes = JSObject::defineOwnProperty(
-      intern,
-      runtime,
-      Predefined::getSymbolID(Predefined::concat),
-      constantDPF,
-      runtime->makeHandle(std::move(*propRes)));
-  assert(
-      putRes != ExecutionStatus::EXCEPTION && *putRes &&
-      "Failed to set HermesInternal.concat.");
-  (void)putRes;
-
   JSObject::preventExtensions(*intern);
 
   return intern;

test/hermes/internal-test-methods.js

@@ -10,8 +10,11 @@
 // RUN: %hermes -Xhermes-internal-test-methods=true %s | %FileCheck --match-full-lines --check-prefix=CHKIME %s
 // RUN: %hermes -Xhermes-internal-test-methods=false %s | %FileCheck --match-full-lines --check-prefix=CHKIMD %s
 
-// Check that we can disable all fields of HermesInternal.
-print(Object.getOwnPropertyNames(HermesInternal).length !== 0);
+// HermesInternal.concat
+var SAFE_FIELDS_COUNT = 1;
+
+// Check that we can disable unsafe fields of HermesInternal.
+print(Object.getOwnPropertyNames(HermesInternal).length !== SAFE_FIELDS_COUNT);
 //CHKHIE: true
 //CHKHID: false
 //CHKIME: true
@@ -23,3 +26,15 @@ print(typeof HermesInternal.detachArrayBuffer);
 //CHKHID-NEXT: undefined
 //CHKIME-NEXT: function
 //CHKIMD-NEXT: undefined
+
+// Check that HermesInternal.concat is kept even HermesInternal is diabled.
+print(typeof HermesInternal.concat)
+//CHKHIE-NEXT: function
+//CHKHID-NEXT: function
+//CHKIME-NEXT: function
+//CHKIMD-NEXT: function
+print(`hello${1 + 1}world`);
+//CHKHIE-NEXT: hello2world
+//CHKHID-NEXT: hello2world
+//CHKIME-NEXT: hello2world
+//CHKIMD-NEXT: hello2world