Skip to content

Commit

Permalink
Pass the id of the extension which blocked BFCache to devtools.
Browse files Browse the repository at this point in the history
The goal is to display it in the UI.

This allows attaching an arbitrary string to a BFCache blocking reason.
The string is treated as opaque data by content/ allowing chrome/
to pass data to devtools without breaking layering.

Bug: 1284548
Change-Id: Ibd28032682d0797c30676c29502be388c1e8cb92
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3445921
Reviewed-by: Danil Somsikov <dsv@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Reviewed-by: Minoru Chikamune <chikamune@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Fergal Daly <fergal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#974986}
  • Loading branch information
fergald authored and Chromium LUCI CQ committed Feb 25, 2022
1 parent 30b60b5 commit ede879e
Show file tree
Hide file tree
Showing 7 changed files with 37 additions and 15 deletions.
5 changes: 3 additions & 2 deletions components/back_forward_cache/back_forward_cache_disable.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,11 @@ std::string ReasonIdToString(DisabledReasonId reason_id) {
}

content::BackForwardCache::DisabledReason DisabledReason(
DisabledReasonId reason_id) {
DisabledReasonId reason_id,
const std::string& context) {
return content::BackForwardCache::DisabledReason(
{content::BackForwardCache::DisabledSource::kEmbedder,
static_cast<content::BackForwardCache::DisabledReasonType>(reason_id),
ReasonIdToString(reason_id)});
ReasonIdToString(reason_id), context});
}
} // namespace back_forward_cache
3 changes: 2 additions & 1 deletion components/back_forward_cache/back_forward_cache_disable.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
namespace back_forward_cache {
// Constructs a chrome-specific DisabledReason
content::BackForwardCache::DisabledReason DisabledReason(
DisabledReasonId reason_id);
DisabledReasonId reason_id,
const std::string& context = "");
} // namespace back_forward_cache

#endif // COMPONENTS_BACK_FORWARD_CACHE_BACK_FORWARD_CACHE_DISABLE_H_
7 changes: 5 additions & 2 deletions content/browser/devtools/protocol/page_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1792,13 +1792,16 @@ CreateNotRestoredExplanation(
} else if (reason == BackForwardCacheMetrics::NotRestoredReason::
kDisableForRenderFrameHostCalled) {
for (auto disabled_reason : disabled_reasons) {
reasons->emplace_back(
auto reason =
Page::BackForwardCacheNotRestoredExplanation::Create()
.SetType(
MapDisableForRenderFrameHostReasonToType(disabled_reason))
.SetReason(
DisableForRenderFrameHostReasonToProtocol(disabled_reason))
.Build());
.Build();
if (!disabled_reason.context.empty())
reason->SetContext(disabled_reason.context);
reasons->emplace_back(std::move(reason));
}
} else {
reasons->emplace_back(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,9 @@ std::string DisabledReasonsToString(
const std::set<BackForwardCache::DisabledReason>& reasons) {
std::vector<std::string> descriptions;
for (const auto& reason : reasons) {
descriptions.push_back(base::StringPrintf(
"%d:%d:%s", reason.source, reason.id, reason.description.c_str()));
descriptions.push_back(
base::StringPrintf("%d:%d:%s:%s", reason.source, reason.id,
reason.description.c_str(), reason.context.c_str()));
}
return base::JoinString(descriptions, ", ");
}
Expand Down
9 changes: 6 additions & 3 deletions content/public/browser/back_forward_cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,16 @@ class CONTENT_EXPORT BackForwardCache {
typedef uint16_t DisabledReasonType;
static const uint16_t kDisabledReasonTypeBits = 16;

// Represents a reason to disable back-forward cache, given by a source. It
// preserves the string that accompanied it, however the string is ignored for
// <, == and !=.
// Represents a reason to disable back-forward cache, given by a |source|.
// |context| is arbitrary context that will be preserved and passed through,
// e.g. an extension ID responsible for disabling BFCache that can be shown in
// passed devtools. It preserves the |description| and |context| that
// accompany it, however they are ignored for <, == and !=.
struct CONTENT_EXPORT DisabledReason {
const BackForwardCache::DisabledSource source;
const BackForwardCache::DisabledReasonType id;
const std::string description;
const std::string context;

bool operator<(const DisabledReason&) const;
bool operator==(const DisabledReason&) const;
Expand Down
18 changes: 13 additions & 5 deletions extensions/browser/api/messaging/extension_message_port.cc
Original file line number Diff line number Diff line change
Expand Up @@ -449,19 +449,27 @@ void ExtensionMessagePort::SendToPort(IPCBuilderCallback ipc_builder) {

for (const IPCTarget& target : targets) {
// Frames in the BackForwardCache are not allowed to receive messages (or
// even have them queued). In such a case, we evict the frame from the cache
// even have them queued). In such a case, we evict the page from the cache
// and "drop" the message (See comment in `DidFinishNavigation()`).
// Note: Since this will cause the frame to be deleted, we do this here
// instead of in the loop above to avoid modifying `frames_` while it is
// being iterated.
//
// This could cause the same page to be evicted multiple times if it has
// multiple frames receiving this message. This is harmless as the reason is
// the same in every case. Also multiple extensions may send messages before
// the page is actually evicted. The last one will be the one the user
// sees. It is not worth the effort to present all of them to the user. It's
// unlikely they will see the same one every time and if they do, when they
// fix that one, they will see the others.
if (target.render_frame_host &&
target.render_frame_host->GetLifecycleState() ==
content::RenderFrameHost::LifecycleState::kInBackForwardCache) {
content::BackForwardCache::DisableForRenderFrameHost(
target.render_frame_host,
back_forward_cache::DisabledReason(
back_forward_cache::DisabledReasonId::
kExtensionSentMessageToCachedFrame));
target.render_frame_host, back_forward_cache::DisabledReason(
back_forward_cache::DisabledReasonId::
kExtensionSentMessageToCachedFrame,
/*context=*/extension_id_));
continue;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8207,6 +8207,11 @@ domain Page
BackForwardCacheNotRestoredReasonType type
# Not restored reason
BackForwardCacheNotRestoredReason reason
# Context associated with the reason. The meaning of this context is
# dependent on the reason:
# - EmbedderExtensionSentMessageToCachedFrame: the extension ID.
#
optional string context

experimental type BackForwardCacheNotRestoredExplanationTree extends object
properties
Expand Down

0 comments on commit ede879e

Please sign in to comment.