v8bindings: Fix History.state === PopStateEvent.state

"history.state === popStateEvent.state" should be evaluated to
true, however, the result changes depending on their evaluation
order.  See the bug for details.

This patch removes the custom bindings of PopStateEvent.state
and reimplements History.state and PopStateEvent.state in a
less hacky way, moving caching with V8 private property from
Blink-V8 bindings into Blink implementation.

WPT failures in this patch are expected.  I'll send a fix of
WPT separately.

Bug: 1070938
Change-Id: Iaf9bdbc6545cdbe18d8f3542944dfce61c443da1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2147488Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759223}

third_party/blink/renderer/bindings/bindings.gni

@@ -28,7 +28,6 @@ bindings_core_v8_files =
                     "core/v8/custom/v8_dev_tools_host_custom.cc",
                     "core/v8/custom/v8_html_all_collection_custom.cc",
                     "core/v8/custom/v8_html_plugin_element_custom.cc",
-                    "core/v8/custom/v8_pop_state_event_custom.cc",
                     "core/v8/custom/v8_promise_rejection_event_custom.cc",
                     "core/v8/custom/v8_window_custom.cc",
                     "core/v8/custom/v8_xml_http_request_custom.cc",

third_party/blink/renderer/bindings/core/v8/custom/v8_pop_state_event_custom.cc

-/*
- * Copyright (C) 2010 Google Inc. All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are
- * met:
- *
- *     * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- *     * Redistributions in binary form must reproduce the above
- * copyright notice, this list of conditions and the following disclaimer
- * in the documentation and/or other materials provided with the
- * distribution.
- *     * Neither the name of Google Inc. nor the names of its
- * contributors may be used to endorse or promote products derived from
- * this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#include "third_party/blink/renderer/bindings/core/v8/v8_pop_state_event.h"
-
-#include "third_party/blink/renderer/bindings/core/v8/v8_history.h"
-#include "third_party/blink/renderer/core/events/pop_state_event.h"
-#include "third_party/blink/renderer/core/frame/history.h"
-#include "third_party/blink/renderer/platform/bindings/v8_private_property.h"
-
-namespace blink {
-
-const V8PrivateProperty::SymbolKey kPrivatePropertyState;
-
-// Save the state value to a hidden attribute in the V8PopStateEvent, and return
-// it, for convenience.
-static v8::Local<v8::Value> CacheState(ScriptState* script_state,
-                                       v8::Local<v8::Object> pop_state_event,
-                                       v8::Local<v8::Value> state) {
-  V8PrivateProperty::GetSymbol(script_state->GetIsolate(),
-                               kPrivatePropertyState)
-      .Set(pop_state_event, state);
-  return state;
-}
-
-void V8PopStateEvent::StateAttributeGetterCustom(
-    const v8::FunctionCallbackInfo<v8::Value>& info) {
-  v8::Isolate* isolate = info.GetIsolate();
-  ScriptState* script_state = ScriptState::Current(isolate);
-  V8PrivateProperty::Symbol property_symbol =
-      V8PrivateProperty::GetSymbol(isolate, kPrivatePropertyState);
-  v8::Local<v8::Value> result;
-
-  if (property_symbol.GetOrUndefined(info.Holder()).ToLocal(&result) &&
-      !result->IsUndefined()) {
-    V8SetReturnValue(info, result);
-    return;
-  }
-
-  PopStateEvent* event = V8PopStateEvent::ToImpl(info.Holder());
-  History* history = event->GetHistory();
-  if (!history || !event->SerializedState()) {
-    // If the event doesn't have serializedState(), it means that the
-    // event was initialized with PopStateEventInit. In such case, we need
-    // to get a v8 value for the current world from state().
-    if (event->SerializedState())
-      result = event->SerializedState()->Deserialize(isolate);
-    else
-      result = event->state(script_state).V8Value();
-    if (result.IsEmpty())
-      result = v8::Null(isolate);
-    V8SetReturnValue(info, CacheState(script_state, info.Holder(), result));
-    return;
-  }
-
-  // There's no cached value from a previous invocation, nor a state value was
-  // provided by the event, but there is a history object, so first we need to
-  // see if the state object has been deserialized through the history object
-  // already.
-  // The current history state object might've changed in the meantime, so we
-  // need to take care of using the correct one, and always share the same
-  // deserialization with history.state.
-
-  bool is_same_state = history->IsSameAsCurrentState(event->SerializedState());
-  if (is_same_state) {
-    V8PrivateProperty::Symbol history_state =
-        V8PrivateProperty::GetHistoryStateSymbol(info.GetIsolate());
-    v8::Local<v8::Value> v8_history_value =
-        ToV8(history, info.Holder(), isolate);
-    if (v8_history_value.IsEmpty())
-      return;
-    v8::Local<v8::Object> v8_history = v8_history_value.As<v8::Object>();
-    if (!history->stateChanged() && history_state.HasValue(v8_history)) {
-      v8::Local<v8::Value> value;
-      if (!history_state.GetOrUndefined(v8_history).ToLocal(&value))
-        return;
-      V8SetReturnValue(info, CacheState(script_state, info.Holder(), value));
-      return;
-    }
-    result = event->SerializedState()->Deserialize(isolate);
-    history_state.Set(v8_history, result);
-  } else {
-    result = event->SerializedState()->Deserialize(isolate);
-  }
-
-  V8SetReturnValue(info, CacheState(script_state, info.Holder(), result));
-}
-
-}  // namespace blink

third_party/blink/renderer/bindings/templates/attributes.cc.tmpl

@@ -71,10 +71,6 @@ const v8::FunctionCallbackInfo<v8::Value>& info
 
   {% if attribute.cached_attribute_validation_method %}
   // [CachedAttribute]
-  {% if cpp_class == 'History' and attribute.camel_case_name == 'State' %}
-  V8PrivateProperty::Symbol property_symbol =
-      V8PrivateProperty::GetHistoryStateSymbol(info.GetIsolate());
-  {% else %}
   {% if not attribute.private_property_is_shared_between_getter_and_setter %}
   static const V8PrivateProperty::SymbolKey kPrivateProperty{{attribute.camel_case_name}};
 
@@ -82,7 +78,6 @@ const v8::FunctionCallbackInfo<v8::Value>& info
   V8PrivateProperty::Symbol property_symbol =
       V8PrivateProperty::GetSymbol(info.GetIsolate(),
           kPrivateProperty{{attribute.camel_case_name}});
-  {% endif %}
   if (!static_cast<const {{cpp_class}}*>(impl)->{{attribute.cached_attribute_validation_method}}()) {
     v8::Local<v8::Value> v8_value;
     if (property_symbol.GetOrUndefined(holder).ToLocal(&v8_value) && !v8_value->IsUndefined()) {

third_party/blink/renderer/core/events/pop_state_event.cc

@@ -33,16 +33,32 @@
 
 namespace blink {
 
-PopStateEvent::PopStateEvent()
-    : serialized_state_(nullptr), history_(nullptr) {}
+PopStateEvent* PopStateEvent::Create() {
+  return MakeGarbageCollected<PopStateEvent>();
+}
+
+PopStateEvent* PopStateEvent::Create(ScriptState* script_state,
+                                     const AtomicString& type,
+                                     const PopStateEventInit* initializer) {
+  return MakeGarbageCollected<PopStateEvent>(script_state, type, initializer);
+}
+
+PopStateEvent* PopStateEvent::Create(
+    scoped_refptr<SerializedScriptValue> serialized_state,
+    History* history) {
+  return MakeGarbageCollected<PopStateEvent>(std::move(serialized_state),
+                                             history);
+}
 
 PopStateEvent::PopStateEvent(ScriptState* script_state,
                              const AtomicString& type,
                              const PopStateEventInit* initializer)
-    : Event(type, initializer), history_(nullptr) {
+    : Event(type, initializer) {
+  v8::Isolate* isolate = script_state->GetIsolate();
   if (initializer->hasState()) {
-    world_ = WrapRefCounted(&script_state->World());
-    state_.Set(script_state->GetIsolate(), initializer->state().V8Value());
+    state_.Set(isolate, initializer->state().V8Value());
+  } else {
+    state_.Set(isolate, v8::Null(isolate));
   }
 }
 
@@ -53,43 +69,27 @@ PopStateEvent::PopStateEvent(
       serialized_state_(std::move(serialized_state)),
       history_(history) {}
 
-PopStateEvent::~PopStateEvent() = default;
-
-ScriptValue PopStateEvent::state(ScriptState* script_state) const {
-  if (state_.IsEmpty())
-    return ScriptValue();
-
+ScriptValue PopStateEvent::state(ScriptState* script_state,
+                                 ExceptionState& exception_state) {
   v8::Isolate* isolate = script_state->GetIsolate();
-  if (world_->GetWorldId() != script_state->World().GetWorldId()) {
-    v8::Local<v8::Value> value = state_.NewLocal(isolate);
-    scoped_refptr<SerializedScriptValue> serialized =
-        SerializedScriptValue::SerializeAndSwallowExceptions(isolate, value);
-    return ScriptValue(isolate, serialized->Deserialize(isolate));
-  }
-  return ScriptValue(isolate, state_.NewLocal(isolate));
-}
 
-PopStateEvent* PopStateEvent::Create() {
-  return MakeGarbageCollected<PopStateEvent>();
-}
+  if (!state_.IsEmpty())
+    return ScriptValue(isolate, state_.GetAcrossWorld(script_state));
 
-PopStateEvent* PopStateEvent::Create(
-    scoped_refptr<SerializedScriptValue> serialized_state,
-    History* history) {
-  return MakeGarbageCollected<PopStateEvent>(std::move(serialized_state),
-                                             history);
-}
+  if (history_ && history_->IsSameAsCurrentState(serialized_state_.get())) {
+    return history_->state(script_state, exception_state);
+  }
 
-PopStateEvent* PopStateEvent::Create(ScriptState* script_state,
-                                     const AtomicString& type,
-                                     const PopStateEventInit* initializer) {
-  return MakeGarbageCollected<PopStateEvent>(script_state, type, initializer);
-}
+  v8::Local<v8::Value> v8_state;
+  if (serialized_state_) {
+    ScriptState::EscapableScope target_context_scope(script_state);
+    v8_state =
+        target_context_scope.Escape(serialized_state_->Deserialize(isolate));
+  } else {
+    v8_state = v8::Null(isolate);
+  }
 
-void PopStateEvent::SetSerializedState(
-    scoped_refptr<SerializedScriptValue> state) {
-  DCHECK(!serialized_state_);
-  serialized_state_ = std::move(state);
+  return ScriptValue(isolate, v8_state);
 }
 
 const AtomicString& PopStateEvent::InterfaceName() const {

third_party/blink/renderer/core/events/pop_state_event.h

@@ -28,9 +28,8 @@
 #define THIRD_PARTY_BLINK_RENDERER_CORE_EVENTS_POP_STATE_EVENT_H_
 
 #include "third_party/blink/renderer/bindings/core/v8/v8_pop_state_event_init.h"
+#include "third_party/blink/renderer/bindings/core/v8/world_safe_v8_reference.h"
 #include "third_party/blink/renderer/core/dom/events/event.h"
-#include "third_party/blink/renderer/platform/bindings/dom_wrapper_world.h"
-#include "third_party/blink/renderer/platform/bindings/trace_wrapper_v8_reference.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 
 namespace blink {
@@ -38,36 +37,36 @@ namespace blink {
 class History;
 class SerializedScriptValue;
 
-class PopStateEvent final : public Event {
+class CORE_EXPORT PopStateEvent final : public Event {
   DEFINE_WRAPPERTYPEINFO();
 
  public:
-  PopStateEvent();
-  PopStateEvent(ScriptState*, const AtomicString&, const PopStateEventInit*);
-  PopStateEvent(scoped_refptr<SerializedScriptValue>, History*);
-  ~PopStateEvent() override;
-
   static PopStateEvent* Create();
-  static PopStateEvent* Create(scoped_refptr<SerializedScriptValue>, History*);
-  static PopStateEvent* Create(ScriptState*,
-                               const AtomicString&,
-                               const PopStateEventInit*);
+  static PopStateEvent* Create(ScriptState* script_state,
+                               const AtomicString& type,
+                               const PopStateEventInit* initializer);
+  static PopStateEvent* Create(
+      scoped_refptr<SerializedScriptValue> serialized_state,
+      History* history);
+
+  PopStateEvent() = default;
+  PopStateEvent(ScriptState* script_state,
+                const AtomicString& type,
+                const PopStateEventInit* initializer);
+  PopStateEvent(scoped_refptr<SerializedScriptValue> serialized_state,
+                History* history);
+  ~PopStateEvent() override = default;
 
-  ScriptValue state(ScriptState*) const;
-  SerializedScriptValue* SerializedState() const {
-    return serialized_state_.get();
-  }
-  void SetSerializedState(scoped_refptr<SerializedScriptValue> state);
-  History* GetHistory() const { return history_.Get(); }
+  ScriptValue state(ScriptState* script_state, ExceptionState& exception_state);
+  bool IsStateDirty() const { return false; }
 
   const AtomicString& InterfaceName() const override;
 
   void Trace(Visitor*) override;
 
  private:
+  WorldSafeV8Reference<v8::Value> state_;
   scoped_refptr<SerializedScriptValue> serialized_state_;
-  scoped_refptr<DOMWrapperWorld> world_;
-  TraceWrapperV8Reference<v8::Value> state_;
   Member<History> history_;
 };
 

third_party/blink/renderer/core/events/pop_state_event.idl

@@ -31,5 +31,5 @@
     Exposed=Window
 ] interface PopStateEvent : Event {
     [CallWith=ScriptState] constructor(DOMString type, optional PopStateEventInit eventInitDict = {});
-    [Custom=Getter] readonly attribute any state;
+    [CallWith=ScriptState, RaisesException, CachedAttribute=IsStateDirty] readonly attribute any state;
 };

third_party/blink/renderer/core/events/pop_state_event_init.idl

@@ -5,5 +5,5 @@
 // https://html.spec.whatwg.org/C/#the-popstateevent-interface
 
 dictionary PopStateEventInit : EventInit {
-    any state;
+    any state = null;
 };

third_party/blink/renderer/core/frame/history.cc

@@ -78,19 +78,42 @@ unsigned History::length(ExceptionState& exception_state) const {
   return GetFrame()->Client()->BackForwardLength();
 }
 
-ScriptValue History::state(v8::Isolate* isolate,
+ScriptValue History::state(ScriptState* script_state,
                            ExceptionState& exception_state) {
+  v8::Isolate* isolate = script_state->GetIsolate();
+  static const V8PrivateProperty::SymbolKey kHistoryStatePrivateProperty;
+  auto private_prop =
+      V8PrivateProperty::GetSymbol(isolate, kHistoryStatePrivateProperty);
+  v8::Local<v8::Object> v8_history = ToV8(this, script_state).As<v8::Object>();
+  v8::Local<v8::Value> v8_state;
+
+  // Returns the same V8 value unless the history gets updated.  This
+  // implementation is mostly the same as the one of [CachedAttribute], but
+  // it's placed in this function rather than in Blink-V8 bindings layer so
+  // that PopStateEvent.state can also access the same V8 value.
+  scoped_refptr<SerializedScriptValue> current_state = StateInternal();
+  if (last_state_object_requested_ == current_state) {
+    if (!private_prop.GetOrUndefined(v8_history).ToLocal(&v8_state))
+      return ScriptValue::CreateNull(isolate);
+    if (!v8_state->IsUndefined())
+      return ScriptValue(isolate, v8_state);
+  }
+
   if (!GetFrame()) {
     exception_state.ThrowSecurityError(
-        "May not use a History object associated with a Document that is not "
-        "fully active");
-    return ScriptValue::CreateNull(isolate);
+        "May not use a History object associated with a Document that is "
+        "not fully active");
+    v8_state = v8::Null(isolate);
+  } else if (!current_state) {
+    v8_state = v8::Null(isolate);
+  } else {
+    ScriptState::EscapableScope target_context_scope(script_state);
+    v8_state = target_context_scope.Escape(current_state->Deserialize(isolate));
   }
-  last_state_object_requested_ = StateInternal();
-  if (!last_state_object_requested_)
-    return ScriptValue::CreateNull(isolate);
-  return ScriptValue(isolate,
-                     last_state_object_requested_->Deserialize(isolate));
+
+  last_state_object_requested_ = current_state;
+  private_prop.Set(v8_history, v8_state);
+  return ScriptValue(isolate, v8_state);
 }
 
 SerializedScriptValue* History::StateInternal() const {
@@ -156,10 +179,6 @@ HistoryScrollRestorationType History::ScrollRestorationInternal() const {
   return history_item->ScrollRestorationType();
 }
 
-bool History::stateChanged() const {
-  return last_state_object_requested_ != StateInternal();
-}
-
 bool History::IsSameAsCurrentState(SerializedScriptValue* state) const {
   return state == StateInternal();
 }

third_party/blink/renderer/core/frame/history.h

@@ -53,7 +53,7 @@ class CORE_EXPORT History final : public ScriptWrappable,
   explicit History(LocalFrame*);
 
   unsigned length(ExceptionState&) const;
-  ScriptValue state(v8::Isolate*, ExceptionState&);
+  ScriptValue state(ScriptState*, ExceptionState&);
 
   void back(ScriptState*, ExceptionState&);
   void forward(ScriptState*, ExceptionState&);
@@ -74,7 +74,6 @@ class CORE_EXPORT History final : public ScriptWrappable,
   void setScrollRestoration(const String& value, ExceptionState&);
   String scrollRestoration(ExceptionState&);
 
-  bool stateChanged() const;
   bool IsSameAsCurrentState(SerializedScriptValue*) const;
 
   void Trace(Visitor*) override;

third_party/blink/renderer/core/frame/history.idl

@@ -32,7 +32,7 @@ enum ScrollRestoration {"auto", "manual"};
 ] interface History {
     [MeasureAs=HistoryLength, RaisesException] readonly attribute unsigned long length;
     [Measure, RaisesException] attribute ScrollRestoration scrollRestoration;
-    [CachedAttribute=stateChanged, CallWith=Isolate, RaisesException] readonly attribute any state;
+    [CallWith=ScriptState, RaisesException] readonly attribute any state;
     [CallWith=ScriptState, RaisesException] void go(optional long delta = 0);
     [CallWith=ScriptState, RaisesException] void back();
     [CallWith=ScriptState, RaisesException] void forward();

third_party/blink/renderer/platform/bindings/v8_private_property.h

@@ -115,13 +115,6 @@ class PLATFORM_EXPORT V8PrivateProperty {
   static Symbol GetCachedAccessor(v8::Isolate* isolate,
                                   CachedAccessor symbol_id);
 
-  // This is a hack for PopStateEvent to get the same private property of
-  // History, named State.
-  static Symbol GetHistoryStateSymbol(v8::Isolate* isolate) {
-    static const SymbolKey kPrivatePropertyKey;
-    return GetSymbol(isolate, kPrivatePropertyKey);
-  }
-
   // Returns a Symbol to access a private property. Symbol instances from same
   // |key| are guaranteed to access the same property.
   static Symbol GetSymbol(v8::Isolate* isolate, const SymbolKey& key);

third_party/blink/web_tests/external/wpt/html/browsers/history/the-history-interface/001-expected.txt

@@ -36,7 +36,7 @@ PASS state data should cope with circular object references
 PASS state data should be a clone of the original object, not a reference to it
 PASS history.state should also reference a clone of the original object (2)
 PASS history.state should be a clone of the original object, not a reference to it (2)
-PASS history.state should be a separate clone of the object, not a reference to the object passed to the event handler
+FAIL history.state should be a separate clone of the object, not a reference to the object passed to the event handler assert_false: expected false got true
 PASS pushState should not actually load the new URL
 PASS reloading a pushed state should actually load the new URL
 Harness: the test ran to completion.

third_party/blink/web_tests/external/wpt/html/browsers/history/the-history-interface/002-expected.txt

+This is a testharness.js-based test.
+PASS history.length should update when loading pages in an iframe
+PASS history.length should update when setting location.hash
+PASS history.replaceState must exist
+PASS history.replaceState must exist within iframes
+PASS initial history.state should be null
+PASS history.length should not update when replacing a state with no URL
+PASS history.state should update after a state is pushed
+PASS hash should not change when replaceState is called without a URL
+PASS history.length should not update when replacing a state with a URL
+PASS hash should change when replaceState is called with a URL
+PASS replaceState must replace the existing state and not add an extra one
+PASS replaceState must replace the existing state without altering the forward history
+PASS replaceState must not be allowed to create invalid URLs
+PASS replaceState must not be allowed to create cross-origin URLs
+PASS replaceState must not be allowed to create cross-origin URLs (about:blank)
+PASS replaceState must not be allowed to create cross-origin URLs (data:URI)
+PASS security errors are expected to be thrown in the context of the document that owns the history object
+PASS replaceState must be able to set location.pathname
+PASS replaceState must be able to set absolute URLs to the same host
+PASS replaceState must not remove any tasks queued by the history traversal task source
+PASS .go must queue a task with the history traversal task source (run asynchronously)
+PASS replaceState must not fire hashchange events
+PASS replaceState must not be able to use a function as data
+PASS replaceState must not be able to use a DOM node as data
+PASS replaceState must be able to use an error object as data
+PASS security errors are expected to be thrown in the context of the document that owns the history object (2)
+PASS replaceState must be able to make structured clones of complex objects
+PASS history.state should also reference a clone of the original object
+PASS history.state should be a clone of the original object, not a reference to it
+PASS popstate event should fire when navigation occurs
+PASS popstate event should pass the state data
+PASS state data should cope with circular object references
+PASS state data should be a clone of the original object, not a reference to it
+PASS history.state should also reference a clone of the original object (2)
+PASS history.state should be a clone of the original object, not a reference to it (2)
+FAIL history.state should be a separate clone of the object, not a reference to the object passed to the event handler assert_false: expected false got true
+PASS replaceState should not actually load the new URL
+PASS reloading a replaced state should actually load the new URL
+Harness: the test ran to completion.
+