[WIP] [Extensions] Display runtime errors in chrome://extensions. [chromium/src : main]
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Istiaque Ahmed.
Ghazale Hosseinabadi would like Hiroki Nakagawa to review this change.
[WIP] [Extensions] Display runtime errors in chrome://extensions.
This CL displays runtime errors in chrome://extensions page.
Bug: 1152202
Change-Id: I4e8979ddd35c0d17e14fefba3557e8740bdac5b9
---
M chrome/browser/extensions/service_worker_apitest.cc
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/manifest.json
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js
M extensions/browser/service_worker_task_queue.cc
M extensions/browser/service_worker_task_queue.h
5 files changed, 97 insertions(+), 1 deletion(-)
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Makoto Shimazu, Istiaque Ahmed.
2 comments:
Patchset:
Now that crrev.com/c/2638931 is landed, I resumed my work on this. […]
nhiroki@ I was working on this CL with Makoto and I was wondering if you can now review my CL? I think Makoto wanted to report the error from DidRegisterServiceWorker(), but I couldn't make it work in DidRegisterServiceWorker(). How is this done for a web page with a service worker? Could you please point me to the code that does that? Thanks so much!
Patchset:
Hi Hiroki,
Could you please respond to my comment in this CL?
Thanks,
Ghazale
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi, Makoto Shimazu, Istiaque Ahmed.
3 comments:
Commit Message:
Patch Set #12, Line 9:
chrome://extensionsJust curious: How do we see errors in the chrome://extensions page? I cannot find console outputs there.
Patchset:
nhiroki@ I was working on this CL with Makoto and I was wondering if you can now review my CL? I thi […]
Sure, I'll take over.
We could change the type of the callback (StatusCallback) on ServiceWorkerContextWrapper::RegisterServiceWorker() so that it takes an error message, and then run the callback with an error message in the lambda function here:
https://source.chromium.org/chromium/chromium/src/+/master:content/browser/service_worker/service_worker_context_wrapper.cc;l=481-484;drc=b6590af2c9ba51513fe1f8460681dac7e6d81208The std::string on the 3rd argument of the lambda function should be the error message (status message).
By the way, this error message doesn't contain error location etc (e.g. line number) so the current approach could be more useful for debugging.
File extensions/browser/service_worker_task_queue.cc:
ExtensionsBrowserClient::Get()->ReportError(browser_context_,
std::move(error));If we observe OnReportConsoleMessage(), does this duplicate the error message? If so, we might want to remove this and just return.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Makoto Shimazu.
3 comments:
Commit Message:
Patch Set #12, Line 9:
chrome://extensions
Just curious: How do we see errors in the chrome://extensions page? I cannot find console outputs th […]
I will email you screenshots showing the error.
Patchset:
Sure, I'll take over. […]
We want to highlight the syntax error. Since error message doesn't contain error location etc (e.g. line number), what do you think of having this CL as the solution?
File extensions/browser/service_worker_task_queue.cc:
ExtensionsBrowserClient::Get()->ReportError(browser_context_,
std::move(error));
If we observe OnReportConsoleMessage(), does this duplicate the error message? If so, we might want […]
Yes, this duplicated the error message. I can send a separate CL to revert https://chromium-review.googlesource.com/c/chromium/src/+/2505602. I can do the revert right before I land this CL. Does that sound good to you?
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi, Makoto Shimazu.
10 comments:
Commit Message:
Patch Set #12, Line 9:
chrome://extensions
I will email you screenshots showing the error.
Thanks!
Patchset:
We want to highlight the syntax error. Since error message doesn't contain error location etc (e.g. […]
Proceeding with this CL sounds good to me.
Patchset:
Looks good.
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #12, Line 390:
constexprI'm not sure why `constexpr` has been used (in NonRootDirectory). `const` seems sufficient.
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/manifest.json:
Patch Set #12, Line 4:
"manifest_version": 2,Is service-worker based background page available for the manifest V2? According to this article, looks like it's a feature for the manifest V3.
https://developer.chrome.com/docs/extensions/mv3/intro/mv3-overview/
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
2021
Patch Set #12, Line 5:
console.lg('test');indent.
File extensions/browser/service_worker_task_queue.cc:
ExtensionsBrowserClient::Get()->ReportError(browser_context_,
std::move(error));
Yes, this duplicated the error message. I can send a separate CL to revert https://chromium-review. […]
sgtm.
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)Can we use console::ConsoleMessage.message_level as the source of logging::LogSeverity?
Patch Set #12, Line 607:
-1 /* render_process_id */)Is it feasible to specify the worker's process id? If not, it's ok for now.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi.
Ghazale Hosseinabadi uploaded patch set #13 to this change.
[Extensions] Display runtime errors in chrome://extensions.
This CL displays runtime errors in chrome://extensions page.
Bug: 1152202
Change-Id: I4e8979ddd35c0d17e14fefba3557e8740bdac5b9
---
M chrome/browser/extensions/service_worker_apitest.cc
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/manifest.json
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js
M extensions/browser/service_worker_task_queue.cc
M extensions/browser/service_worker_task_queue.h
5 files changed, 97 insertions(+), 1 deletion(-)
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Istiaque Ahmed.
7 comments:
Patchset:
Hi Istiaque,
Could you please review my CL?
Thanks,
Ghazale
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #12, Line 390:
constexprI'm not sure why `constexpr` has been used (in NonRootDirectory). `const` seems sufficient.
Changed it to const.
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/manifest.json:
Patch Set #12, Line 4:
"manifest_version": 2,
Is service-worker based background page available for the manifest V2? According to this article, lo […]
changed it to 3.
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
2021
Done
Patch Set #12, Line 5:
console.lg('test');indent.
Done
File extensions/browser/service_worker_task_queue.cc:
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)Can we use console::ConsoleMessage. […]
For that, I need ConsoleMessageLevelToLogSeverity. Can I move content/browser/log_console_message.* to content/public/browser/log_console_message.*?
Patch Set #12, Line 607:
-1 /* render_process_id */)Is it feasible to specify the worker's process id? If not, it's ok for now.
No. OnReportConsoleMessage does not give me the worker's process id and it seems to me it's not possible to retrieve the worker's process id in this function.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi, Istiaque Ahmed.
4 comments:
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/manifest.json:
Patch Set #12, Line 4:
"manifest_version": 2,
changed it to 3.
Done
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
What will happen if the service worker script contains an undefined variable (or throw an error etc) […]
Any updates on this?
We could mimic these tests (maybe in follow-up CLs?):
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/web_tests/external/wpt/service-workers/service-worker/resources/malformed-worker.py;drc=9c00256bc926a519e0e75b7e8efe35968e27e5f1
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/web_tests/external/wpt/service-workers/service-worker/resources/registration-tests-script.js;l=1;drc=9c00256bc926a519e0e75b7e8efe35968e27e5f1(I'm also curious about "the doc". Would it be possible to share it?)
File extensions/browser/service_worker_task_queue.cc:
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)
For that, I need ConsoleMessageLevelToLogSeverity. Can I move content/browser/log_console_message. […]
Using the existing function sounds nice. I'd move only ConsoleMessageLevelToLogSeverity() to content/public/browser/console_message.*.
Patch Set #12, Line 607:
-1 /* render_process_id */)
No. […]
Acked. Thanks for investigation!
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Istiaque Ahmed (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi.
1 comment:
File extensions/browser/service_worker_task_queue.cc:
Patch Set #14, Line 239:
partition->GetServiceWorkerContext()->AddObserver(this);This could potentially add the same oberver(= this) to the same ServiceWorkerContext. Does this fail in that scenario?
Try loading two extensions in the same SWContext (typically loading two regular extensions with workers will produce the effect) and see.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Istiaque Ahmed.
1 comment:
File extensions/browser/service_worker_task_queue.cc:
Patch Set #14, Line 239:
partition->GetServiceWorkerContext()->AddObserver(this);
This could potentially add the same oberver(= this) to the same ServiceWorkerContext. […]
that is true. I loaded two extensions and both had the same oberver(= this) to the same ServiceWorkerContext. I can add an if here to add this to observer_list_ only once, but I was wondering if there is a better solution?
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Istiaque Ahmed (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi.
2 comments:
Patchset:
There are previous patchsets' unresolved comments in this CL, could you also resolve them before I take another look? Thanks.
File extensions/browser/service_worker_task_queue.cc:
Patch Set #14, Line 239:
partition->GetServiceWorkerContext()->AddObserver(this);
that is true. […]
Yes, the way I did it once (locally) was to remember which SWContext*s are being observed... and use that to decide whether to add the observer or not. I did not think enough that time to figure out a better solution : )
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Istiaque Ahmed.
4 comments:
Patchset:
There are previous patchsets' unresolved comments in this CL, could you also resolve them before I t […]
Done.
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
Any updates on this? […]
It is the "Service Worker/Extension Layer Interaction" doc that I just shared with you. I tested this scenario (undefined variable) and it is showing an error on chrome://extensions.I added a test here and I will add a follow-up CL to add the wpt tests.
File extensions/browser/service_worker_task_queue.cc:
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)
Using the existing function sounds nice. […]
I did this, but when building I get the following error (during linking). I couldn't figure out why this is happening, could you please take a look? Thanks!
error: undefined symbol: content::ConsoleMessageLevelToLogSeverity(blink::mojom::ConsoleMessageLevel)
>>> referenced by log_console_message.cc:22 (../../content/browser/log_console_message.cc:22)
>>> obj/content/browser/browser/log_console_message.o:(content::LogConsoleMessage(blink::mojom::ConsoleMessageLevel, std::__Cr::basic_string<char16_t, std::__Cr::char_traits<char16_t>, std::__Cr::allocator<char16_t> > const&, int, bool, bool, std::__Cr::basic_string<char16_t, std::__Cr::char_traits<char16_t>, std::__Cr::allocator<char16_t> > const&))
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.
File extensions/browser/service_worker_task_queue.cc:
Patch Set #14, Line 239:
partition->GetServiceWorkerContext()->AddObserver(this);
Yes, the way I did it once (locally) was to remember which SWContext*s are being observed... […]
Done
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi, Istiaque Ahmed.
3 comments:
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
It is the "Service Worker/Extension Layer Interaction" doc that I just shared with you. […]
Thanks! Acked.
File content/public/browser/console_message.h:
Patch Set #15, Line 15:
logging::LogSeverity ConsoleMessageLevelToLogSeverity(To fix the link issue, probably this needs CONTENT_EXPORT.
File extensions/browser/service_worker_task_queue.cc:
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)
I did this, but when building I get the following error (during linking). […]
Seems like the function is not exposed to the outside of the content module. See my comment in content/public/browser/console_message.h.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi, Istiaque Ahmed.
Ghazale Hosseinabadi uploaded patch set #17 to this change.
[Extensions] Display runtime errors in chrome://extensions.
This CL makes code changes to display runtime errors on chrome://extensions for service worker based extensions.
Bug: 1152202
Change-Id: I4e8979ddd35c0d17e14fefba3557e8740bdac5b9
---
M chrome/browser/extensions/service_worker_apitest.cc
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/manifest.json
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/undefined_variable/manifest.json
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/undefined_variable/service_worker_background.js
M components/performance_manager/service_worker_context_adapter.cc
M components/performance_manager/service_worker_context_adapter.h
M content/browser/log_console_message.cc
M content/browser/log_console_message.h
M content/browser/service_worker/service_worker_context_wrapper.cc
M content/browser/service_worker/service_worker_context_wrapper.h
A content/public/browser/console_message.cc
M content/public/browser/console_message.h
M content/public/browser/service_worker_context.h
M extensions/browser/service_worker_task_queue.cc
M extensions/browser/service_worker_task_queue.h
16 files changed, 202 insertions(+), 28 deletions(-)
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi, Istiaque Ahmed.
Ghazale Hosseinabadi uploaded patch set #18 to this change.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Istiaque Ahmed.
1 comment:
File extensions/browser/service_worker_task_queue.cc:
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)
Seems like the function is not exposed to the outside of the content module. […]
I added CONTENT_EXPORT, but I still get the same error:
error: undefined symbol: content::ConsoleMessageLevelToLogSeverity(blink::mojom::ConsoleMessageLevel)
>>> referenced by log_console_message.cc:22 (../../content/browser/log_console_message.cc:22)
>>> obj/content/browser/browser/log_console_message.o:(content::LogConsoleMessage(blink::mojom::ConsoleMessageLevel, std::__Cr::basic_string<char16_t, std::__Cr::char_traits<char16_t>, std::__Cr::allocator<char16_t> > const&, int, bool, bool, std::__Cr::basic_string<char16_t, std::__Cr::char_traits<char16_t>, std::__Cr::allocator<char16_t> > const&))
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi, Istiaque Ahmed.
1 comment:
File extensions/browser/service_worker_task_queue.cc:
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)
I added CONTENT_EXPORT, but I still get the same error: […]
Ah, content/public/browser/console_message.cc is a new file but not added to any BUILD.gn, so it's not compiled.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Istiaque Ahmed.
2 comments:
File extensions/browser/service_worker_task_queue.cc:
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)
Ah, content/public/browser/console_message.cc is a new file but not added to any BUILD. […]
Thanks, that fixed my problem. I can not not use content::ConsoleMessageLevelToLogSeverity(message.message_level), since that causes chrome to crash at
[3849870:3849870:0427/152828.527812:FATAL:error_console.cc(136)] Check failed: error->level() >= extension_misc::kMinimumSeverityToReportError (0 vs. 1)Errors less than severity warning should not be reported.
Is it OK to go back to using
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)?
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)
Ah, content/public/browser/console_message.cc is a new file but not added to any BUILD. […]
That worked, thanks so much!
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi, Istiaque Ahmed.
4 comments:
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
Patch Set #19, Line 3:
// found in the LICENSE file.It would be nice to have a blank line between the license and scripts.
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/undefined_variable/service_worker_background.js:
Patch Set #19, Line 3:
// found in the LICENSE file.Ditto.
File content/browser/log_console_message.cc:
Can you revert this change?
File extensions/browser/service_worker_task_queue.cc:
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)
Thanks, that fixed my problem. I can not not use content::ConsoleMessageLevelToLogSeverity(message. […]
I think static_cast'ing to logging::LOGGING_ERROR is not the right fix because an informational message can be reported with a higher severity and sends wrong signal to web developers.
Looks like the Extentions defines the minimum severity and filters out messages with lower severity, for example, here:
https://source.chromium.org/chromium/chromium/src/+/master:extensions/renderer/extensions_render_frame_observer.cc;l=122;drc=221e331b49dfefadbc6fa40b0c68e6f97606d0b3Probably we should do the same thing in ServiceWorkerTaskQueue::OnReportConsoleMessage().
By the way, I'm curious about what kind of error message is reported with `error->level() == 0` because it's the INFO severity. Did you check the message content?
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Istiaque Ahmed.
4 comments:
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
Patch Set #19, Line 3:
// found in the LICENSE file.It would be nice to have a blank line between the license and scripts.
Done
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/undefined_variable/service_worker_background.js:
Patch Set #19, Line 3:
// found in the LICENSE file.Ditto.
Done
File content/browser/log_console_message.cc:
Can you revert this change?
Done
File extensions/browser/service_worker_task_queue.cc:
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)
I think static_cast'ing to logging::LOGGING_ERROR is not the right fix because an informational mess […]
I got this error because I was setting const int32_t resolved_level = 0;
in https://chromium-review.googlesource.com/c/chromium/src/+/2567672/19/content/browser/log_console_message.cc#21Now that I fixed that, this problem is also fixed and errors are being reported to the console.
When error->level() == 0 I got the following in the console:
[3849870:3849870:0427/152828.527812:FATAL:error_console.cc(136)] Check failed: error->level() >= extension_misc::kMinimumSeverityToReportError (0 vs. 1)Errors less than severity warning should not be reported.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi, Istiaque Ahmed.
Patch set 20:Code-Review +1
8 comments:
Patchset:
LGTM from the perspective of the service workers.
File content/browser/log_console_message.h:
Patch Set #20, Line 16:
// blink::mojom::ConsoleMessageLevel level);Can you remove this?
File content/public/browser/console_message.h:
Patch Set #15, Line 15:
logging::LogSeverity ConsoleMessageLevelToLogSeverity(To fix the link issue, probably this needs CONTENT_EXPORT.
Done
File extensions/browser/service_worker_task_queue.cc:
Patch Set #12, Line 606:
static_cast<logging::LogSeverity>(logging::LOGGING_ERROR)
I got this error because I was setting const int32_t resolved_level = 0; […]
Acked. Thanks!
File extensions/browser/service_worker_task_queue.cc:
if (lazy_context_id.browser_context() != browser_context_)
return;I wonder if this if-condition is never satisfied because LazyContextId is created from `browser_context_` at line 227.
Patch Set #20, Line 237:
lazy_context_id.extension_id(), lazy_context_id.browser_context()We can pass `extension_id` and `browser_context_` here without taking `lazy_context_id` from `context_id`.
Patch Set #20, Line 286:
if (lazy_context_id.browser_context() != browser_context_)Ditto.
Patch Set #20, Line 291:
lazy_context_id.extension_id(), lazy_context_id.browser_context());Ditto.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Istiaque Ahmed (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi.
2 comments:
Patchset:
Sending one comment/question below. Could you also resolve all the unresolved comments in the CL before next round? Thanks.
File extensions/browser/service_worker_task_queue.cc:
Patch Set #20, Line 293:
partition->GetServiceWorkerContext()->RemoveObserver(this);I think there could be issues here: e.g.:
If we had extension1 -> service_worker_context1
and extension2 -> service_worker_context1,
then say extension1 added observer first, and then extension2 tried to add observer (skipping because HasObserver will return true), then in the case extension1 removes its observer:
- extension2 would no longer have an observer, isn't that a problem?
- extension2 will try to remove the observer that was already removed. That, in theory, will/might fail.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Istiaque Ahmed.
8 comments:
Patchset:
Sending one comment/question below. […]
Done.
File content/browser/log_console_message.h:
Patch Set #20, Line 16:
// blink::mojom::ConsoleMessageLevel level);Can you remove this?
Done
Patch Set #20, Line 16:
// blink::mojom::ConsoleMessageLevel level);Can you remove this?
Done
File extensions/browser/service_worker_task_queue.cc:
if (lazy_context_id.browser_context() != browser_context_)
return;
I wonder if this if-condition is never satisfied because LazyContextId is created from `browser_cont […]
right, I deleted it.
Patch Set #20, Line 237:
lazy_context_id.extension_id(), lazy_context_id.browser_context()
We can pass `extension_id` and `browser_context_` here without taking `lazy_context_id` from `contex […]
Done
Patch Set #20, Line 286:
if (lazy_context_id.browser_context() != browser_context_)Ditto.
Done
Patch Set #20, Line 291:
lazy_context_id.extension_id(), lazy_context_id.browser_context());Ditto.
Done
Patch Set #20, Line 293:
partition->GetServiceWorkerContext()->RemoveObserver(this);
I think there could be issues here: e.g.: […]
In which scenario the above happens? I tried loading (or reloading) two extensions. I have copy/pasted the logs that I see. Basically, RemoveObserver is called when loading the extension is done. And when the other extension is loaded, AddObserver and then RemoveObserver are called.
[2422968:2422968:0506/162435.925466:INFO:CONSOLE(2)] "Uncaught TypeError: console.lg is not a function", source: chrome-extension://hjpmelllpnicnbngibcognoknefogbmi/background.js (2)
[2422968:2422968:0506/162435.926152:WARNING:service_worker_task_queue.cc(237)] AddObserver
[2422968:2422968:0506/162435.949230:WARNING:service_worker_task_queue.cc(285)] RemoveObserver
[2422968:2422968:0506/162437.789541:INFO:CONSOLE(2)] "Breakpoint!", source: chrome-extension://lmlndkckcngbkcmpgkologjapikonghg/background.js (2)
[2422968:2422968:0506/162437.791629:INFO:CONSOLE(3)] "Uncaught TypeError: console.g is not a function", source: chrome-extension://lmlndkckcngbkcmpgkologjapikonghg/background.js (3)
[2422968:2422968:0506/162437.798990:WARNING:service_worker_task_queue.cc(237)] AddObserver
[2422968:2422968:0506/162437.806216:WARNING:service_worker_task_queue.cc(285)] RemoveObserver
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Istiaque Ahmed.
1 comment:
File extensions/browser/service_worker_task_queue.cc:
Patch Set #20, Line 293:
partition->GetServiceWorkerContext()->RemoveObserver(this);In which scenario the above happens? I tried loading (or reloading) two extensions. […]
Yes, the above can happen. I added count to fix that problem.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Istiaque Ahmed (Gerrit)
Patchset:
One comment below. There are multiple red bots... could you also take care of those before the next round of review?
File extensions/browser/service_worker_task_queue.cc:
Patch Set #23, Line 286:
if (observers_count_ == 0)This is assuming for a particular BrowserContext, ServiceWorkerContext* will always be the same. Which isn't true in theory, see isolated storage note in [1].
Rather than relying on this assumption..., could we make the change accommodate that? lmk if you need some more background on this.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Istiaque Ahmed (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi.
3 comments:
Patchset:
I think I've found what the problem was: replying with that only. Aside, could you resolve all the "unresolved" comments that were resolved or reply what couldn't be resolved? It should help reducing distractions to review that way. Ty.
File extensions/browser/service_worker_task_queue.cc:
Patch Set #26, Line 89:
observed_worker_contexts_.erase(service_worker_context);AFAIK, this will invalidate the iterator.
I'd just call
for (auto* const service_worker_context : ...) {
service_worker_context->RemoveObserver(this);
}
And then clear |observing_worker_contexts_|. But we probably don't need to clear it explicitly cause we're in destructor, which will clear them anyway.Patch Set #26, Line 632:
for (auto* const service_worker_context : observed_worker_contexts_) {OnDestruct is called for particular |context| that is being destructed, so this method should only call context->RemoveObserver(this); and erase the corresponding context from |observed_worker_contexts_| -- And not all the contexts from the container.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Istiaque Ahmed.
3 comments:
Patchset:
Thanks Istiaque, this CL is now ready for the final review.
File extensions/browser/service_worker_task_queue.cc:
Patch Set #26, Line 89:
observed_worker_contexts_.erase(service_worker_context);
AFAIK, this will invalidate the iterator. […]
Thanks, this fixed the failing tests.
Patch Set #26, Line 632:
for (auto* const service_worker_context : observed_worker_contexts_) {
OnDestruct is called for particular |context| that is being destructed, so this method should only c […]
Done
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Istiaque Ahmed (Gerrit)
Attention is currently required from: Hiroki Nakagawa, Ghazale Hosseinabadi, Makoto Shimazu.
11 comments:
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
Patch Set #28, Line 6:
console.lg('test');nit: add a note here that this is intentional syntax error.
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/undefined_variable/service_worker_background.js:
Patch Set #28, Line 6:
console.log(undefined_variable);nit: ditto about commenting this line to make it clear.
File extensions/browser/service_worker_task_queue.h:
Patch Set #28, Line 129:
void OnReportConsoleMessage(int64_t version_id,We generally group overridden methods together (w/o whitespace separating them) and prefix them with the base class name it's overriding the methods from, e.g.
// content::ServiceWorkerContextObserver:
void OnReported...Patch Set #28, Line 209:
observed_worker_contexts_nit: |observing_worker_contexts_| is slightly more appropriate.
File extensions/browser/service_worker_task_queue.cc:
Patch Set #9, Line 653:
std::unique_ptr<ExtensionError> error_instance(I feel using `auto` is enough since the type is explicit at std::make_unique<>. […]
+1 for auto.
File extensions/browser/service_worker_task_queue.cc:
if (lazy_context_id.browser_context() != browser_context_)
return;
right, I deleted it.
Please mark the comments that are resolved by checking "Resolved" checkbox(es).
Patch Set #20, Line 293:
partition->GetServiceWorkerContext()->RemoveObserver(this);
Yes, the above can happen. I added count to fix that problem.
I'd update this comment to note that we're using multiset-s.
File extensions/browser/service_worker_task_queue.cc:
Patch Set #28, Line 243:
observed_worker_contexts_.end())nit: {} for multiline
Patch Set #28, Line 270:
DidStopServiceWorkerContextWere you able to check renderer shutdown scenario I mentioned before? i.e. What happens if one worker is terminated (by killing the worker process) and we don't see the DidStopServiceWorkerContext IPC?
Patch Set #28, Line 294:
if (observed_worker_contexts_.find(service_worker_context) !=nit: {} for multiline
Patch Set #28, Line 300:
service_worker_context->RemoveObserver(this);{} for multiline.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Istiaque Ahmed.
15 comments:
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
Patch Set #28, Line 6:
console.lg('test');nit: add a note here that this is intentional syntax error.
Done
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/undefined_variable/service_worker_background.js:
Patch Set #28, Line 6:
console.log(undefined_variable);nit: ditto about commenting this line to make it clear.
Done
File extensions/browser/service_worker_task_queue.h:
Patch Set #28, Line 129:
void OnReportConsoleMessage(int64_t version_id,
We generally group overridden methods together (w/o whitespace separating them) and prefix them with […]
Done
Patch Set #28, Line 209:
observed_worker_contexts_nit: |observing_worker_contexts_| is slightly more appropriate.
Done
File extensions/browser/service_worker_task_queue.cc:
Patch Set #9, Line 653:
std::unique_ptr<ExtensionError> error_instance(
+1 for auto.
Done
Patch Set #9, Line 660:
logging::LOGGING_ERRORShould we respect mesasge. […]
Done
File extensions/browser/service_worker_task_queue.cc:
ExtensionsBrowserClient::Get()->ReportError(browser_context_,
std::move(error));
sgtm.
Done
File extensions/browser/service_worker_task_queue.cc:
if (lazy_context_id.browser_context() != browser_context_)
return;
Please mark the comments that are resolved by checking "Resolved" checkbox(es).
Done
Patch Set #20, Line 293:
partition->GetServiceWorkerContext()->RemoveObserver(this);
I'd update this comment to note that we're using multiset-s.
Done
File extensions/browser/service_worker_task_queue.cc:
Patch Set #23, Line 286:
if (observers_count_ == 0)
This is assuming for a particular BrowserContext, ServiceWorkerContext* will always be the same. […]
Done
File extensions/browser/service_worker_task_queue.cc:
Patch Set #26, Line 89:
observed_worker_contexts_.erase(service_worker_context);
Thanks, this fixed the failing tests.
Done
File extensions/browser/service_worker_task_queue.cc:
Patch Set #28, Line 243:
observed_worker_contexts_.end())nit: {} for multiline
Done
Patch Set #28, Line 270:
DidStopServiceWorkerContext
Were you able to check renderer shutdown scenario I mentioned before? i.e. […]
I couldn't simulate a scenario that a worker is terminated, but I don't get a DidStopServiceWorkerContext IPC. I tried terminating the SW from chrome://inspect and also by closing an open devtools window. If we need more investigation here, can it be done in a future CL? since this CL has been under review for a long time.
Patch Set #28, Line 294:
if (observed_worker_contexts_.find(service_worker_context) !=nit: {} for multiline
Done
Patch Set #28, Line 300:
service_worker_context->RemoveObserver(this);{} for multiline.
Done
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Istiaque Ahmed (Gerrit)
Patchset:
I don't see any new patchset for this CL. Forgot to upload?
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Istiaque Ahmed.
1 comment:
Patchset:
I don't see any new patchset for this CL. […]
sorry, just uploaded the CL.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Istiaque Ahmed (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi.
11 comments:
Patchset:
The error reporting code in SWTQ that I looked into this round seems to be passing dummy values to ExtensionError/RuntimeError. It likely is necessary in some scenarios, but I couldn't figure the entire reasoning, mind explaining those a bit?
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #29, Line 93:
using ::testing::HasSubstr;Is this unused?
Patch Set #29, Line 453:
registration_observer.WaitForRegistrationStored();Is registration stored signal *necessary* for this test? i.e. would this fail (perhaps flakily) if we didn't observe this?
(applies also to the same code below at line 479).
Patch Set #29, Line 459:
extensionWe should also add ASSERT_TRUE(extension) before as we're accessing it here.
Patch Set #29, Line 462:
Uncaught chrome.test.failureIs there a way developers could see the actual syntax error?
console.lg is malformed or sth.
and would it be possible to test that?
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/module_service_worker_dynamic_import/service_worker_background.js:
Patch Set #29, Line 7:
importCould you explain this change here for my understanding? i.e. why was it changed from importScripts->import and which one is "dynamic" import?
File extensions/browser/service_worker_task_queue.cc:
Patch Set #28, Line 270:
DidStopServiceWorkerContextI couldn't simulate a scenario that a worker is terminated, but I don't get a DidStopServiceWorkerCo […]
I wanted to ensure we don't unexpectedly fail/crash in this case.
chrome://inspect only allows to stop the worker, I was talking about forcefully killing the worker process (for a running worker, use task manager to terminate the worker process). And then observe what notifications we see and ensure closing browser afterwards doesn't crash.
See if you can test this manually. However, my current speculation is that it *should* work. Also given that we have other tests that (sort of at least) exercises killing worker process, we might be okay.
File extensions/browser/service_worker_task_queue.cc:
Patch Set #29, Line 618:
auto error_instance(nit: we typically assign in this case, i.e. auto error_instance = std::make_unique...
Is it OK to be source empty here?
StackTrace(1,
StackFrame(message.line_number, 1,
base::UTF8ToUTF16(message.source_url.spec()), u"")),This doesn't look like it's the actual "trace", we're constructing a trace to contain one frame with the error?
I'd like that we comment it explicitly here if that's the case.
Patch Set #29, Line 625:
-1 /* render_view_id */, -1 /* render_process_id */Why is this OK to send dummy ids in actual/production code here?
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Istiaque Ahmed.
11 comments:
Patchset:
The error reporting code in SWTQ that I looked into this round seems to be passing dummy values to E […]
In ServiceWorkerTaskQueue::OnReportConsoleMessage, I do not have access to render_view_id and render_process_id. That is why I am passing -1. Is there a way to extraxt render_view_id */, -1 /* render_process_id in ServiceWorkerTaskQueue::OnReportConsoleMessage?
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #29, Line 93:
using ::testing::HasSubstr;Is this unused?
yes, deleted.
Patch Set #29, Line 453:
registration_observer.WaitForRegistrationStored();
Is registration stored signal *necessary* for this test? i.e. […]
The test does not fail without this line. I ran the test a couple of times and it passed. I deleted this observer for both the tests.
Patch Set #29, Line 459:
extensionWe should also add ASSERT_TRUE(extension) before as we're accessing it here.
Done
Patch Set #29, Line 462:
Uncaught chrome.test.failure
Is there a way developers could see the actual syntax error? […]
I only get "Uncaught chrome.test.failure" in this test. And as I checked ErrorConsole is the only method to get the error console. I tested the same error with an extension on chrome and the error that I get on chrome://extensions is "Uncaught TypeError: console.lg is not a function". So developers see the actual syntax error, but it seems to me that we can not test that.
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/module_service_worker_dynamic_import/service_worker_background.js:
Patch Set #29, Line 7:
import
Could you explain this change here for my understanding? i.e. […]
import() is the dynamic import. This was a mistake in another CL and I planned to fix it. Since this CL surfaces this error, I thought it is better to fix it in this CL (i.e., use import() instead of importScript()).
File extensions/browser/service_worker_task_queue.cc:
Patch Set #28, Line 270:
DidStopServiceWorkerContext
I wanted to ensure we don't unexpectedly fail/crash in this case. […]
I used pkill -9 to kill the worker process. With that I got DidStopServiceWorkerContext IPC and closing browser afterwards did not crash.
File extensions/browser/service_worker_task_queue.cc:
Patch Set #29, Line 618:
auto error_instance(nit: we typically assign in this case, i.e. auto error_instance = std::make_unique...
Done
Is it OK to be source empty here?
replaced it with message.source
StackTrace(1,
StackFrame(message.line_number, 1,
base::UTF8ToUTF16(message.source_url.spec()), u"")),
This doesn't look like it's the actual "trace", we're constructing a trace to contain one frame with […]
right, added a comment.
Patch Set #29, Line 625:
-1 /* render_view_id */, -1 /* render_process_id */Why is this OK to send dummy ids in actual/production code here?
I do not have access to render_view_id and render_process_id. That is why I am passing -1. I see the error being reported to chrome://extensions, that was why I thought it should be fine to pass -1 here.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Istiaque Ahmed.
1 comment:
File extensions/browser/service_worker_task_queue.cc:
Patch Set #29, Line 625:
-1 /* render_view_id */, -1 /* render_process_id */I do not have access to render_view_id and render_process_id. That is why I am passing -1. […]
I added comments for ender_view_id and render_process_id.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Istiaque Ahmed (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi.
5 comments:
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #29, Line 462:
Uncaught chrome.test.failureI only get "Uncaught chrome.test.failure" in this test. […]
That's likely due to how the test is structured: currently the error is surfaced from inside chrome.test.runTests. If we move it to global scope method, I can see the ~error from ErrorConsole.
Changing the JS to:
chrome.test.sendMessage('ready', ()=> {
syntaxError();
});var syntaxError = function() {
// Intentional syntax error.
console.lg('test');
};That should trickle the "TypeError: console.lg is not a function" error (sth similar) to |error_list| and we can verify the error's presence here.
It's important to verify specific error we're looking for here, as there's a chance that the actual error is masked and can potentially incorrectly fail. For example, changing the error to undefined variable error would still leave this test passing.
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #31, Line 457:
error_list[0]Should we also verify that the type() of this is correct, i.e. RUNTIME_ERROR?
Patch Set #31, Line 471:
"service_worker/worker_based_background/undefined_variable"));We can use the same extension as the previous test too. You can send two (nested) sendMessage to execute two error cases (and likely rename the tests to ServiceWorkerBasedBackgroundTest.RuntimeError):
service_worker_background.js:
chrome.test.sendMessage('ready1', ()=> {
chrome.test.sendMessage('ready2' ()=> {
undefinedVariableError();
});
syntaxError();
});Then from C++, you could install two ExtensionTestMessageListener-s to catch 'ready1'/'ready2', then Reply each of them.
File extensions/browser/service_worker_task_queue.cc:
Can we pass the correct parameter here?
Patch Set #31, Line 630:
-1 /* We do not have access to render_process_id here */Let the comment statement not imply that this is expected.
I'd rather file a bug with a TODO and explore how to retrieve the correct process id here. If service worker is running, then there must be a way to retrieve its process id from |version_id|.
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Istiaque Ahmed.
5 comments:
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #29, Line 462:
Uncaught chrome.test.failure
That's likely due to how the test is structured: currently the error is surfaced from inside chrome. […]
Done
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #31, Line 457:
error_list[0]
Should we also verify that the type() of this is correct, i.e. […]
Done
Patch Set #31, Line 471:
"service_worker/worker_based_background/undefined_variable"));
We can use the same extension as the previous test too. […]
I kept them as two separate extensions and tests. In my opinion, this way is more clear.
File extensions/browser/service_worker_task_queue.cc:
Can we pass the correct parameter here?
No, content::ConsoleMessage has only line_number.
Patch Set #31, Line 630:
-1 /* We do not have access to render_process_id here */
Let the comment statement not imply that this is expected. […]
Done
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Istiaque Ahmed (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi.
Patch set 32:Code-Review +1
8 comments:
Patchset:
Thanks, //extensions lgtm with comments below.
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #32, Line 443:
profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, true);Add a comment why setting prefs is necessary.
Patch Set #32, Line 455:
ASSERT_TRUE(extension);nit: This should go right after line 448 above.
Patch Set #32, Line 459:
ExtensionError::RUNTIME_ERRORSwap params: expected, actual:
EXPECT_EQ(RUNTIME_ERROR, error_list[0]->type());Patch Set #32, Line 459:
ASSERT_EQEXPECT_EQ is better/typical here: as the test could gracefully continue to run the next lines and potentially collect more errors that might be useful in case of test failure.
For previous line, ASSERT* is important because we cannot access error_list[0] if error_list didn't have an element in it.
Patch Set #32, Line 460:
ASSERT_THATditto: EXPECT_
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
nit: space before =
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/undefined_variable/service_worker_background.js:
ditto
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Arthur Sonzogni.
Ghazale Hosseinabadi would like Arthur Sonzogni to review this change.
[Extensions] Display runtime errors in chrome://extensions.
This CL makes code changes to display runtime errors on
chrome://extensions for service worker based extensions.
Bug: 1152202
Change-Id: I4e8979ddd35c0d17e14fefba3557e8740bdac5b9
---
M chrome/browser/extensions/service_worker_apitest.cc
M chrome/test/data/extensions/api_test/service_worker/worker_based_background/module_service_worker_dynamic_import/service_worker_background.js
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/manifest.json
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/undefined_variable/manifest.json
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/undefined_variable/service_worker_background.js
M content/browser/log_console_message.cc
M content/browser/log_console_message.h
M content/public/browser/BUILD.gn
A content/public/browser/console_message.cc
M content/public/browser/console_message.h
M extensions/browser/service_worker_task_queue.cc
M extensions/browser/service_worker_task_queue.h
13 files changed, 252 insertions(+), 29 deletions(-)
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Arthur Sonzogni.
1 comment:
Patchset:
Hi Arthur,
Could you please review my changes in console_message and log_console_message?
Thanks,
Ghazale
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Arthur Sonzogni.
5 comments:
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #32, Line 443:
profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, true);Add a comment why setting prefs is necessary.
Done
Patch Set #32, Line 455:
ASSERT_TRUE(extension);nit: This should go right after line 448 above.
Done
Patch Set #32, Line 459:
ASSERT_EQ
EXPECT_EQ is better/typical here: as the test could gracefully continue to run the next lines and po […]
Done
Patch Set #32, Line 459:
ExtensionError::RUNTIME_ERROR
Swap params: expected, actual: […]
Done
Patch Set #32, Line 460:
ASSERT_THATditto: EXPECT_
Done
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Istiaque Ahmed (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi, Arthur Sonzogni.
Patch set 33:Code-Review +1
2 comments:
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #33, Line 470:
profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, true);Add the comment regarding developer mode here too.
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
nit: space before =
(didn't see changes here)
Change to "() => {"
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Arthur Sonzogni.
2 comments:
File chrome/browser/extensions/service_worker_apitest.cc:
Patch Set #33, Line 470:
profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, true);Add the comment regarding developer mode here too.
Done
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js:
(didn't see changes here) […]
Done
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi.
Patch set 34:Code-Review +1
1 comment:
Patchset:
LGTM!
To view, visit change 2567672. To unsubscribe, or for help writing mail filters, visit settings.
Ghazale Hosseinabadi (Gerrit)
Attention is currently required from: Ghazale Hosseinabadi.
Patch set 34:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[Extensions] Display runtime errors in chrome://extensions.
This CL makes code changes to display runtime errors on
chrome://extensions for service worker based extensions.
Bug: 1152202
Change-Id: I4e8979ddd35c0d17e14fefba3557e8740bdac5b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2567672
Reviewed-by: Arthur Sonzogni <arthurs...@chromium.org>
Reviewed-by: Istiaque Ahmed <laz...@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhi...@chromium.org>
Commit-Queue: Ghazale Hosseinabadi <gha...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#892025}
---
M chrome/browser/extensions/service_worker_apitest.cc
M chrome/test/data/extensions/api_test/service_worker/worker_based_background/module_service_worker_dynamic_import/service_worker_background.js
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/manifest.json
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/syntax_error/service_worker_background.js
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/undefined_variable/manifest.json
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/undefined_variable/service_worker_background.js
M content/browser/log_console_message.cc
M content/browser/log_console_message.h
M content/public/browser/BUILD.gn
A content/public/browser/console_message.cc
M content/public/browser/console_message.h
M extensions/browser/service_worker_task_queue.cc
M extensions/browser/service_worker_task_queue.h
13 files changed, 254 insertions(+), 29 deletions(-)