Prevent throwing exceptions more than once during Locale construction.

Change code to propagate errors/exceptions to the top, and throw only once.

Bug: v8:7684
Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
Change-Id: I9a2d15cc4931f5bf029d2a8d78ad86e61f8db452
Reviewed-on: https://chromium-review.googlesource.com/1066808
Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53476}

src/builtins/builtins-intl.cc

@@ -517,8 +517,8 @@ BUILTIN(LocaleConstructor) {
   } else {  // [[Construct]]
     Handle<JSFunction> target = args.target();
     Handle<JSReceiver> new_target = Handle<JSReceiver>::cast(args.new_target());
-    Handle<JSObject> result;
 
+    Handle<JSObject> result;
     ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, result,
                                        JSObject::New(target, new_target));
 
@@ -550,13 +550,10 @@ BUILTIN(LocaleConstructor) {
                                          Object::ToObject(isolate, options));
     }
 
-    if (!JSLocale::InitializeLocale(isolate, Handle<JSLocale>::cast(result),
-                                    locale_string, options_object)) {
-      THROW_NEW_ERROR_RETURN_FAILURE(
-          isolate, NewTypeError(MessageTemplate::kLocaleBadParameters));
-    }
-
-    return *result;
+    RETURN_RESULT_OR_FAILURE(
+        isolate,
+        JSLocale::InitializeLocale(isolate, Handle<JSLocale>::cast(result),
+                                   locale_string, options_object));
   }
 }
 

src/objects/js-locale.cc

@@ -168,10 +168,11 @@ bool PopulateLocaleWithUnicodeTags(Isolate* isolate, const char* icu_locale,
 }
 }  // namespace
 
-bool JSLocale::InitializeLocale(Isolate* isolate,
-                                Handle<JSLocale> locale_holder,
-                                Handle<String> locale,
-                                Handle<JSReceiver> options) {
+MaybeHandle<JSLocale> JSLocale::InitializeLocale(Isolate* isolate,
+                                                 Handle<JSLocale> locale_holder,
+                                                 Handle<String> locale,
+                                                 Handle<JSReceiver> options) {
+  static const char* const kMethod = "Intl.Locale";
   v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate);
   UErrorCode status = U_ZERO_ERROR;
 
@@ -180,28 +181,54 @@ bool JSLocale::InitializeLocale(Isolate* isolate,
   char icu_canonical[ULOC_FULLNAME_CAPACITY];
 
   v8::String::Utf8Value bcp47_locale(v8_isolate, v8::Utils::ToLocal(locale));
-  if (bcp47_locale.length() == 0) return false;
+  if (bcp47_locale.length() == 0) return MaybeHandle<JSLocale>();
 
   int icu_length = uloc_forLanguageTag(
       *bcp47_locale, icu_result, ULOC_FULLNAME_CAPACITY, nullptr, &status);
 
   if (U_FAILURE(status) || status == U_STRING_NOT_TERMINATED_WARNING ||
       icu_length == 0) {
-    return false;
+    THROW_NEW_ERROR(
+        isolate,
+        NewRangeError(MessageTemplate::kLocaleBadParameters,
+                      isolate->factory()->NewStringFromAsciiChecked(kMethod),
+                      locale_holder),
+        JSLocale);
+    return MaybeHandle<JSLocale>();
   }
 
   Maybe<bool> error = InsertOptionsIntoLocale(isolate, options, icu_result);
-  if (error.IsNothing() || !error.FromJust()) {
-    return false;
+  MAYBE_RETURN(error, MaybeHandle<JSLocale>());
+  if (!error.FromJust()) {
+    THROW_NEW_ERROR(
+        isolate,
+        NewRangeError(MessageTemplate::kLocaleBadParameters,
+                      isolate->factory()->NewStringFromAsciiChecked(kMethod),
+                      locale_holder),
+        JSLocale);
+    return MaybeHandle<JSLocale>();
   }
+  DCHECK(error.FromJust());
 
   uloc_canonicalize(icu_result, icu_canonical, ULOC_FULLNAME_CAPACITY, &status);
   if (U_FAILURE(status) || status == U_STRING_NOT_TERMINATED_WARNING) {
-    return false;
+    THROW_NEW_ERROR(
+        isolate,
+        NewRangeError(MessageTemplate::kLocaleBadParameters,
+                      isolate->factory()->NewStringFromAsciiChecked(kMethod),
+                      locale_holder),
+        JSLocale);
+    return MaybeHandle<JSLocale>();
   }
 
   if (!PopulateLocaleWithUnicodeTags(isolate, icu_canonical, locale_holder)) {
-    return false;
+    THROW_NEW_ERROR(
+        isolate,
+        NewRangeError(MessageTemplate::kLocaleBadParameters,
+                      isolate->factory()->NewStringFromAsciiChecked(kMethod),
+                      locale_holder),
+        JSLocale);
+    return MaybeHandle<JSLocale>();
   }
 
   // Extract language, script and region parts.
@@ -215,7 +242,13 @@ bool JSLocale::InitializeLocale(Isolate* isolate,
   uloc_getCountry(icu_canonical, icu_region, ULOC_COUNTRY_CAPACITY, &status);
 
   if (U_FAILURE(status) || status == U_STRING_NOT_TERMINATED_WARNING) {
-    return false;
+    THROW_NEW_ERROR(
+        isolate,
+        NewRangeError(MessageTemplate::kLocaleBadParameters,
+                      isolate->factory()->NewStringFromAsciiChecked(kMethod),
+                      locale_holder),
+        JSLocale);
+    return MaybeHandle<JSLocale>();
   }
 
   Factory* factory = isolate->factory();
@@ -244,7 +277,13 @@ bool JSLocale::InitializeLocale(Isolate* isolate,
   uloc_toLanguageTag(icu_base_name, bcp47_result, ULOC_FULLNAME_CAPACITY, true,
                      &status);
   if (U_FAILURE(status) || status == U_STRING_NOT_TERMINATED_WARNING) {
-    return false;
+    THROW_NEW_ERROR(
+        isolate,
+        NewRangeError(MessageTemplate::kLocaleBadParameters,
+                      isolate->factory()->NewStringFromAsciiChecked(kMethod),
+                      locale_holder),
+        JSLocale);
+    return MaybeHandle<JSLocale>();
   }
   Handle<String> base_name = factory->NewStringFromAsciiChecked(bcp47_result);
   locale_holder->set_base_name(*base_name);
@@ -253,13 +292,19 @@ bool JSLocale::InitializeLocale(Isolate* isolate,
   uloc_toLanguageTag(icu_canonical, bcp47_result, ULOC_FULLNAME_CAPACITY, true,
                      &status);
   if (U_FAILURE(status) || status == U_STRING_NOT_TERMINATED_WARNING) {
-    return false;
+    THROW_NEW_ERROR(
+        isolate,
+        NewRangeError(MessageTemplate::kLocaleBadParameters,
+                      isolate->factory()->NewStringFromAsciiChecked(kMethod),
+                      locale_holder),
+        JSLocale);
+    return MaybeHandle<JSLocale>();
   }
   Handle<String> locale_handle =
       factory->NewStringFromAsciiChecked(bcp47_result);
   locale_holder->set_locale(*locale_handle);
 
-  return true;
+  return locale_holder;
 }
 
 }  // namespace internal

src/objects/js-locale.h

@@ -26,9 +26,10 @@ class JSLocale : public JSObject {
  public:
   // Initializes locale object with properties derived from input locale string
   // and options.
-  static bool InitializeLocale(Isolate* isolate, Handle<JSLocale> locale_holder,
-                               Handle<String> locale,
-                               Handle<JSReceiver> options);
+  static MaybeHandle<JSLocale> InitializeLocale(Isolate* isolate,
+                                                Handle<JSLocale> locale_holder,
+                                                Handle<String> locale,
+                                                Handle<JSReceiver> options);
 
   DECL_CAST(JSLocale)
 

test/intl/locale/locale-constructor.js

@@ -10,8 +10,8 @@ assertThrows(() => Intl.Locale('sr'), TypeError);
 // Non-string locale.
 assertThrows(() => new Intl.Locale(5), TypeError);
 
-// Invalid locale.
-assertThrows(() => new Intl.Locale('abcdefghi'), TypeError);
+// Invalid locale string.
+assertThrows(() => new Intl.Locale('abcdefghi'), RangeError);
 
 // Options will be force converted into Object.
 assertDoesNotThrow(() => new Intl.Locale('sr', 5));
@@ -29,4 +29,89 @@ assertThrows(
           numeric: 'true',
           numberingSystem: 'roman',
         }),
-    TypeError);
+    RangeError);
+
+// Throws only once during construction.
+// Check for all getters to prevent regression.
+assertThrows(
+    () => new Intl.Locale('en-US', {
+      get calendar() {
+        throw new Error('foo');
+      }
+    }),
+    Error);
+
+assertThrows(
+    () => new Intl.Locale('en-US', {
+      get caseFirst() {
+        throw new Error('foo');
+      }
+    }),
+    Error);
+
+assertThrows(
+    () => new Intl.Locale('en-US', {
+      get collation() {
+        throw new Error('foo');
+      }
+    }),
+    Error);
+
+assertThrows(
+    () => new Intl.Locale('en-US', {
+      get hourCycle() {
+        throw new Error('foo');
+      }
+    }),
+    Error);
+
+assertThrows(
+    () => new Intl.Locale('en-US', {
+      get numeric() {
+        throw new Error('foo');
+      }
+    }),
+    Error);
+
+assertThrows(
+    () => new Intl.Locale('en-US', {
+      get numberingSystem() {
+        throw new Error('foo');
+      }
+    }),
+    Error);
+
+// These don't throw yet, we need to implement language/script/region
+// override logic first.
+assertDoesNotThrow(
+    () => new Intl.Locale('en-US', {
+      get language() {
+        throw new Error('foo');
+      }
+    }),
+    Error);
+
+assertDoesNotThrow(
+    () => new Intl.Locale('en-US', {
+      get script() {
+        throw new Error('foo');
+      }
+    }),
+    Error);
+
+assertDoesNotThrow(
+    () => new Intl.Locale('en-US', {
+      get region() {
+        throw new Error('foo');
+      }
+    }),
+    Error);
+
+// There won't be an override for baseName so we don't expect it to throw.
+assertDoesNotThrow(
+    () => new Intl.Locale('en-US', {
+      get baseName() {
+        throw new Error('foo');
+      }
+    }),
+    Error);