Skip to content

Commit

Permalink
Revert "[parallel-text-shaping] CSS changes"
Browse files Browse the repository at this point in the history 
This reverts commit 1f36b64dd81cc0c4f4443598fb09307b78c7ac84.

Reason for revert: Caused a nullptr deref crbug.com/1335246#c23

Original change's description:
> [parallel-text-shaping] CSS changes
>
> This patch changes "core/css", "platform/{fonts,testing}" to make font
> loading, both system and web, to work with font threads for prepration
> of parallel text shaping[1].
>
> This patch is split from the CL[2][3] to exclude `CSSSegmentedFontFace`
> and `CSSFontFace` changes to isolate memory related issues
>
>
> Note: `USE_PARALLEL_TEXT_SHAPING` will be defined on Win and MacOS.
>
> core/css
>   * Make objects thread-safe. Mostly adding locks.
>   * Utilize `ExecutionContext::GetTaskRunner()` to call
>     `FontMatchingMetrics` in owner execution context from font
>     threads.
>
> platform/fonts/font_selector.{cc,h}
>   * Make `SimpleFontData` parameter in reporting function can be cross
>     thread.
>
> platform/fonts/font_test_helper.cc
>   * Adapt for `FontSelector` changes.
>
> [1] http://crrev.com/c/3211416 Parallel Text Shaping implementation
> [2] http://crrev.com/c/3670315: [parallel-text-shaping] CSS changes
> [3] http://crrev.com/c/3703810: [parallel-text-shaping] Use
> Persistent<FontFaceList> in CSSSegmentedFontFace
>
> Design Doc: https://bit.ly/3GqpJLD
> Font slid: https://bit.ly/3JFTSHa
>
> Bug: 1264280
> Change-Id: Ia1dbb155a6cd7d653cd2b52e03e1945db823bf42
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3708289
> Auto-Submit: Yoshifumi Inoue <yosin@chromium.org>
> Reviewed-by: Kent Tamura <tkent@chromium.org>
> Commit-Queue: Kent Tamura <tkent@chromium.org>
> Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1015233}

Bug: 1264280, 1335246
Change-Id: I80943ab56c6c86234f857f392e95edfb9e775403
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3748643
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1021325}
NOKEYCHECK=True
GitOrigin-RevId: a2f82f37ee3c62c899778e18169ae00f0582c365
  • Loading branch information
@xiaochengh @copybara-github
xiaochengh authored and copybara-github committed Jul 6, 2022
1 parent 2c4da8a commit 42e100c
Show file tree
Hide file tree
Showing 12 changed files with 53 additions and 318 deletions.
6 changes: 0 additions & 6 deletions blink/renderer/core/css/css_custom_font_data.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,9 @@ class CSSCustomFontData final : public CustomFontData {

// TODO(Oilpan): consider moving (Custom)FontFace hierarchy to the heap,
// thereby making this reference a Member<>.
#if defined(USE_PARALLEL_TEXT_SHAPING)
CrossThreadWeakPersistent<CSSFontFaceSource> font_face_source_;
std::atomic<FallbackVisibility> fallback_visibility_;
mutable std::atomic<bool> is_loading_{false};
#else
WeakPersistent<CSSFontFaceSource> font_face_source_;
FallbackVisibility fallback_visibility_;
mutable bool is_loading_ = false;
#endif
};

} // namespace blink
Expand Down
15 changes: 3 additions & 12 deletions blink/renderer/core/css/css_font_selector.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,7 @@
namespace blink {

CSSFontSelector::CSSFontSelector(const TreeScope& tree_scope)
: CSSFontSelectorBase(
tree_scope.GetDocument().GetExecutionContext()->GetTaskRunner(
TaskType::kInternalDefault)),
tree_scope_(&tree_scope) {
DCHECK(tree_scope.GetDocument().GetExecutionContext()->IsContextThread());
: tree_scope_(&tree_scope) {
DCHECK(tree_scope.GetDocument().GetFrame());
generic_font_family_settings_ = tree_scope.GetDocument()
.GetFrame()
Expand All @@ -61,8 +57,7 @@ CSSFontSelector::CSSFontSelector(const TreeScope& tree_scope)
CSSFontSelector::~CSSFontSelector() = default;

UseCounter* CSSFontSelector::GetUseCounter() const {
auto* const context = GetExecutionContext();
return context && context->IsContextThread() ? context : nullptr;
return GetExecutionContext();
}

void CSSFontSelector::RegisterForInvalidationCallbacks(
Expand Down Expand Up @@ -153,7 +148,7 @@ scoped_refptr<FontData> CSSFontSelector::GetFontData(
FontCache::Get().GetFontData(request_description, settings_family_name);

ReportFontLookupByUniqueOrFamilyName(settings_family_name,
request_description, font_data);
request_description, font_data.get());

return font_data;
}
Expand All @@ -170,10 +165,6 @@ FontMatchingMetrics* CSSFontSelector::GetFontMatchingMetrics() const {
return GetDocument().GetFontMatchingMetrics();
}

bool CSSFontSelector::IsAlive() const {
return tree_scope_;
}

void CSSFontSelector::Trace(Visitor* visitor) const {
visitor->Trace(tree_scope_);
visitor->Trace(clients_);
Expand Down
1 change: 0 additions & 1 deletion blink/renderer/core/css/css_font_selector.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ class CORE_EXPORT CSSFontSelector : public CSSFontSelectorBase {
void DispatchInvalidationCallbacks(FontInvalidationReason);

// `CSSFontSelectorBase` overrides
bool IsAlive() const override;
FontMatchingMetrics* GetFontMatchingMetrics() const override;
UseCounter* GetUseCounter() const override;

Expand Down
218 changes: 14 additions & 204 deletions blink/renderer/core/css/css_font_selector_base.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,58 +16,17 @@

namespace blink {

CSSFontSelectorBase::CSSFontSelectorBase(
scoped_refptr<base::SingleThreadTaskRunner> task_runner)
#if defined(USE_PARALLEL_TEXT_SHAPING)
: task_runner_(task_runner)
#endif
{
DCHECK(IsContextThread());
}

void CSSFontSelectorBase::CountUse(WebFeature feature) const {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (IsContextThread())
return UseCounter::Count(GetUseCounter(), feature);
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(&CSSFontSelectorBase::CountUse,
WrapCrossThreadPersistent(this), feature));
#endif
return UseCounter::Count(GetUseCounter(), feature);
}

AtomicString CSSFontSelectorBase::FamilyNameFromSettings(
const FontDescription& font_description,
const FontFamily& generic_family_name) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsContextThread()) {
if (IsWebkitBodyFamily(font_description)) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(
&CSSFontSelectorBase::CountUse, WrapCrossThreadPersistent(this),
WebFeature::kFontSelectorCSSFontFamilyWebKitPrefixBody));
}
return FontSelector::FamilyNameFromSettings(generic_font_family_settings_,
font_description,
generic_family_name, nullptr);
}
#endif
return FontSelector::FamilyNameFromSettings(
generic_font_family_settings_, font_description, generic_family_name,
GetUseCounter());
}

bool CSSFontSelectorBase::IsContextThread() const {
#if defined(USE_PARALLEL_TEXT_SHAPING)
return task_runner_->RunsTasksInCurrentSequence();
#else
return true;
#endif
}

bool CSSFontSelectorBase::IsPlatformFamilyMatchAvailable(
const FontDescription& font_description,
const FontFamily& passed_family) {
Expand All @@ -81,19 +40,6 @@ bool CSSFontSelectorBase::IsPlatformFamilyMatchAvailable(
void CSSFontSelectorBase::ReportEmojiSegmentGlyphCoverage(
unsigned num_clusters,
unsigned num_broken_clusters) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(
&CSSFontSelectorBase::ReportEmojiSegmentGlyphCoverage,
WrapCrossThreadPersistent(this), num_clusters,
num_broken_clusters));
return;
}
#endif
GetFontMatchingMetrics()->ReportEmojiSegmentGlyphCoverage(
num_clusters, num_broken_clusters);
}
Expand All @@ -103,173 +49,63 @@ void CSSFontSelectorBase::ReportFontFamilyLookupByGenericFamily(
UScriptCode script,
FontDescription::GenericFamilyType generic_family_type,
const AtomicString& resulting_font_name) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(
&CSSFontSelectorBase::ReportFontFamilyLookupByGenericFamily,
WrapCrossThreadPersistent(this), generic_font_family_name, script,
generic_family_type, resulting_font_name));
return;
}
#endif
GetFontMatchingMetrics()->ReportFontFamilyLookupByGenericFamily(
generic_font_family_name, script, generic_family_type,
resulting_font_name);
}

void CSSFontSelectorBase::ReportSuccessfulFontFamilyMatch(
const AtomicString& font_family_name) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(&CSSFontSelectorBase::ReportFailedFontFamilyMatch,
WrapCrossThreadPersistent(this), font_family_name));
return;
}
#endif
GetFontMatchingMetrics()->ReportSuccessfulFontFamilyMatch(font_family_name);
}

void CSSFontSelectorBase::ReportFailedFontFamilyMatch(
const AtomicString& font_family_name) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(&CSSFontSelectorBase::ReportFailedFontFamilyMatch,
WrapCrossThreadPersistent(this), font_family_name));
return;
}
#endif
GetFontMatchingMetrics()->ReportFailedFontFamilyMatch(font_family_name);
}

void CSSFontSelectorBase::ReportSuccessfulLocalFontMatch(
const AtomicString& font_name) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(
&CSSFontSelectorBase::ReportSuccessfulLocalFontMatch,
WrapCrossThreadPersistent(this), font_name));
return;
}
#endif
GetFontMatchingMetrics()->ReportSuccessfulLocalFontMatch(font_name);
}

void CSSFontSelectorBase::ReportFailedLocalFontMatch(
const AtomicString& font_name) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(&CSSFontSelectorBase::ReportFailedLocalFontMatch,
WrapCrossThreadPersistent(this), font_name));
return;
}
#endif
GetFontMatchingMetrics()->ReportFailedLocalFontMatch(font_name);
}

void CSSFontSelectorBase::ReportFontLookupByUniqueOrFamilyName(
const AtomicString& name,
const FontDescription& font_description,
scoped_refptr<SimpleFontData> resulting_font_data) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(
&CSSFontSelectorBase::ReportFontLookupByUniqueOrFamilyName,
WrapCrossThreadPersistent(this), name, font_description,
resulting_font_data));
return;
}
#endif
SimpleFontData* resulting_font_data) {
GetFontMatchingMetrics()->ReportFontLookupByUniqueOrFamilyName(
name, font_description, resulting_font_data.get());
name, font_description, resulting_font_data);
}

void CSSFontSelectorBase::ReportFontLookupByUniqueNameOnly(
const AtomicString& name,
const FontDescription& font_description,
scoped_refptr<SimpleFontData> resulting_font_data,
SimpleFontData* resulting_font_data,
bool is_loading_fallback) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(
&CSSFontSelectorBase::ReportFontLookupByUniqueNameOnly,
WrapCrossThreadPersistent(this), name, font_description,
resulting_font_data, is_loading_fallback));
return;
}
#endif
GetFontMatchingMetrics()->ReportFontLookupByUniqueNameOnly(
name, font_description, resulting_font_data.get(), is_loading_fallback);
name, font_description, resulting_font_data, is_loading_fallback);
}

void CSSFontSelectorBase::ReportFontLookupByFallbackCharacter(
UChar32 fallback_character,
FontFallbackPriority fallback_priority,
const FontDescription& font_description,
scoped_refptr<SimpleFontData> resulting_font_data) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(
&CSSFontSelectorBase::ReportFontLookupByFallbackCharacter,
WrapCrossThreadPersistent(this), fallback_character,
fallback_priority, font_description, resulting_font_data));
return;
}
#endif
SimpleFontData* resulting_font_data) {
GetFontMatchingMetrics()->ReportFontLookupByFallbackCharacter(
fallback_character, fallback_priority, font_description,
resulting_font_data.get());
resulting_font_data);
}

void CSSFontSelectorBase::ReportLastResortFallbackFontLookup(
const FontDescription& font_description,
scoped_refptr<SimpleFontData> resulting_font_data) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(
&CSSFontSelectorBase::ReportLastResortFallbackFontLookup,
WrapCrossThreadPersistent(this), font_description,
resulting_font_data));
return;
}
#endif
SimpleFontData* resulting_font_data) {
GetFontMatchingMetrics()->ReportLastResortFallbackFontLookup(
font_description, resulting_font_data.get());
font_description, resulting_font_data);
}

void CSSFontSelectorBase::ReportNotDefGlyph() const {
Expand All @@ -278,33 +114,11 @@ void CSSFontSelectorBase::ReportNotDefGlyph() const {

void CSSFontSelectorBase::ReportSystemFontFamily(
const AtomicString& font_family_name) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(&CSSFontSelectorBase::ReportSystemFontFamily,
WrapCrossThreadPersistent(this), font_family_name));
return;
}
#endif
GetFontMatchingMetrics()->ReportSystemFontFamily(font_family_name);
}

void CSSFontSelectorBase::ReportWebFontFamily(
const AtomicString& font_family_name) {
#if defined(USE_PARALLEL_TEXT_SHAPING)
if (!IsAlive())
return;
if (!IsContextThread()) {
PostCrossThreadTask(
*task_runner_, FROM_HERE,
CrossThreadBindOnce(&CSSFontSelectorBase::ReportWebFontFamily,
WrapCrossThreadPersistent(this), font_family_name));
return;
}
#endif
GetFontMatchingMetrics()->ReportWebFontFamily(font_family_name);
}

Expand All @@ -313,21 +127,17 @@ void CSSFontSelectorBase::WillUseFontData(
const FontFamily& family,
const String& text) {
if (family.FamilyIsGeneric()) {
if (family.IsPrewarmed() || UNLIKELY(family.FamilyName().IsEmpty()))
if (family.IsPrewarmed())
return;
family.SetIsPrewarmed();
// |FamilyNameFromSettings| has a visible impact on the load performance.
// Because |FamilyName.IsPrewarmed| can prevent doing this multiple times
// only when the |Font| is shared across elements, and therefore it can't
// help when e.g., the font size is different, check once more if this
// generic family is already prewarmed.
{
AutoLockForParallelTextShaping guard(prewarmed_generic_families_lock_);
const auto result =
prewarmed_generic_families_.insert(family.FamilyName());
if (!result.is_new_entry)
return;
}
const auto result = prewarmed_generic_families_.insert(family.FamilyName());
if (!result.is_new_entry)
return;
const AtomicString& family_name =
FamilyNameFromSettings(font_description, family);
if (!family_name.IsEmpty())
Expand All @@ -341,7 +151,7 @@ void CSSFontSelectorBase::WillUseFontData(
return;
}

if (family.IsPrewarmed() || UNLIKELY(family.FamilyName().IsEmpty()))
if (family.IsPrewarmed())
return;
family.SetIsPrewarmed();
FontCache::PrewarmFamily(family.FamilyName());
Expand Down
Loading

0 comments on commit 42e100c

Please sign in to comment.