CSSStyleDeclaration: Stop calling ScriptState::ContextIsValid() in the setter

When the custom bindings were removed in commit 5b84e88a ("Remove custom bindings for CSSStyleDeclaration"), the new anonymous setter code added a check for ScriptState::ContextIsValid() to ensure it had received a valid |script_state|. It turns out this check is too strict, as in addition to verifying a ScriptState has an associated v8::Context it also asserts the ScriptState has a valid |per_context_data_|. This is valid in most cases, but when a node is moved across different documents and its previous document gets removed its |per_context_data_| is disposed of and ScriptState::ContextIsValid() fails. Since the anonymous setter is only invoked by the bindings layer, we can assume it is passed a ScriptState that is in a minimally usable state, so it is possible to relax the ContextIsValid() check and only make sure we can get a valid ExecutionContext from the ScriptState. Bug: 852190 Change-Id: I307de0e003e5258bf1f670b26f1dc86e4dae9286 Reviewed-on: https://chromium-review.googlesource.com/1100836 Commit-Queue: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Rune Lillesveen <futhark@chromium.org> Cr-Commit-Position: refs/heads/master@{#567310}

CSSStyleDeclaration: Stop calling ScriptState::ContextIsValid() in the setter
When the custom bindings were removed in commit 5b84e88a ("Remove custom bindings for CSSStyleDeclaration"), the new anonymous setter code added a check for ScriptState::ContextIsValid() to ensure it had received a valid |script_state|. It turns out this check is too strict, as in addition to verifying a ScriptState has an associated v8::Context it also asserts the ScriptState has a valid |per_context_data_|. This is valid in most cases, but when a node is moved across different documents and its previous document gets removed its |per_context_data_| is disposed of and ScriptState::ContextIsValid() fails. Since the anonymous setter is only invoked by the bindings layer, we can assume it is passed a ScriptState that is in a minimally usable state, so it is possible to relax the ContextIsValid() check and only make sure we can get a valid ExecutionContext from the ScriptState. Bug: 852190 Change-Id: I307de0e003e5258bf1f670b26f1dc86e4dae9286 Reviewed-on: https://chromium-review.googlesource.com/1100836 Commit-Queue: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Rune Lillesveen <futhark@chromium.org> Cr-Commit-Position: refs/heads/master@{#567310}
b179b0b8 · Raphael Kubo da Costa · Commit Bot · 0f9c1f1f · b179b0b8 · b179b0b8
Commit b179b0b8 authored Jun 14, 2018 by Raphael Kubo da Costa Committed by Commit Bot Jun 14, 2018
2 changed files
--- a/third_party/WebKit/LayoutTests/external/wpt/css/cssom/style-attr-update-across-documents.html
+++ b/third_party/WebKit/LayoutTests/external/wpt/css/cssom/style-attr-update-across-documents.html
+<!DOCTYPE html>
+<title>CSSStyleDeclaration setter works when a node changes document</title>
+<link rel="author" title="Raphael Kubo da Costa" href="raphael.kubo.da.costa@intel.com">
+<link rel="help" href="https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-camel-cased-attribute">
+<link rel="help" href="https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-webkit-cased-attribute">
+<link rel="help" href="https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-dashed-attribute">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<div id="original-div" style="background-color:green; height: 100px; width: 100px"></div>
+<script>
+  // Create and return a 100x100 red <div>.
+  function createDiv(containerDocument) {
+    const div = containerDocument.createElement("div");
+    div.style.backgroundColor = "red";
+    div.style.height = "100px";
+    div.style.width = "100px";
+    containerDocument.body.appendChild(div);
+    return div;
+  }
+
+  async_test(t => {
+    const iframeElement = document.createElement("iframe");
+    iframeElement.addEventListener("load", t.step_func_done(() => {
+      const originalDiv = document.getElementById("original-div");
+      const newDiv = createDiv(iframeElement.contentDocument);
+
+      assert_not_equals(newDiv.ownerDocument, document,
+                        "The new <div> belongs to the iframe, not the main document");
+
+      document.body.insertBefore(newDiv, originalDiv);
+      assert_equals(newDiv.ownerDocument, document,
+                    "The new <div> now belongs to the main document");
+
+      newDiv.style.backgroundColor = "blue";
+      assert_equals(window.getComputedStyle(newDiv).getPropertyValue("background-color"),
+                    "rgb(0, 0, 255)",
+                    "The new <div>'s background-color is blue");
+
+      document.body.removeChild(iframeElement);
+
+      newDiv.style.backgroundColor = "green";
+      assert_equals(window.getComputedStyle(newDiv).getPropertyValue("background-color"),
+                    "rgb(0, 128, 0)",
+                    "The new <div>'s background-color is green");
+    }));
+    document.body.appendChild(iframeElement);
+  }, "Changing the style of a node that switched documents works");
+</script>
--- a/third_party/blink/renderer/core/css/css_style_declaration.cc
+++ b/third_party/blink/renderer/core/css/css_style_declaration.cc
@@ -162,9 +162,10 @@ String CSSStyleDeclaration::AnonymousNamedGetter(const AtomicString& name) {
 bool CSSStyleDeclaration::AnonymousNamedSetter(ScriptState* script_state,
                                               const AtomicString& name,
                                               const String& value) {
-  if (!script_state->ContextIsValid())
+  const ExecutionContext* execution_context =
+      ExecutionContext::From(script_state);
+  if (!execution_context)
    return false;
-
  CSSPropertyID unresolved_property = CssPropertyInfo(name);
  if (!unresolved_property)
    return false;
@@ -177,10 +178,9 @@ bool CSSStyleDeclaration::AnonymousNamedSetter(ScriptState* script_state,
      "CSSStyleDeclaration",
      CSSProperty::Get(resolveCSSPropertyID(unresolved_property))
          .GetPropertyName());
-  SetPropertyInternal(
-      unresolved_property, String(), value, false,
-      ExecutionContext::From(script_state)->GetSecureContextMode(),
-      exception_state);
+  SetPropertyInternal(unresolved_property, String(), value, false,
+                      execution_context->GetSecureContextMode(),
+                      exception_state);
  if (exception_state.HadException())
    return false;
  return true;