Commit 349a31e4 authored by Xida Chen's avatar Xida Chen Committed by Commit Bot

Make PaintWorkletInput take a WTF::HashMap

Right the PaintWorkletInput takes a PaintWorkletStylePropertyMap which
is GC-ed, this doesn't seem to be thread-safe considering that the
style map will eventually be passed to v8, to execute the JS paint
callback.

The solution here is to make PaintWorkletInput take a HashMap, which is
the underline data structure of the PaintWorkletStylePropertyMap.
Once we pass the PaintWorkletInpt to the worklet thread, we will build
a new instance of PaintWorkletStylePropertyMap from the HashMap and
give that instance to v8.

Bug: 895579
Change-Id: I35cc7667fb3fc257eb2306313056bd3ec31c51f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1521027
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: default avatarAnders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: default avatarStephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#644863}
parent cbf6ea27
......@@ -19,4 +19,10 @@ bool CrossThreadKeywordValue::operator==(
return false;
}
std::unique_ptr<CrossThreadStyleValue> CrossThreadKeywordValue::IsolatedCopy()
const {
return std::make_unique<CrossThreadKeywordValue>(
keyword_value_.IsolatedCopy());
}
} // namespace blink
......@@ -26,6 +26,8 @@ class CORE_EXPORT CrossThreadKeywordValue final : public CrossThreadStyleValue {
}
CSSStyleValue* ToCSSStyleValue() override;
std::unique_ptr<CrossThreadStyleValue> IsolatedCopy() const override;
bool operator==(const CrossThreadStyleValue&) const override;
private:
......
......@@ -5,6 +5,8 @@
#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSSOM_CROSS_THREAD_STYLE_VALUE_H_
#define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSSOM_CROSS_THREAD_STYLE_VALUE_H_
#include <memory>
#include "base/macros.h"
#include "third_party/blink/renderer/core/core_export.h"
......@@ -26,6 +28,7 @@ class CORE_EXPORT CrossThreadStyleValue {
virtual StyleValueType GetType() const = 0;
virtual CSSStyleValue* ToCSSStyleValue() = 0;
virtual std::unique_ptr<CrossThreadStyleValue> IsolatedCopy() const = 0;
virtual bool operator==(const CrossThreadStyleValue&) const = 0;
......
......@@ -19,4 +19,9 @@ bool CrossThreadUnitValue::operator==(
return false;
}
std::unique_ptr<CrossThreadStyleValue> CrossThreadUnitValue::IsolatedCopy()
const {
return std::make_unique<CrossThreadUnitValue>(value_, unit_);
}
} // namespace blink
......@@ -23,6 +23,7 @@ class CORE_EXPORT CrossThreadUnitValue final : public CrossThreadStyleValue {
StyleValueType GetType() const override { return StyleValueType::kUnitType; }
CSSStyleValue* ToCSSStyleValue() override;
std::unique_ptr<CrossThreadStyleValue> IsolatedCopy() const override;
bool operator==(const CrossThreadStyleValue&) const override;
......
......@@ -19,4 +19,9 @@ bool CrossThreadUnsupportedValue::operator==(
return false;
}
std::unique_ptr<CrossThreadStyleValue>
CrossThreadUnsupportedValue::IsolatedCopy() const {
return std::make_unique<CrossThreadUnsupportedValue>(value_.IsolatedCopy());
}
} // namespace blink
......@@ -25,6 +25,7 @@ class CORE_EXPORT CrossThreadUnsupportedValue final
return StyleValueType::kUnknownType;
}
CSSStyleValue* ToCSSStyleValue() override;
std::unique_ptr<CrossThreadStyleValue> IsolatedCopy() const override;
bool operator==(const CrossThreadStyleValue&) const override;
......
......@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include <utility>
#include "third_party/blink/renderer/core/css/cssom/paint_worklet_input.h"
namespace blink {
......@@ -10,10 +12,10 @@ PaintWorkletInput::PaintWorkletInput(
const std::string& name,
const FloatSize& container_size,
float effective_zoom,
CrossThreadPersistent<PaintWorkletStylePropertyMap> style_map)
PaintWorkletStylePropertyMap::CrossThreadData data)
: name_(name),
container_size_(container_size),
effective_zoom_(effective_zoom),
style_map_(style_map) {}
style_map_data_(std::move(data)) {}
} // namespace blink
......@@ -6,11 +6,12 @@
#define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSSOM_PAINT_WORKLET_INPUT_H_
#include <memory>
#include <utility>
#include "cc/paint/paint_worklet_input.h"
#include "third_party/blink/renderer/core/core_export.h"
#include "third_party/blink/renderer/core/css/cssom/paint_worklet_style_property_map.h"
#include "third_party/blink/renderer/platform/geometry/float_size.h"
#include "third_party/blink/renderer/platform/heap/persistent.h"
namespace blink {
......@@ -19,7 +20,7 @@ class CORE_EXPORT PaintWorkletInput : public cc::PaintWorkletInput {
PaintWorkletInput(const std::string& name,
const FloatSize& container_size,
float effective_zoom,
CrossThreadPersistent<PaintWorkletStylePropertyMap>);
PaintWorkletStylePropertyMap::CrossThreadData values);
~PaintWorkletInput() override = default;
......@@ -31,15 +32,15 @@ class CORE_EXPORT PaintWorkletInput : public cc::PaintWorkletInput {
const std::string& Name() const { return name_; }
const FloatSize& ContainerSize() const { return container_size_; }
float EffectiveZoom() const { return effective_zoom_; }
const PaintWorkletStylePropertyMap* StyleMap() const {
return style_map_.Get();
PaintWorkletStylePropertyMap::CrossThreadData StyleMapData() {
return PaintWorkletStylePropertyMap::CopyCrossThreadData(style_map_data_);
}
private:
const std::string name_;
const FloatSize container_size_;
const float effective_zoom_;
const CrossThreadPersistent<PaintWorkletStylePropertyMap> style_map_;
PaintWorkletStylePropertyMap::CrossThreadData style_map_data_;
};
} // namespace blink
......
......@@ -2,11 +2,17 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include <memory>
#include <utility>
#include "third_party/blink/renderer/core/css/cssom/paint_worklet_style_property_map.h"
#include "third_party/blink/renderer/core/css/css_custom_property_declaration.h"
#include "third_party/blink/renderer/core/css/css_variable_data.h"
#include "third_party/blink/renderer/core/css/cssom/computed_style_property_map.h"
#include "third_party/blink/renderer/core/css/cssom/cross_thread_keyword_value.h"
#include "third_party/blink/renderer/core/css/cssom/cross_thread_unit_value.h"
#include "third_party/blink/renderer/core/css/cssom/cross_thread_unsupported_value.h"
#include "third_party/blink/renderer/core/css/cssom/css_unparsed_value.h"
#include "third_party/blink/renderer/core/css/cssom/css_unsupported_style_value.h"
#include "third_party/blink/renderer/core/css/properties/css_property_ref.h"
......@@ -48,26 +54,10 @@ class PaintWorkletStylePropertyMapIterationSource final
const HeapVector<PaintWorkletStylePropertyMap::StylePropertyMapEntry> values_;
};
} // namespace
PaintWorkletStylePropertyMap::PaintWorkletStylePropertyMap(
const Document& document,
const ComputedStyle& style,
Node* styled_node,
const Vector<CSSPropertyID>& native_properties,
const Vector<AtomicString>& custom_properties)
: StylePropertyMapReadOnly() {
DCHECK(IsMainThread());
values_.ReserveCapacityForSize(native_properties.size() +
custom_properties.size());
BuildNativeValues(style, styled_node, native_properties);
BuildCustomValues(document, style, styled_node, custom_properties);
}
void PaintWorkletStylePropertyMap::BuildNativeValues(
const ComputedStyle& style,
Node* styled_node,
const Vector<CSSPropertyID>& native_properties) {
void BuildNativeValues(const ComputedStyle& style,
Node* styled_node,
const Vector<CSSPropertyID>& native_properties,
PaintWorkletStylePropertyMap::CrossThreadData& data) {
DCHECK(IsMainThread());
for (const auto& property_id : native_properties) {
// Silently drop shorthand properties.
......@@ -83,15 +73,15 @@ void PaintWorkletStylePropertyMap::BuildNativeValues(
String key = CSSProperty::Get(property_id).GetPropertyNameString();
if (!key.IsSafeToSendToAnotherThread())
key = key.IsolatedCopy();
values_.Set(key, std::move(value));
data.Set(key, std::move(value));
}
}
void PaintWorkletStylePropertyMap::BuildCustomValues(
const Document& document,
const ComputedStyle& style,
Node* styled_node,
const Vector<AtomicString>& custom_properties) {
void BuildCustomValues(const Document& document,
const ComputedStyle& style,
Node* styled_node,
const Vector<AtomicString>& custom_properties,
PaintWorkletStylePropertyMap::CrossThreadData& data) {
DCHECK(IsMainThread());
for (const auto& property_name : custom_properties) {
CSSPropertyRef ref(property_name, document);
......@@ -103,10 +93,45 @@ void PaintWorkletStylePropertyMap::BuildCustomValues(
String key = property_name.GetString();
if (!key.IsSafeToSendToAnotherThread())
key = key.IsolatedCopy();
values_.Set(key, std::move(value));
data.Set(key, std::move(value));
}
}
} // namespace
PaintWorkletStylePropertyMap::CrossThreadData
PaintWorkletStylePropertyMap::BuildCrossThreadData(
const Document& document,
const ComputedStyle& style,
Node* styled_node,
const Vector<CSSPropertyID>& native_properties,
const Vector<AtomicString>& custom_properties) {
DCHECK(IsMainThread());
PaintWorkletStylePropertyMap::CrossThreadData data;
data.ReserveCapacityForSize(native_properties.size() +
custom_properties.size());
BuildNativeValues(style, styled_node, native_properties, data);
BuildCustomValues(document, style, styled_node, custom_properties, data);
return data;
}
PaintWorkletStylePropertyMap::CrossThreadData
PaintWorkletStylePropertyMap::CopyCrossThreadData(const CrossThreadData& data) {
PaintWorkletStylePropertyMap::CrossThreadData copied_data;
copied_data.ReserveCapacityForSize(data.size());
for (auto& pair : data)
copied_data.Set(pair.key.IsolatedCopy(), pair.value->IsolatedCopy());
return copied_data;
}
// The |data| comes from PaintWorkletInput, where its string is already an
// isolated copy from the main thread string, so we don't need to make another
// isolated copy here.
PaintWorkletStylePropertyMap::PaintWorkletStylePropertyMap(CrossThreadData data)
: data_(std::move(data)) {
DCHECK(!IsMainThread());
}
CSSStyleValue* PaintWorkletStylePropertyMap::get(
const ExecutionContext* execution_context,
const String& property_name,
......@@ -129,8 +154,8 @@ CSSStyleValueVector PaintWorkletStylePropertyMap::getAll(
DCHECK(isValidCSSPropertyID(property_id));
CSSStyleValueVector values;
auto value = values_.find(property_name);
if (value == values_.end())
auto value = data_.find(property_name);
if (value == data_.end())
return CSSStyleValueVector();
values.push_back(value->value->ToCSSStyleValue());
return values;
......@@ -144,7 +169,7 @@ bool PaintWorkletStylePropertyMap::has(
}
unsigned PaintWorkletStylePropertyMap::size() const {
return values_.size();
return data_.size();
}
PaintWorkletStylePropertyMap::IterationSource*
......
......@@ -5,6 +5,8 @@
#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSSOM_PAINT_WORKLET_STYLE_PROPERTY_MAP_H_
#define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSSOM_PAINT_WORKLET_STYLE_PROPERTY_MAP_H_
#include <memory>
#include "base/macros.h"
#include "third_party/blink/renderer/core/css/cssom/cross_thread_style_value.h"
#include "third_party/blink/renderer/core/css/cssom/style_property_map_read_only.h"
......@@ -16,20 +18,30 @@ namespace blink {
// threads.
//
// Here is a typical usage.
// At CSSPaintValue::GetImage which is on the main thread, build an instance
// of Blink::PaintWorkletInput which calls the constructor of this class. The
// ownership of this style map belongs to the Blink::PaintWorkletInput, which
// will eventually be passed to the worklet thread and used in the JS callback.
// At CSSPaintValue::GetImage which is on the main thread, call the
// BuildCrossThreadData and give the data to the Blink::PaintWorkletInput.
// The PaintWorkletInput is passed to the worklet thread, and we build an
// instance of PaintWorkletStylePropertyMap from the data, and the instance is
// eventually pass to the JS paint callback.
class CORE_EXPORT PaintWorkletStylePropertyMap
: public StylePropertyMapReadOnly {
public:
using StylePropertyMapEntry = std::pair<String, CSSStyleValueVector>;
// This constructor should be called on main-thread only.
PaintWorkletStylePropertyMap(const Document&,
const ComputedStyle&,
Node* styled_node,
const Vector<CSSPropertyID>& native_properties,
const Vector<AtomicString>& custom_properties);
using CrossThreadData =
HashMap<String, std::unique_ptr<CrossThreadStyleValue>>;
// Build the data that will be passed to the worklet thread to construct a
// style map. Should be called on the main thread only.
static CrossThreadData BuildCrossThreadData(
const Document&,
const ComputedStyle&,
Node* styled_node,
const Vector<CSSPropertyID>& native_properties,
const Vector<AtomicString>& custom_properties);
static CrossThreadData CopyCrossThreadData(const CrossThreadData& data);
// This constructor should be called on the worklet-thread only.
explicit PaintWorkletStylePropertyMap(CrossThreadData data);
CSSStyleValue* get(const ExecutionContext*,
const String& property_name,
......@@ -47,23 +59,12 @@ class CORE_EXPORT PaintWorkletStylePropertyMap
void Trace(blink::Visitor*) override;
const HashMap<String, std::unique_ptr<CrossThreadStyleValue>>& ValuesForTest()
const {
return values_;
}
const CrossThreadData& StyleMapDataForTest() const { return data_; }
private:
IterationSource* StartIteration(ScriptState*, ExceptionState&) override;
void BuildNativeValues(const ComputedStyle&,
Node* styled_node,
const Vector<CSSPropertyID>& native_properties);
void BuildCustomValues(const Document&,
const ComputedStyle&,
Node* styled_node,
const Vector<AtomicString>& custom_properties);
HashMap<String, std::unique_ptr<CrossThreadStyleValue>> values_;
CrossThreadData data_;
DISALLOW_COPY_AND_ASSIGN(PaintWorkletStylePropertyMap);
};
......
......@@ -102,48 +102,48 @@ class PaintWorkletStylePropertyMapTest : public PageTestBase {
DCHECK(!IsMainThread());
thread_->InitializeOnThread();
const PaintWorkletStylePropertyMap* map = input->StyleMap();
PaintWorkletStylePropertyMap* map =
MakeGarbageCollected<PaintWorkletStylePropertyMap>(
input->StyleMapData());
DCHECK(map);
DummyExceptionStateForTesting exception_state;
CheckNativeProperties(map, exception_state);
CheckCustomProperties(map, exception_state);
const HashMap<String, std::unique_ptr<CrossThreadStyleValue>>& values =
map->ValuesForTest();
EXPECT_EQ(values.size(), 8u);
EXPECT_EQ(values.at("color")->ToCSSStyleValue()->CSSText(),
"rgb(0, 255, 0)");
EXPECT_EQ(values.at("color")->ToCSSStyleValue()->GetType(),
const PaintWorkletStylePropertyMap::CrossThreadData& data =
map->StyleMapDataForTest();
EXPECT_EQ(data.size(), 8u);
EXPECT_EQ(data.at("color")->ToCSSStyleValue()->CSSText(), "rgb(0, 255, 0)");
EXPECT_EQ(data.at("color")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kUnknownType);
EXPECT_EQ(values.at("align-items")->ToCSSStyleValue()->CSSText(), "normal");
EXPECT_EQ(values.at("align-items")->ToCSSStyleValue()->GetType(),
EXPECT_EQ(data.at("align-items")->ToCSSStyleValue()->CSSText(), "normal");
EXPECT_EQ(data.at("align-items")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kUnknownType);
EXPECT_EQ(
static_cast<CSSKeywordValue*>(values.at("display")->ToCSSStyleValue())
static_cast<CSSKeywordValue*>(data.at("display")->ToCSSStyleValue())
->value(),
"block");
EXPECT_EQ(values.at("display")->ToCSSStyleValue()->GetType(),
EXPECT_EQ(data.at("display")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kKeywordType);
EXPECT_EQ(values.at("--foo")->ToCSSStyleValue()->CSSText(), "PaintWorklet");
EXPECT_EQ(values.at("--foo")->ToCSSStyleValue()->GetType(),
EXPECT_EQ(data.at("--foo")->ToCSSStyleValue()->CSSText(), "PaintWorklet");
EXPECT_EQ(data.at("--foo")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kUnknownType);
EXPECT_EQ(values.at("--bar")->ToCSSStyleValue()->CSSText(), "");
EXPECT_EQ(values.at("--bar")->ToCSSStyleValue()->GetType(),
EXPECT_EQ(data.at("--bar")->ToCSSStyleValue()->CSSText(), "");
EXPECT_EQ(data.at("--bar")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kUnknownType);
EXPECT_EQ(values.at("--keyword")->ToCSSStyleValue()->GetType(),
EXPECT_EQ(data.at("--keyword")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kKeywordType);
EXPECT_EQ(
To<CSSKeywordValue>(values.at("--keyword")->ToCSSStyleValue())->value(),
To<CSSKeywordValue>(data.at("--keyword")->ToCSSStyleValue())->value(),
"test");
EXPECT_EQ(values.at("--x")->ToCSSStyleValue()->GetType(),
EXPECT_EQ(data.at("--x")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kUnitType);
EXPECT_EQ(To<CSSUnitValue>(values.at("--x")->ToCSSStyleValue())->value(),
10);
EXPECT_EQ(To<CSSUnitValue>(values.at("--x")->ToCSSStyleValue())->unit(),
EXPECT_EQ(To<CSSUnitValue>(data.at("--x")->ToCSSStyleValue())->value(), 10);
EXPECT_EQ(To<CSSUnitValue>(data.at("--x")->ToCSSStyleValue())->unit(),
"px");
EXPECT_EQ(values.at("--y")->ToCSSStyleValue()->GetType(),
EXPECT_EQ(data.at("--y")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kUnknownType);
EXPECT_EQ(values.at("--y")->ToCSSStyleValue()->CSSText(), "rgb(0, 255, 0)");
EXPECT_EQ(data.at("--y")->ToCSSStyleValue()->CSSText(), "rgb(0, 255, 0)");
waitable_event->Signal();
}
......@@ -152,64 +152,6 @@ class PaintWorkletStylePropertyMapTest : public PageTestBase {
std::unique_ptr<WebThreadSupportingGC> thread_;
};
TEST_F(PaintWorkletStylePropertyMapTest, NativePropertyAccessors) {
Vector<CSSPropertyID> native_properties({CSSPropertyID::kColor,
CSSPropertyID::kAlignItems,
CSSPropertyID::kBackground});
Vector<AtomicString> empty_custom_properties;
GetDocument().documentElement()->style()->setProperty(
&GetDocument(), "color", "rgb(0, 255, 0)", "", ASSERT_NO_EXCEPTION);
UpdateAllLifecyclePhasesForTest();
Node* node = PageNode();
const PaintWorkletStylePropertyMap* map =
MakeGarbageCollected<PaintWorkletStylePropertyMap>(
GetDocument(), node->ComputedStyleRef(), node, native_properties,
empty_custom_properties);
const HashMap<String, std::unique_ptr<CrossThreadStyleValue>>& values =
map->ValuesForTest();
EXPECT_EQ(values.size(), 2u);
EXPECT_EQ(values.at("color")->ToCSSStyleValue()->CSSText(), "rgb(0, 255, 0)");
EXPECT_EQ(values.at("color")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kUnknownType);
EXPECT_EQ(values.at("align-items")->ToCSSStyleValue()->CSSText(), "normal");
EXPECT_EQ(values.at("align-items")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kUnknownType);
DummyExceptionStateForTesting exception_state;
CheckNativeProperties(map, exception_state);
}
TEST_F(PaintWorkletStylePropertyMapTest, CustomPropertyAccessors) {
Vector<CSSPropertyID> empty_native_properties;
Vector<AtomicString> custom_properties({"--foo", "--bar"});
GetDocument().documentElement()->style()->setProperty(
&GetDocument(), "--foo", "PaintWorklet", "", ASSERT_NO_EXCEPTION);
UpdateAllLifecyclePhasesForTest();
Node* node = PageNode();
const PaintWorkletStylePropertyMap* map =
MakeGarbageCollected<PaintWorkletStylePropertyMap>(
GetDocument(), node->ComputedStyleRef(), node,
empty_native_properties, custom_properties);
const HashMap<String, std::unique_ptr<CrossThreadStyleValue>>& values =
map->ValuesForTest();
EXPECT_EQ(values.size(), 2u);
EXPECT_EQ(values.at("--foo")->ToCSSStyleValue()->CSSText(), "PaintWorklet");
EXPECT_EQ(values.at("--foo")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kUnknownType);
EXPECT_EQ(values.at("--bar")->ToCSSStyleValue()->CSSText(), "");
EXPECT_EQ(values.at("--bar")->ToCSSStyleValue()->GetType(),
CSSStyleValue::StyleValueType::kUnknownType);
DummyExceptionStateForTesting exception_state;
CheckCustomProperties(map, exception_state);
}
// This test ensures that Blink::PaintWorkletInput can be safely passed cross
// threads and no information is lost.
TEST_F(PaintWorkletStylePropertyMapTest, PassValuesCrossThread) {
......@@ -240,13 +182,13 @@ TEST_F(PaintWorkletStylePropertyMapTest, PassValuesCrossThread) {
UpdateAllLifecyclePhasesForTest();
Node* node = PageNode();
CrossThreadPersistent<PaintWorkletStylePropertyMap> style_map =
MakeGarbageCollected<PaintWorkletStylePropertyMap>(
PaintWorkletStylePropertyMap::CrossThreadData data =
PaintWorkletStylePropertyMap::BuildCrossThreadData(
GetDocument(), node->ComputedStyleRef(), node, native_properties,
custom_properties);
scoped_refptr<PaintWorkletInput> input =
base::MakeRefCounted<PaintWorkletInput>("test", FloatSize(100, 100), 1.0f,
style_map);
std::move(data));
DCHECK(input);
thread_ = std::make_unique<WebThreadSupportingGC>(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment