inspector: return errors when CDP protocol event emission fails#62162
Merged
Merged
inspector: return errors when CDP protocol event emission fails#62162
Conversation
Collaborator
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62162 +/- ##
==========================================
+ Coverage 89.65% 89.75% +0.09%
==========================================
Files 676 676
Lines 206543 206643 +100
Branches 39547 39553 +6
==========================================
+ Hits 185184 185463 +279
+ Misses 13480 13353 -127
+ Partials 7879 7827 -52
🚀 New features to boost your workflow:
|
legendecas
reviewed
Mar 9, 2026
| Isolate* isolate = Isolate::GetCurrent(); | ||
| if (!ObjectGetProtocolString(context, storage_id_obj, "securityOrigin") | ||
| .To(&security_origin)) { | ||
| ThrowEventError(isolate, "Missing securityOrigin in storageId"); |
Member
There was a problem hiding this comment.
IIUC, this would throw an identical error for each session connected. Can we validate the object shape once so that the error is only thrown once? This would also prevent returning an array of errors in the JS API.
Member
Author
There was a problem hiding this comment.
If errors that depend on session-specific state are introduced in the future, this may need to be changed. However, since we currently only perform type checks, would it make sense to throw the error directly?
I’ve added a commit to implement this change.
Member
There was a problem hiding this comment.
Sounds good! Either way, returning an error is probably not the pattern as it does not align with the exception convention.
legendecas
reviewed
Mar 9, 2026
legendecas
approved these changes
Mar 16, 2026
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
Collaborator
Collaborator
Collaborator
cola119
approved these changes
Apr 6, 2026
Collaborator
|
Landed in 4f08c64 |
aduh95
pushed a commit
that referenced
this pull request
May 5, 2026
PR-URL: #62162 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
thisalihassan
pushed a commit
to thisalihassan/node
that referenced
this pull request
May 7, 2026
PR-URL: nodejs#62162 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
thisalihassan
added a commit
to thisalihassan/node
that referenced
this pull request
May 7, 2026
Signed-off-by: Ali Hassan <ali-hassan27@outlook.com> PR-URL: nodejs#62616 Refs: nodejs#62145 Refs: nodejs#62162 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
aduh95
pushed a commit
to thisalihassan/node
that referenced
this pull request
May 9, 2026
PR-URL: nodejs#62162 Backport-PR-URL: nodejs#63176 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95
pushed a commit
to thisalihassan/node
that referenced
this pull request
May 9, 2026
Signed-off-by: Ali Hassan <ali-hassan27@outlook.com> PR-URL: nodejs#62616 Backport-PR-URL: nodejs#63176 Refs: nodejs#62145 Refs: nodejs#62162 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
jkleinsc
added a commit
to electron/electron
that referenced
this pull request
Jun 3, 2026
Ref: nodejs/node#62162 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jkleinsc
added a commit
to electron/electron
that referenced
this pull request
Jun 3, 2026
Ref: nodejs/node#62162 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jkleinsc
added a commit
to electron/electron
that referenced
this pull request
Jun 3, 2026
Ref: nodejs/node#62162 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jkleinsc
added a commit
to electron/electron
that referenced
this pull request
Jun 4, 2026
Adapts api_remove_deprecated_getisolate.patch for the inspector NetworkAgent: adapt the GetIsolate removal to the NetworkAgent member helpers, qualify the v8 Maybe helpers in network_agent, and drop duplicate inspector helper definitions. Ref: nodejs/node#62162 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jkleinsc
added a commit
to electron/electron
that referenced
this pull request
Jun 4, 2026
Adapts api_remove_deprecated_getisolate.patch for the inspector NetworkAgent: adapt the GetIsolate removal to the NetworkAgent member helpers, qualify the v8 Maybe helpers in network_agent, and drop duplicate inspector helper definitions. Ref: nodejs/node#62162 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jkleinsc
added a commit
to electron/electron
that referenced
this pull request
Jun 5, 2026
…ctor Upstream refactored the inspector network agent helpers into NetworkAgent:: methods, shifting where context->GetIsolate() appeared and reorganizing node_contextify's PropertyDefinerCallback. Re-applied the deprecated context->GetIsolate() -> v8::Isolate::GetCurrent() replacements to the new structure. Ref: nodejs/node#62162 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jkleinsc
added a commit
to electron/electron
that referenced
this pull request
Jun 8, 2026
* chore: bump node in DEPS to v24.16.0
* chore: remove upstreamed patch
Node.js restored fs patchability in the ESM loader upstream, making
the fix_lazyload_fs_in_esm_loaders_to_apply_asar_patches.patch
obsolete (the patch's exact change is now in lib/internal/modules/
esm/{load,resolve,translators}.js).
Ref: nodejs/node#62835
Co-Authored-By: Claude <noreply@anthropic.com>
(cherry picked from commit 72638db)
* fix(patch): re-add experimental_fetch member after upstream cleanup
Upstream removed the experimental_fetch field from EnvironmentOptions,
but Electron's patch still registers --experimental-fetch as a CLI
option bound to that field. Re-add the member so the option compiles.
Ref: nodejs/node#62759
Co-Authored-By: Claude <noreply@anthropic.com>
(cherry picked from commit 6fd9370)
* fix(patch): cast const away when freeing uv_cpu_info_t.model
libuv 1.52.1 typed uv_cpu_info_t.model as const char*, but uv__free
takes void*. Electron builds with -Werror,-Wincompatible-pointer-types-
discards-qualifiers, so add a cast. The memory is allocated via strdup
so the cast is safe.
Ref: nodejs/node#61829
Co-Authored-By: Claude <noreply@anthropic.com>
(cherry picked from commit 90d1009)
* fix(patch): silence sign-compare warning in sessionVarintGetSafe
Cast int nBuf to size_t when comparing with sizeof(aCopy) so the
bundled sqlite3 amalgamation compiles under -Werror,-Wsign-compare.
Ref: nodejs/node#62699
Co-Authored-By: Claude <noreply@anthropic.com>
* test: move root package.json aside in node spec runner
third_party/electron_node lives under Chromium's src/, whose package.json
("type": "module") is always an ancestor of the Node.js test tree. Upstream
assumes no package.json sits above the tests, so that ancestor changes how
test files and fixtures resolve their module type: it disables module-syntax
detection (breaking test-compile-cache-typescript-esm) and emits
MODULE_TYPELESS_PACKAGE_JSON warnings that break tests asserting clean stderr
(test-esm-detect-ambiguous, test-esm-import-meta-main-eval,
test-output-coverage-with-mock).
Move src/package.json aside for the duration of the run so the environment
matches upstream exactly, then restore it. The original is kept in a sibling
backup file so an interrupted/killed run self-heals on the next invocation
rather than leaving src/package.json missing.
Ref: Unable to locate reference
Co-Authored-By: Claude <noreply@anthropic.com>
(cherry picked from commit 29abd0c)
* fix(patch): mark test-macos-app-sandbox as flaky
The test copies the Electron binary into a standalone .app bundle and
code-signs it; under parallel suite runs this races with dyld resolving
the Electron Framework rpath and intermittently aborts (SIGABRT). It
passes reliably when run alone. Mark it flaky so flakes don't fail CI.
Ref: Unable to locate reference
Co-Authored-By: Claude <noreply@anthropic.com>
(cherry picked from commit 3853529)
* test: disable test-tls-set-default-ca-certificates-extra-override
setDefaultCACertificates() round-trips the default CA set through
BoringSSL's X509_STORE, which dedups a duplicate-subject root (DigiCert
Global Root CA) that OpenSSL keeps. The set therefore loses one cert on
re-add (149 -> 148), so the test's assertEqualCerts round-trip check
fails under Electron's BoringSSL. The sibling -recovery test is disabled
for the same reason.
Ref: nodejs/node#58822
Co-Authored-By: Claude <noreply@anthropic.com>
(cherry picked from commit 3dbc596)
* fix(patch): mark test-runner watch tests as flaky
test-run-watch-repeatedly, test-run-watch-run-duration and
test-run-watch-without-file race under parallel suite load: the watcher
fires an extra re-run before the assertion, so the expected single-run
output ("tests 1") arrives with accumulated subtests. All three pass in
isolation.
Ref: nodejs/node#44898
Co-Authored-By: Claude <noreply@anthropic.com>
(cherry picked from commit 0f9c327)
* chore(patches): update libuv const-cast patch management
Combine the Windows libuv cpu_info const-cast update into the existing
chore_cast_const_away_when_freeing_uv_cpu_info_t_model.patch and keep
a single patch-management commit for the final exported patch series.
Co-Authored-By: Claude <noreply@anthropic.com>
(cherry picked from commit c590614)
* fix(patch): add UV_LOOP_INTERRUPT_ON_IO_CHANGE option
Ref: libuv/libuv#3308
Co-Authored-By: Claude <noreply@anthropic.com>
* chore: update patches (trivial only)
Co-Authored-By: Claude <noreply@anthropic.com>
* chore: update patches
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(patch): adapt GetIsolate removal for the inspector NetworkAgent
Adapts api_remove_deprecated_getisolate.patch for the inspector NetworkAgent:
adapt the GetIsolate removal to the NetworkAgent member helpers, qualify the
v8 Maybe helpers in network_agent, and drop duplicate inspector helper
definitions.
Ref: nodejs/node#62162
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(patch): guard vm contextual-store strict assertion for Chromium's V8
test-vm-global-contextual-store was added upstream in nodejs/node#62571. It
asserts that an undeclared strict-mode contextual store (`"use strict"; z = 42`)
throws a ReferenceError. Chromium's V8 removed
v8::PropertyCallbackInfo<T>::This(), so Electron's contextify setter cannot
distinguish a contextual store from an explicit `globalThis.x = v` store; it
keeps the original behavior (writing to the sandbox) so explicit global stores
in vm modules keep working (test-vm-module-referrer-realm). Guard the
strict-mode ReferenceError assertion in the new test under Electron.
Ref: nodejs/node#62571
Ref: nodejs/node#60616
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, there was no feedback when functions that emit events such as
Network.webSocketCreatedfailed. This change modifies the behavior so that feedback can be obtained when such failures occur.Since an event may be emitted to multiple sessions, instead of throwing an error directly, the errors are returned as an array.