Fix RequestInit IDL compatibility issues

RequestInit is manually implemented and has some compatibility issues.

 1. undefined value is treated as a real value, e.g.,
    (new Request('/', {method: undefined}).method
    returns "undefined".
 2. Exceptions are silently ignored, e.g.,
    new Request('/', {get method() { throw Error(); }})
    doesn't throw.

This change fixes the issues.

Bug: 775318
Change-Id: I3d5b2b49f47cfdf92ba0707f6af0260653e386d9
Reviewed-on: https://chromium-review.googlesource.com/722543
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509714}

third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js

@@ -560,6 +560,72 @@ test(() => {
     assert_throws({name: 'TypeError'}, () => { req.clone(); });
   }, 'Used => clone');
 
+test(() => {
+  // We implement RequestInit manually so we need to test the functionality
+  // here.
+  function undefined_notpresent(property_name) {
+    assert_not_equals(property_name, 'referrer', 'property_name');
+    const request = new Request('/', {referrer: '/'});
+    let init = {};
+    init[property_name] = undefined;
+    assert_equals((new Request(request, init)).referrer, request.url,
+                  property_name);
+  }
+
+  undefined_notpresent('method');
+  undefined_notpresent('headers');
+  undefined_notpresent('body');
+  undefined_notpresent('referrerPolicy');
+  undefined_notpresent('mode');
+  undefined_notpresent('credentials');
+  undefined_notpresent('cache');
+  undefined_notpresent('redirect');
+  undefined_notpresent('integrity');
+  undefined_notpresent('keepalive');
+  undefined_notpresent('signal');
+  undefined_notpresent('window');
+
+  // |undefined_notpresent| uses referrer for testing, so we need to test
+  // the property manually.
+  const request = new Request('/', {referrerPolicy: 'same-origin'});
+  assert_equals(new Request(request, {referrer: undefined}).referrerPolicy,
+                'same-origin', 'referrer');
+}, 'An undefined member should be treated as not-present');
+
+test(() => {
+  // We implement RequestInit manually so we need to test the functionality
+  // here.
+  const e = Error();
+  assert_throws(e, () => {
+    new Request('/', {get method() { throw e; }})}, 'method');
+  assert_throws(e, () => {
+    new Request('/', {get headers() { throw e; }})}, 'headers');
+  assert_throws(e, () => {
+    new Request('/', {get body() { throw e; }})}, 'body');
+  assert_throws(e, () => {
+    new Request('/', {get referrer() { throw e; }})}, 'referrer');
+  assert_throws(e, () => {
+    new Request('/', {get referrerPolicy() { throw e; }})}, 'referrerPolicy');
+  assert_throws(e, () => {
+    new Request('/', {get mode() { throw e; }})}, 'mode');
+  assert_throws(e, () => {
+    new Request('/', {get credentials() { throw e; }})}, 'credentials');
+  assert_throws(e, () => {
+    new Request('/', {get cache() { throw e; }})}, 'cache');
+  assert_throws(e, () => {
+    new Request('/', {get redirect() { throw e; }})}, 'redirect');
+  assert_throws(e, () => {
+    new Request('/', {get integrity() { throw e; }})}, 'integrity');
+
+  // Not implemented
+  // assert_throws(e, () => {
+  //  new Request('/', {get keepalive() { throw e; }})}, 'keepalive');
+  // assert_throws(e, () => {
+  //  new Request('/', {get signal() { throw e; }})}, 'signal');
+  // assert_throws(e, () => {
+  //  new Request('/', {get window() { throw e; }})}, 'window');
+}, 'Getter exceptions should not be silently ignored');
+
 promise_test(function() {
     var headers = new Headers;
     headers.set('Content-Language', 'ja');

third_party/WebKit/Source/bindings/core/v8/Dictionary.h

@@ -162,16 +162,6 @@ struct DictionaryHelper {
   template <typename T>
   static bool Get(const Dictionary&, const StringView& key, T& value);
   template <typename T>
-  static bool GetWithUndefinedCheck(const Dictionary& dictionary,
-                                    const StringView& key,
-                                    T& value) {
-    v8::Local<v8::Value> v8_value;
-    if (!dictionary.Get(key, v8_value) || v8_value.IsEmpty() ||
-        v8_value->IsUndefined())
-      return false;
-    return DictionaryHelper::Get(dictionary, key, value);
-  }
-  template <typename T>
   static bool Get(const Dictionary&, const StringView& key, Nullable<T>& value);
 };
 

third_party/WebKit/Source/modules/fetch/RequestInit.cpp

@@ -5,6 +5,9 @@
 #include "modules/fetch/RequestInit.h"
 
 #include "bindings/core/v8/Dictionary.h"
+#include "bindings/core/v8/ExceptionState.h"
+#include "bindings/core/v8/IDLTypes.h"
+#include "bindings/core/v8/NativeValueTraitsImpl.h"
 #include "bindings/core/v8/V8ArrayBuffer.h"
 #include "bindings/core/v8/V8ArrayBufferView.h"
 #include "bindings/core/v8/V8BindingForCore.h"
@@ -27,118 +30,194 @@
 
 namespace blink {
 
-// TODO(yiyix): Verify if any DictionaryHelper::get should be replaced with
-// DictionaryHelper::getWithUndefinedCheck.
+struct RequestInit::IDLPassThrough
+    : public IDLBaseHelper<v8::Local<v8::Value>> {};
+
+template <>
+struct NativeValueTraits<RequestInit::IDLPassThrough>
+    : public NativeValueTraitsBase<RequestInit::IDLPassThrough> {
+  static v8::Local<v8::Value> NativeValue(v8::Isolate* isolate,
+                                          v8::Local<v8::Value> value,
+                                          ExceptionState& exception_state) {
+    DCHECK(!value.IsEmpty());
+    return value;
+  }
+};
+
+class RequestInit::GetterHelper {
+  STACK_ALLOCATED();
+  WTF_MAKE_NONCOPYABLE(GetterHelper);
+
+ public:
+  // |this| object must not outlive |src| and |exception_state|.
+  GetterHelper(const Dictionary& src, ExceptionState& exception_state)
+      : src_(src), exception_state_(exception_state) {}
+
+  template <typename IDLType>
+  WTF::Optional<typename IDLType::ImplType> Get(const StringView& key) {
+    auto r = src_.Get<IDLType>(key, exception_state_);
+    are_any_members_set_ = are_any_members_set_ || r.has_value();
+    return r;
+  }
+
+  bool AreAnyMembersSet() const { return are_any_members_set_; }
+
+ private:
+  const Dictionary& src_;
+  ExceptionState& exception_state_;
+  bool are_any_members_set_ = false;
+};
+
 RequestInit::RequestInit(ExecutionContext* context,
                          const Dictionary& options,
                          ExceptionState& exception_state) {
-  are_any_members_set_ |= DictionaryHelper::Get(options, "method", method_);
-
-  // From https://github.com/whatwg/fetch/issues/479:
-  // - undefined is the same as "this member has not been passed".
-  // - {} means "the list of headers is empty", so the member has been set.
-  // - null is an invalid value for both sequences and records, but it is not
-  //   the same as undefined: a value has been set, even if invalid, and will
-  //   throw a TypeError later when it gets converted to a HeadersInit object.
-  v8::Local<v8::Value> v8_headers;
-  bool is_header_set =
-      options.Get("headers", v8_headers) && !v8_headers->IsUndefined();
-  are_any_members_set_ |= is_header_set;
-
-  are_any_members_set_ |= DictionaryHelper::Get(options, "mode", mode_);
-  if (RuntimeEnabledFeatures::FetchRequestCacheEnabled())
-    are_any_members_set_ |= DictionaryHelper::Get(options, "cache", cache_);
-
-  are_any_members_set_ |= DictionaryHelper::Get(options, "redirect", redirect_);
-  AtomicString referrer_string;
-  bool is_referrer_string_set = DictionaryHelper::GetWithUndefinedCheck(
-      options, "referrer", referrer_string);
-  are_any_members_set_ |= is_referrer_string_set;
-  are_any_members_set_ |=
-      DictionaryHelper::Get(options, "integrity", integrity_);
-  AtomicString referrer_policy_string;
-  bool is_referrer_policy_set =
-      DictionaryHelper::Get(options, "referrerPolicy", referrer_policy_string);
-  are_any_members_set_ |= is_referrer_policy_set;
-
-  v8::Local<v8::Value> v8_body;
-  bool is_body_set = options.Get("body", v8_body);
-  are_any_members_set_ |= is_body_set;
-
-  v8::Local<v8::Value> v8_credential;
-  bool is_credential_set = options.Get("credentials", v8_credential);
-  are_any_members_set_ |= is_credential_set;
-
-  if (are_any_members_set_) {
-    // A part of the Request constructor algorithm is performed here. See
-    // the comments in the Request constructor code for the detail.
-
-    // We need to use "about:client" instead of |clientReferrerString|,
-    // because "about:client" => |clientReferrerString| conversion is done
-    // in Request::createRequestWithRequestOrString.
-    referrer_ = Referrer("about:client", kReferrerPolicyDefault);
-    if (is_referrer_string_set)
-      referrer_.referrer = referrer_string;
-    if (is_referrer_policy_set) {
-      if (referrer_policy_string == "") {
-        referrer_.referrer_policy = kReferrerPolicyDefault;
-      } else if (referrer_policy_string == "no-referrer") {
-        referrer_.referrer_policy = kReferrerPolicyNever;
-      } else if (referrer_policy_string == "no-referrer-when-downgrade") {
-        referrer_.referrer_policy = kReferrerPolicyNoReferrerWhenDowngrade;
-      } else if (referrer_policy_string == "origin") {
-        referrer_.referrer_policy = kReferrerPolicyOrigin;
-      } else if (referrer_policy_string == "origin-when-cross-origin") {
-        referrer_.referrer_policy = kReferrerPolicyOriginWhenCrossOrigin;
-      } else if (referrer_policy_string == "same-origin") {
-        referrer_.referrer_policy = kReferrerPolicySameOrigin;
-      } else if (referrer_policy_string == "strict-origin") {
-        referrer_.referrer_policy = kReferrerPolicyStrictOrigin;
-      } else if (referrer_policy_string == "unsafe-url") {
-        referrer_.referrer_policy = kReferrerPolicyAlways;
-      } else if (referrer_policy_string == "strict-origin-when-cross-origin") {
-        referrer_.referrer_policy =
-            kReferrerPolicyNoReferrerWhenDowngradeOriginWhenCrossOrigin;
-      } else {
-        exception_state.ThrowTypeError("Invalid referrer policy");
-        return;
-      }
-    }
+  GetterHelper h(options, exception_state);
+
+  method_ = h.Get<IDLByteString>("method").value_or(String());
+  if (exception_state.HadException())
+    return;
+
+  auto v8_headers = h.Get<IDLPassThrough>("headers");
+  if (exception_state.HadException())
+    return;
+
+  mode_ = h.Get<IDLUSVString>("mode").value_or(String());
+  if (exception_state.HadException())
+    return;
+
+  if (RuntimeEnabledFeatures::FetchRequestCacheEnabled()) {
+    cache_ = h.Get<IDLUSVString>("cache").value_or(String());
+    if (exception_state.HadException())
+      return;
   }
 
+  redirect_ = h.Get<IDLUSVString>("redirect").value_or(String());
+  if (exception_state.HadException())
+    return;
+
+  auto referrer_string = h.Get<IDLUSVString>("referrer");
+  if (exception_state.HadException())
+    return;
+
+  auto referrer_policy_string = h.Get<IDLUSVString>("referrerPolicy");
+  if (exception_state.HadException())
+    return;
+
+  integrity_ = h.Get<IDLString>("integrity").value_or(String());
+  if (exception_state.HadException())
+    return;
+
+  auto v8_body = h.Get<IDLPassThrough>("body");
+  if (exception_state.HadException())
+    return;
+
+  auto v8_credentials = h.Get<IDLPassThrough>("credentials");
+  if (exception_state.HadException())
+    return;
+
+  are_any_members_set_ = h.AreAnyMembersSet();
+
+  SetUpReferrer(referrer_string, referrer_policy_string, exception_state);
+  if (exception_state.HadException())
+    return;
+
   v8::Isolate* isolate = ToIsolate(context);
 
-  if (is_header_set) {
+  if (v8_headers.has_value()) {
     V8ByteStringSequenceSequenceOrByteStringByteStringRecord::ToImpl(
-        isolate, v8_headers, headers_, UnionTypeConversionMode::kNotNullable,
+        isolate, *v8_headers, headers_, UnionTypeConversionMode::kNotNullable,
         exception_state);
     if (exception_state.HadException())
       return;
   }
 
-  if (is_credential_set) {
-    if (V8PasswordCredential::hasInstance(v8_credential, isolate)) {
-      Deprecation::CountDeprecation(context,
-                                    WebFeature::kCredentialManagerCustomFetch);
-      // TODO(mkwst): According to the spec, we'd serialize this once we touch
-      // the network. We're serializing it here, ahead of time, because lifetime
-      // issues around ResourceRequest make it pretty difficult to pass a
-      // PasswordCredential around at the platform level, and the hop between
-      // the browser and renderer processes to deal with service workers is
-      // equally painful. There should be no developer-visible difference in
-      // behavior with this option, except that the `Content-Type` header will
-      // be set early. That seems reasonable.
-      PasswordCredential* credential =
-          V8PasswordCredential::ToImpl(v8_credential.As<v8::Object>());
-      attached_credential_ = credential->EncodeFormData(content_type_);
-      credentials_ = "password";
-    } else if (v8_credential->IsString()) {
-      credentials_ = ToUSVString(isolate, v8_credential, exception_state);
+  if (v8_credentials.has_value()) {
+    SetUpCredentials(context, isolate, *v8_credentials, exception_state);
+    if (exception_state.HadException())
+      return;
+  }
+
+  if (v8_body.has_value()) {
+    SetUpBody(context, isolate, *v8_body, exception_state);
+    if (exception_state.HadException())
+      return;
+  }
+}
+
+void RequestInit::SetUpReferrer(
+    const WTF::Optional<String>& referrer_string,
+    const WTF::Optional<String>& referrer_policy_string,
+    ExceptionState& exception_state) {
+  if (!are_any_members_set_)
+    return;
+
+  // A part of the Request constructor algorithm is performed here. See
+  // the comments in the Request constructor code for the detail.
+
+  // We need to use "about:client" instead of |clientReferrerString|,
+  // because "about:client" => |clientReferrerString| conversion is done
+  // in Request::createRequestWithRequestOrString.
+  referrer_ = Referrer("about:client", kReferrerPolicyDefault);
+  if (referrer_string.has_value())
+    referrer_.referrer = AtomicString(*referrer_string);
+  if (referrer_policy_string.has_value()) {
+    if (*referrer_policy_string == "") {
+      referrer_.referrer_policy = kReferrerPolicyDefault;
+    } else if (*referrer_policy_string == "no-referrer") {
+      referrer_.referrer_policy = kReferrerPolicyNever;
+    } else if (*referrer_policy_string == "no-referrer-when-downgrade") {
+      referrer_.referrer_policy = kReferrerPolicyNoReferrerWhenDowngrade;
+    } else if (*referrer_policy_string == "origin") {
+      referrer_.referrer_policy = kReferrerPolicyOrigin;
+    } else if (*referrer_policy_string == "origin-when-cross-origin") {
+      referrer_.referrer_policy = kReferrerPolicyOriginWhenCrossOrigin;
+    } else if (*referrer_policy_string == "same-origin") {
+      referrer_.referrer_policy = kReferrerPolicySameOrigin;
+    } else if (*referrer_policy_string == "strict-origin") {
+      referrer_.referrer_policy = kReferrerPolicyStrictOrigin;
+    } else if (*referrer_policy_string == "unsafe-url") {
+      referrer_.referrer_policy = kReferrerPolicyAlways;
+    } else if (*referrer_policy_string == "strict-origin-when-cross-origin") {
+      referrer_.referrer_policy =
+          kReferrerPolicyNoReferrerWhenDowngradeOriginWhenCrossOrigin;
+    } else {
+      exception_state.ThrowTypeError("Invalid referrer policy");
+      return;
     }
   }
+}
+
+void RequestInit::SetUpCredentials(ExecutionContext* context,
+                                   v8::Isolate* isolate,
+                                   v8::Local<v8::Value> v8_credentials,
+                                   ExceptionState& exception_state) {
+  if (V8PasswordCredential::hasInstance(v8_credentials, isolate)) {
+    Deprecation::CountDeprecation(context,
+                                  WebFeature::kCredentialManagerCustomFetch);
+    // TODO(mkwst): According to the spec, we'd serialize this once we touch
+    // the network. We're serializing it here, ahead of time, because lifetime
+    // issues around ResourceRequest make it pretty difficult to pass a
+    // PasswordCredential around at the platform level, and the hop between
+    // the browser and renderer processes to deal with service workers is
+    // equally painful. There should be no developer-visible difference in
+    // behavior with this option, except that the `Content-Type` header will
+    // be set early. That seems reasonable.
+    PasswordCredential* credential =
+        V8PasswordCredential::ToImpl(v8_credentials.As<v8::Object>());
+    attached_credential_ = credential->EncodeFormData(content_type_);
+    credentials_ = "password";
+  } else if (v8_credentials->IsString()) {
+    credentials_ = ToUSVString(isolate, v8_credentials, exception_state);
+    if (exception_state.HadException())
+      return;
+  }
+}
 
-  if (attached_credential_.get() || !is_body_set || v8_body->IsUndefined() ||
-      v8_body->IsNull())
+void RequestInit::SetUpBody(ExecutionContext* context,
+                            v8::Isolate* isolate,
+                            v8::Local<v8::Value> v8_body,
+                            ExceptionState& exception_state) {
+  if (attached_credential_ || v8_body->IsNull())
     return;
 
   if (v8_body->IsArrayBuffer()) {

third_party/WebKit/Source/modules/fetch/RequestInit.h

@@ -5,11 +5,13 @@
 #ifndef RequestInit_h
 #define RequestInit_h
 
+#include "bindings/core/v8/NativeValueTraits.h"
 #include "bindings/modules/v8/byte_string_sequence_sequence_or_byte_string_byte_string_record.h"
 #include "modules/fetch/Headers.h"
 #include "platform/heap/Handle.h"
 #include "platform/network/EncodedFormData.h"
 #include "platform/weborigin/Referrer.h"
+#include "platform/wtf/Optional.h"
 #include "platform/wtf/RefPtr.h"
 #include "platform/wtf/text/WTFString.h"
 
@@ -41,6 +43,24 @@ class RequestInit {
   bool AreAnyMembersSet() const { return are_any_members_set_; }
 
  private:
+  // These are defined here to avoid JUMBO ambiguity.
+  class GetterHelper;
+  struct IDLPassThrough;
+  friend struct NativeValueTraits<IDLPassThrough>;
+  friend struct NativeValueTraitsBase<IDLPassThrough>;
+
+  void SetUpReferrer(const WTF::Optional<String>& referrer_string,
+                     const WTF::Optional<String>& referrer_policy_string,
+                     ExceptionState&);
+  void SetUpCredentials(ExecutionContext*,
+                        v8::Isolate*,
+                        v8::Local<v8::Value> v8_credentials,
+                        ExceptionState&);
+  void SetUpBody(ExecutionContext*,
+                 v8::Isolate*,
+                 v8::Local<v8::Value> v8_body,
+                 ExceptionState&);
+
   String method_;
   HeadersInit headers_;
   String content_type_;