Revert "[Extensions] Upgrade APIBindingsSystem::InitializeType DCHECK to CHECK" [chromium/src : main]
0 views
Skip to first unread message
Grey Wang (Gerrit)
Jun 18, 2026, 12:01:13 AM (10 days ago) Jun 18
to Devlin Cronin, Tim, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org
Attention needed from Devlin Cronin and Tim
Grey Wang has uploaded the change for review![]( "Open in Gerrit")
Grey Wang would like Devlin Cronin, Tim and Chromium LUCI CQ to review this change.
Commit message
Revert "[Extensions] Upgrade APIBindingsSystem::InitializeType DCHECK to CHECK"
This reverts commit e4ad067e9374dd36120f04a75639e86523e20409.
Reason for revert: Suspect it breaks uprev
Failure Link: https://chromium-review.git.corp.google.com/c/chromiumos/overlays/chromiumos-overlay/+/7958847?tab=checks
Original change's description:
> [Extensions] Upgrade APIBindingsSystem::InitializeType DCHECK to CHECK
>
> This CL updates a DCHECK to a CHECK in APIBindingsSystem::InitializeType
> so if the requested API is already present in the api_bindings_ map, the
> renderer is killed.
>
> This prevents an issue where requesting an unknown type for an already
> instantiated API would unconditionally overwrite and destroy the
> existing APIBinding object. Destroying the binding could cause a
> Use-After-Free if any v8::Functions created by it (which hold raw
> pointers to the binding's internal data) were subsequently invoked.
>
> An expect-death test has also been added to document this.
>
> Fixed: 513467993
> Change-Id: I676a60ba207daa76cbdc67e5d2aca2db4d6a3a30
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7934732
> Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
> Commit-Queue: Tim <tjud...@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1648609}
Bug: 513467993
Change-Id: I71a338c5c3590da1058889094850558015a77638
Change diff
diff --git a/extensions/renderer/bindings/api_bindings_system.cc b/extensions/renderer/bindings/api_bindings_system.cc
index 81f305e..c9ca2f4 100644
--- a/extensions/renderer/bindings/api_bindings_system.cc
+++ b/extensions/renderer/bindings/api_bindings_system.cc
@@ -111,7 +111,7 @@
std::string api_name = type_name.substr(0, dot);
// If we've already instantiated the binding, the type should have been in
// there.
- CHECK(!api_bindings_.contains(api_name)) << api_name;
+ DCHECK(!api_bindings_.contains(api_name)) << api_name;
api_bindings_[api_name] = CreateNewAPIBinding(api_name);
}
diff --git a/extensions/renderer/bindings/api_bindings_system_unittest.cc b/extensions/renderer/bindings/api_bindings_system_unittest.cc
index a57213f..6d652fe 100644
--- a/extensions/renderer/bindings/api_bindings_system_unittest.cc
+++ b/extensions/renderer/bindings/api_bindings_system_unittest.cc
@@ -612,22 +612,4 @@
EXPECT_NE(event, other_event);
}
-// Tests that requesting an unknown type for an already-instantiated API
-// crashes the renderer. In theory this could be triggered by a compromised
-// renderer mishandling bindingUtil, so we just kill the renderer if it happens.
-TEST_F(APIBindingsSystemTest, DeathOnUnknownTypeInitialize) {
- v8::HandleScope handle_scope(isolate());
- v8::Local<v8::Context> context = MainContext();
-
- // Instantiate the 'alpha' API.
- v8::Local<v8::Object> alpha_api =
- bindings_system()->CreateAPIInstance(kAlphaAPIName, context, nullptr);
- ASSERT_FALSE(alpha_api.IsEmpty());
-
- // Trigger InitializeType for the 'alpha' API with an unknown type.
- EXPECT_DEATH_IF_SUPPORTED(
- bindings_system()->type_reference_map()->GetSpec("alpha.doesNotExist"),
- "api_bindings_\\.contains");
-}
-
} // namespace extensions
Change information
Files:
- M extensions/renderer/bindings/api_bindings_system.cc
- M extensions/renderer/bindings/api_bindings_system_unittest.cc
Change size: S
Delta: 2 files changed, 1 insertion(+), 19 deletions(-)
Related details
Attention is currently required from:
- Devlin Cronin
- Tim
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
rubber-stamper@appspot.gserviceaccount.com (Gerrit)
Jun 18, 2026, 12:02:39 AM (10 days ago) Jun 18
to Grey Wang, Devlin Cronin, Tim, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Devlin Cronin and Tim
rubber-...@appspot.gserviceaccount.com voted Bot-Commit+1![]( "Open in Gerrit")
| Bot-Commit | +1 |
Related details
Attention is currently required from:
- Devlin Cronin
- Tim
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Grey Wang (Gerrit)
Jun 18, 2026, 12:07:00 AM (10 days ago) Jun 18
to rubber-...@appspot.gserviceaccount.com, Devlin Cronin, Tim, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Devlin Cronin and Tim
Grey Wang voted Owners-Override+1![]( "Open in Gerrit")
| Owners-Override | +1 |
Related details
Attention is currently required from:
- Devlin Cronin
- Tim
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Grey Wang (Gerrit)
Jun 18, 2026, 12:08:09 AM (10 days ago) Jun 18
to rubber-...@appspot.gserviceaccount.com, Devlin Cronin, Tim, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Devlin Cronin and Tim
Grey Wang voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Jun 18, 2026, 1:00:37 AM (10 days ago) Jun 18
to Grey Wang, rubber-...@appspot.gserviceaccount.com, Devlin Cronin, Tim, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Revert "[Extensions] Upgrade APIBindingsSystem::InitializeType DCHECK to CHECK"
This reverts commit e4ad067e9374dd36120f04a75639e86523e20409.
Reason for revert: Suspect it breaks uprev
Failure Link: https://chromium-review.git.corp.google.com/c/chromiumos/overlays/chromiumos-overlay/+/7958847?tab=checks
Original change's description:
> [Extensions] Upgrade APIBindingsSystem::InitializeType DCHECK to CHECK
>
> This CL updates a DCHECK to a CHECK in APIBindingsSystem::InitializeType
> so if the requested API is already present in the api_bindings_ map, the
> renderer is killed.
>
> This prevents an issue where requesting an unknown type for an already
> instantiated API would unconditionally overwrite and destroy the
> existing APIBinding object. Destroying the binding could cause a
> Use-After-Free if any v8::Functions created by it (which hold raw
> pointers to the binding's internal data) were subsequently invoked.
>
> An expect-death test has also been added to document this.
>
> Fixed: 513467993
> Change-Id: I676a60ba207daa76cbdc67e5d2aca2db4d6a3a30
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7934732
> Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
> Commit-Queue: Tim <tjud...@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1648609}
Bug: 513467993
Change-Id: I71a338c5c3590da1058889094850558015a77638
Commit-Queue: Grey Wang <grey...@google.com>
Owners-Override: Grey Wang <grey...@google.com>
Cr-Commit-Position: refs/heads/main@{#1648800}
Files:
- M extensions/renderer/bindings/api_bindings_system.cc
- M extensions/renderer/bindings/api_bindings_system_unittest.cc
Change size: S
Delta: 2 files changed, 1 insertion(+), 19 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: Bot-Commit+1 by rubber-...@appspot.gserviceaccount.com
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages