[SharedWorker] Implement exception reporting [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
int exception_id) {This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent) \
To opt out permanently, please join [Blink AIAgent Optout](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)
The `exception_id` parameter is unused in this function's body. To adhere to the style guide, please comment out the name of the unused parameter (e.g., `int /* exception_id */`) to indicate it is intentionally not used. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
For a quick and terse communication, please consider using one of these options: \
Done | b/<bug_id> | Invalid: reason | Won't fix: reason<br/>
NOTE: AI reviews can give wrong results. \
We apologize in advance for any inconvenience and would love to take your feedback to make this better. \
[file a bug](http://go/blink-c++-code-review-agent-feedback) or [ping us on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4)
Related details
- Hiroki Nakagawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent) \
To opt out permanently, please join [Blink AIAgent Optout](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)The `exception_id` parameter is unused in this function's body. To adhere to the style guide, please comment out the name of the unused parameter (e.g., `int /* exception_id */`) to indicate it is intentionally not used. (Blink Style Guide: Naming - May leave obvious parameter names out of function declarations)
For a quick and terse communication, please consider using one of these options: \
Done | b/<bug_id> | Invalid: reason | Won't fix: reason<br/>NOTE: AI reviews can give wrong results. \
We apologize in advance for any inconvenience and would love to take your feedback to make this better. \
[file a bug](http://go/blink-c++-code-review-agent-feedback) or [ping us on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4)
Done
Related details
- Hiroki Nakagawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: toyo...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): toyo...@chromium.org
Reviewer source(s):
toyo...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Hiroki Nakagawa
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
FYI, Anna might have asked me to implement this before.
Related details
- Hiroki Nakagawa
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 7 comments![]( "Open in Gerrit")
MockSharedWorkerClient::MockSharedWorkerClient() {}Please fix this WARNING reported by ClangTidy: check: modernize-use-equals-default
use '= default' to define a trivial default...
check: modernize-use-equals-default
use '= default' to define a trivial default constructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-equals-default` footer to the CL description to skip the check)
(Note: building this file or its dependencies failed; this diagnostic might be incorrect as a result.)
(Lint observed on `mac-clang-tidy-rel`, but not on `android-clang-tidy-rel` or `linux-clang-tidy-rel`)
DCHECK(!on_report_exception_received_);EXPECT_FALSE is better in a mock? (ditto on existing DCHECKs above)
// Reports an exception to the creator frame's console.Can we add some more detailed explanations on the `is_eval_error` parameter?
E.g. like a comment in shared_worker_client.cc
fyi, especially in Blink, we prefer enum over bool; https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/blink-c++.md#Prefer-enums-or-StrongAliases-to-bare-bools-for-function-parameters
But as this is a Mojo interface, it's on border, and maybe ok to keep using bool here (but needs a comment as this variable meaning is not trivial)
// Reports an exception to the creator frame's console.ditto
DCHECK(IsMainThread());maybe prefer CHECK() for new code (and ideally make all cases in the same file consistent)
ErrorEvent::Create(error_message, location, /*error=*/nullptr);maybe meant `world`?
DCHECK(!IsMainThread());Can we check if this is called only once at most?
`CHECK(!script_evaluated_);`
Related details
- Hiroki Nakagawa
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 2 comments![]( "Open in Gerrit")
FYI: I'll review this CL after having offline discussion about the spec with yyanagisawa@ tomorrow.
// https://html.spec.whatwg.org/multipage/workers.html#worker-processing-modelhttps://html.spec.whatwg.org/C/#worker-processing-model is shorter and more consistent with the next link.
Related details
- Yoshisato Yanagisawa
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 8 comments![]( "Open in Gerrit")
MockSharedWorkerClient::MockSharedWorkerClient() {}Please fix this WARNING reported by ClangTidy: check: modernize-use-equals-default
use '= default' to define a trivial default...
check: modernize-use-equals-default
use '= default' to define a trivial default constructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-equals-default` footer to the CL description to skip the check)
(Note: building this file or its dependencies failed; this diagnostic might be incorrect as a result.)
(Lint observed on `mac-clang-tidy-rel`, but not on `android-clang-tidy-rel` or `linux-clang-tidy-rel`)
Done
EXPECT_FALSE is better in a mock? (ditto on existing DCHECKs above)
Done
// Reports an exception to the creator frame's console.Can we add some more detailed explanations on the `is_eval_error` parameter?
E.g. like a comment in shared_worker_client.ccfyi, especially in Blink, we prefer enum over bool; https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/blink-c++.md#Prefer-enums-or-StrongAliases-to-bare-bools-for-function-parameters
But as this is a Mojo interface, it's on border, and maybe ok to keep using bool here (but needs a comment as this variable meaning is not trivial)
Sure.
updated the arguments.
// Reports an exception to the creator frame's console.Yoshisato Yanagisawaditto
Done
maybe prefer CHECK() for new code (and ideally make all cases in the same file consistent)
Done
ErrorEvent::Create(error_message, location, /*error=*/nullptr);Yoshisato Yanagisawamaybe meant `world`?
Done
// https://html.spec.whatwg.org/multipage/workers.html#worker-processing-modelhttps://html.spec.whatwg.org/C/#worker-processing-model is shorter and more consistent with the next link.
Done
Can we check if this is called only once at most?
`CHECK(!script_evaluated_);`
- Hiroki Nakagawa
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
int32 line_number;
int32 column_number;
string source_url;We may use netowrk.mojom.SourceLocation for these triples here?
services/network/public/mojom/source_location.mojom
Related details
- Hiroki Nakagawa
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hiroki Nakagawa
| Code-Review | +1 |
1 comment
Related details
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
int32 line_number;
int32 column_number;
string source_url;We may use netowrk.mojom.SourceLocation for these triples here?
services/network/public/mojom/source_location.mojom
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
Can I ask you to review the BUILD.gn?
I have added shared_worker_exception_details.mojom, which has a struct used by other mojom files. When SharedWorker error happens during its set up time, it need to report that to the client. The client needs to know details of the exceptions and the struct contains them.
Related details
- Kent Tamura
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura voted Code-Review+1![]( "Open in Gerrit")
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[SharedWorker] Implement exception reporting
This patch implements the previously missing exception
reporting for Shared Workers. When an unhandled exception
occurs in a worker, the details are now propagated to all
connected clients.
To align with the HTML specification and pass relevant Web
Platform Tests, this change distinguishes between script
evaluation errors and runtime errors. A generic `Event` is
dispatched for script errors (e.g., parse errors), while a
detailed `ErrorEvent` is dispatched for runtime exceptions.
This is achieved by:
- `SharedWorkerGlobalScope` reporting all exceptions to its
`SharedWorkerReportingProxy`.
- The proxy tracks whether the top-level script has finished
evaluation. It prevents reporting of exceptions that occur
after evaluation (i.e., runtime errors).
- For pre-evaluation errors, the proxy uses a heuristic to
distinguish script parse errors from other evaluation-time
errors by checking for "SyntaxError" in the message. This
determines the value of an `is_eval_error` boolean.
- This boolean is passed through the `OnReportException`
Mojo IPC call from the worker, to the browser process (
`SharedWorkerHost`), and then to all connected renderer
clients (`SharedWorkerClient`).
- The `SharedWorkerClient` uses `is_eval_error` to dispatch
the appropriate event type (`Event` for parse errors,
`ErrorEvent` otherwise) to the `worker.onerror` handler.
This change improves the debugging experience by making
runtime errors visible in the developer console, while
correctly handling script evaluation failures.
- M content/browser/worker_host/mock_shared_worker.cc
- M content/browser/worker_host/mock_shared_worker.h
- M content/browser/worker_host/shared_worker_host.cc
- M content/browser/worker_host/shared_worker_host.h
- M content/browser/worker_host/shared_worker_host_unittest.cc
- M third_party/blink/public/mojom/BUILD.gn
- M third_party/blink/public/mojom/worker/shared_worker_client.mojom
- A third_party/blink/public/mojom/worker/shared_worker_exception_details.mojom
- M third_party/blink/public/mojom/worker/shared_worker_host.mojom
- M third_party/blink/renderer/core/exported/web_shared_worker_impl.cc
- M third_party/blink/renderer/core/exported/web_shared_worker_impl.h
- M third_party/blink/renderer/core/workers/shared_worker_client.cc
- M third_party/blink/renderer/core/workers/shared_worker_client.h
- M third_party/blink/renderer/core/workers/shared_worker_reporting_proxy.cc
- M third_party/blink/renderer/core/workers/shared_worker_reporting_proxy.h
- M third_party/blink/web_tests/TestExpectations
Code-Review: +1 by Hiroki Nakagawa, +1 by Takashi Toyoshima, +1 by Kent Tamura
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |