Commit 0237e90a authored by Benoit Lize's avatar Benoit Lize Committed by Commit Bot

blink/loader: Don't clear decoded data in FontResource.

FontResource clears the decoded data when all clients are removed. This
is not interacting well with a recent change,
https://chromium-review.googlesource.com/c/chromium/src/+/2134291.

The change above discards the encoded data after it has been decoded, as
the encoded data is never used afterwards. However, when all clients of
a FontResource instance are destroyed (typically when the @font-face src
value object is destroyed), we clear the decoded data, but do not remove
the FontResource from the memory cache.

This means that the following scenario:
1. CSSFontFaceSrcValue is destroyed (typically after a navigation)
2. *No GC*
3. A new document is parsed, and the font is found in the memory cache

The newly-matched FontResource is not usable, since it doesn't hold a
decoded or encoded representation, triggering console messages, which
appear as a regression in the linked bug.

This can be fixed in two ways:
- Revert the encoded data clearing patch above
- Don't drop the decoded data when clients disappear

The second solution is preferable, since once all clients have
disappeared, the resource will not outlive the next GC cycle unless
claimed in the meantime. However clearing the encoded data reduces
memory footprint in steady state. In addition, the current behavior leads
to a new decompression of the font resource at each navigation, which is
wasteful.

As a consequence, this patch removes the clearing of decoded data once
clients are gone.
This was verified as removing the console warnings on cnn.com, which is
one of the sites triggering the alert on perf bots.

Bug: 1067226, 1067930
Change-Id: I3ebb96b8e310a2d6cf46fbbedfdce95999b520de
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2141924Reviewed-by: default avatarKinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758179}
parent bd552a51
...@@ -175,11 +175,6 @@ void FontResource::NotifyClientsLongLimitExceeded() { ...@@ -175,11 +175,6 @@ void FontResource::NotifyClientsLongLimitExceeded() {
client->FontLoadLongLimitExceeded(this); client->FontLoadLongLimitExceeded(this);
} }
void FontResource::AllClientsAndObserversRemoved() {
font_data_ = nullptr;
Resource::AllClientsAndObserversRemoved();
}
void FontResource::NotifyFinished() { void FontResource::NotifyFinished() {
font_load_short_limit_.Cancel(); font_load_short_limit_.Cancel();
font_load_long_limit_.Cancel(); font_load_long_limit_.Cancel();
......
...@@ -55,7 +55,6 @@ class CORE_EXPORT FontResource final : public Resource { ...@@ -55,7 +55,6 @@ class CORE_EXPORT FontResource final : public Resource {
void SetRevalidatingRequest(const ResourceRequestHead&) override; void SetRevalidatingRequest(const ResourceRequestHead&) override;
void AllClientsAndObserversRemoved() override;
void StartLoadLimitTimersIfNecessary(base::SingleThreadTaskRunner*); void StartLoadLimitTimersIfNecessary(base::SingleThreadTaskRunner*);
String OtsParsingMessage() const { return ots_parsing_message_; } String OtsParsingMessage() const { return ots_parsing_message_; }
...@@ -134,4 +133,4 @@ class FontResourceClient : public ResourceClient { ...@@ -134,4 +133,4 @@ class FontResourceClient : public ResourceClient {
} // namespace blink } // namespace blink
#endif #endif // THIRD_PARTY_BLINK_RENDERER_CORE_LOADER_RESOURCE_FONT_RESOURCE_H_
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