Add status code to service worker registration failure message [chromium/src : main]

Devlin Cronin (Gerrit)

unread,

Aug 3, 2022, 8:45:52 PM8/3/22

to Richard Zhang, chromium-a...@chromium.org, extension...@chromium.org, Istiaque Ahmed, Devlin Cronin, chromium...@chromium.org

Attention is currently required from: Richard Zhang.

View Change

13 comments:

Patchset:
- Patch Set #1:
  Thanks, Richard! This looks great!
  Somehow, Istiaque and I weren't in the "attention set" for the CL (maybe this is because `R=` in the commit message doesn't automatically add us?). Make sure to add us in; otherwise, we may never see it in our review queue : )
File chrome/browser/extensions/service_worker_apitest.cc:
- Patch Set #1, Line 445: Tests a service worker registration that throwing errors.
  nit: this isn't _quite_ right. Maybe:
  // Tests a service worker registration that fails due to the worker script
  // synchronously throwing a runtime error.
- Patch Set #1, Line 449: profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, true);
  nit: add a comment about why we do this:
  // The error console only captures errors if the user is in dev mode.
- Patch Set #1, Line 457:
  nit: add `ASSERT_TRUE(extension);`
- Patch Set #1, Line 458: error_console->SetReportingAllForExtension(extension->id(), true);
  This is actually racy - there's a chance that the error would be thrown before LoadExtension() returns, so the error reporting setting wouldn't actually be set by this call. If this call is necessary, we'd need to do it *before* calling LoadExtension(); however, that's tricky, too, because we don't have the extension ID at that point. There's workarounds for that, but...
  I don't _think_ we actually need this at all. The default reporting setting for all unpacked extensions is to report everything, and LoadExtension() will, by default, load any directory as an unpacked extension. So I think we can just delete this.
- Patch Set #1, Line 463: ASSERT_EQ
  nit: for this and the check below, we can use EXPECT_EQ(), which will allow the test to proceed. (We need ASSERT_EQ for the size check, because otherwise the test would potentially crash with out-of-bounds access)
- Patch Set #1, Line 463:
```
  ASSERT_EQ(
      error_list[0]->message(),
      std::u16string(u"Uncaught Error: lol"));
```
  I think the line wrapping here is a little off - did you run `git cl format`? (If so, this is fine, I'm just surprised)
- Patch Set #1, Line 468: std::u16string(
  nit: no need to construct a u16string here; just passing the u"" will be sufficient.
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/service_worker_registration_failure/manifest.json:
- Patch Set #1, Line 5: importing
  it doesn't actually import anything - it just directly throws the error.
  If you want to keep this line, we could say something like "Tests a service worker script that throws an error in registration". Alternatively, we don't need a description here at all, and can remove it.
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/service_worker_registration_failure/service_worker_background.js:
- Patch Set #1, Line 1: 2021
  2022 : )
- Patch Set #1, Line 7: "
  nit: prefer 'single quotes' in JS (just Google style)
- Patch Set #1, Line 5:
```
let color = '#3aa757';

throw new Error("lol");

chrome.runtime.onInstalled.addListener(() => {
  console.log('Default background color set to %cgreen', `color: ${color}`);
});
```
  the only important line here is throwing the error. Let's remove the others so as to not distract the future reader.
File extensions/browser/service_worker_task_queue.cc:
- Patch Set #1, Line 536:
```
    std::stringstream msg;
    msg << "Service worker registration failed. Status code: "
        << static_cast<int>(status_code);
```
  nit: we don't use stringstreams much in chromium. Instead, prefer:
  std::string msg = base::StringPrintf("Service worker registration failed. Status code: %d", static_cast<int>(status_code));

To view, visit change 3805456. To unsubscribe, or for help writing mail filters, visit settings.

Richard Zhang (Gerrit)

unread,

Aug 3, 2022, 11:19:20 PM8/3/22

to Richard Zhang, chromium-a...@chromium.org, extension...@chromium.org, Istiaque Ahmed, Devlin Cronin, chromium...@chromium.org

Attention is currently required from: Devlin Cronin, Richard Zhang.

View Change

13 comments:

Patchset:
- Patch Set #2:
  changes made according to comments in patch set 2.
File chrome/browser/extensions/service_worker_apitest.cc:

- Patch Set #1, Line 445: Tests a service worker registration that throwing errors.

- nit: this isn't _quite_ right. Maybe: […]
  Done

- Patch Set #1, Line 449: profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, true);

- nit: add a comment about why we do this: […]
  Done

- Patch Set #1, Line 457:
  nit: add `ASSERT_TRUE(extension);`

- Done

- Patch Set #1, Line 458: error_console->SetReportingAllForExtension(extension->id(), true);

- This is actually racy - there's a chance that the error would be thrown before LoadExtension() retur […]
  You are right. This was added when I was trying different things. Should be deleted.

- Patch Set #1, Line 463: ASSERT_EQ

- nit: for this and the check below, we can use EXPECT_EQ(), which will allow the test to proceed. […]
  Done

Patch Set #1, Line 463:

  ASSERT_EQ(
      error_list[0]->message(),
      std::u16string(u"Uncaught Error: lol"));

- I think the line wrapping here is a little off - did you run `git cl format`? (If so, this is fine, […]
  Done

- Patch Set #1, Line 468: std::u16string(
  nit: no need to construct a u16string here; just passing the u"" will be sufficient.

- Done
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/service_worker_registration_failure/manifest.json:

- Patch Set #1, Line 5: importing

- it doesn't actually import anything - it just directly throws the error. […]
  Done
File chrome/test/data/extensions/api_test/service_worker/worker_based_background/service_worker_registration_failure/service_worker_background.js:

- Patch Set #1, Line 1: 2021
  2022 : )

- Done

- Patch Set #1, Line 7: "
  nit: prefer 'single quotes' in JS (just Google style)

- Done

Patch Set #1, Line 5:

let color = '#3aa757';

throw new Error("lol");

chrome.runtime.onInstalled.addListener(() => {
  console.log('Default background color set to %cgreen', `color: ${color}`);
});

- the only important line here is throwing the error. […]
  Done
File extensions/browser/service_worker_task_queue.cc:

Patch Set #1, Line 536:

    std::stringstream msg;
    msg << "Service worker registration failed. Status code: "
        << static_cast<int>(status_code);

- nit: we don't use stringstreams much in chromium. Instead, prefer: […]
  Done

To view, visit change 3805456. To unsubscribe, or for help writing mail filters, visit settings.

Devlin Cronin (Gerrit)

unread,

Aug 5, 2022, 12:54:38 PM8/5/22

to Richard Zhang, chromium-a...@chromium.org, extension...@chromium.org, Devlin Cronin, Richard Zhang, Istiaque Ahmed, chromium...@chromium.org

Attention is currently required from: Richard Zhang.

Patch set 2:Code-Review +1

View Change

2 comments:

Patchset:
- Patch Set #2:
  LGTM % final nit. Thank you, Richard!
File chrome/browser/extensions/service_worker_apitest.cc:
- Patch Set #2, Line 469: u"Service worker registration failed. Status code: 15");
  nit: maybe add a quick comment that "15" is kErrorScriptEvaluateFailed.

To view, visit change 3805456. To unsubscribe, or for help writing mail filters, visit settings.

Richard Zhang (Gerrit)

unread,

Aug 5, 2022, 1:30:52 PM8/5/22

to chromium-a...@chromium.org, extension...@chromium.org, Devlin Cronin, Richard Zhang, Istiaque Ahmed, chromium...@chromium.org

View Change

1 comment:

File chrome/browser/extensions/service_worker_apitest.cc:

- Patch Set #2, Line 469: u"Service worker registration failed. Status code: 15");
  nit: maybe add a quick comment that "15" is kErrorScriptEvaluateFailed.

- Done

To view, visit change 3805456. To unsubscribe, or for help writing mail filters, visit settings.

Richard Zhang (Gerrit)

unread,

Aug 5, 2022, 4:30:24 PM8/5/22

to chromium-a...@chromium.org, extension...@chromium.org, Chromium LUCI CQ, Devlin Cronin, Richard Zhang, Istiaque Ahmed, chromium...@chromium.org

Attention is currently required from: Richard Zhang.

Patch set 3:Commit-Queue +2

View Change

To view, visit change 3805456. To unsubscribe, or for help writing mail filters, visit settings.

Richard Zhang (Gerrit)

unread,

Aug 6, 2022, 12:28:03 AM8/6/22

to chromium-a...@chromium.org, extension...@chromium.org, Tricium, Chromium LUCI CQ, Devlin Cronin, Richard Zhang, Istiaque Ahmed, chromium...@chromium.org

Attention is currently required from: Richard Zhang.

Patch set 4:Commit-Queue +2

View Change

To view, visit change 3805456. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Aug 6, 2022, 12:30:55 AM8/6/22

to Richard Zhang, chromium-a...@chromium.org, extension...@chromium.org, Tricium, Devlin Cronin, Richard Zhang, Istiaque Ahmed, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change



2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: chrome/browser/extensions/service_worker_apitest.cc
Insertions: 10, Deletions: 4.

@@ -463,10 +463,16 @@
   const ErrorList& error_list =
       error_console->GetErrorsForExtension(extension->id());
   ASSERT_EQ(kErrorsExpected, error_list.size());
-  EXPECT_EQ(error_list[0]->message(), u"Uncaught Error: lol");
-  EXPECT_EQ(
-      error_list[1]->message(),
-      u"Service worker registration failed. Status code: 15");
+
+  std::vector<std::u16string> error_message_list;
+  for (std::string::size_type i = 0; i < error_list.size(); i++) {
+    error_message_list.push_back(error_list[i]->message());
+  }
+  // status code 15: kErrorScriptEvaluateFailed
+  EXPECT_THAT(error_message_list,
+              testing::UnorderedElementsAre(
+                  u"Uncaught Error: lol",
+                  u"Service worker registration failed. Status code: 15"));
 }
 
 // Tests that an error is generated if there is a syntax error in the service
```

Approvals:
  Devlin Cronin: Looks good to me
  Richard Zhang: Commit

Add status code to service worker registration failure message

This helps developer understand why the service worker fails to register

Error message:
"Service worker registration failed. Status code: <code>", e.g.
"Service worker registration failed. Status code: 18"

R=laz...@chromium.org, rdevlin...@chromium.org

Bug: 1348391
Change-Id: I3a3af5056909593c57fe09f8396599aa2d43cd07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3805456
Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
Commit-Queue: Richard Zhang <rich...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032240}
---
M chrome/browser/extensions/service_worker_apitest.cc
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/service_worker_registration_failure/manifest.json
A chrome/test/data/extensions/api_test/service_worker/worker_based_background/service_worker_registration_failure/service_worker_background.js
M extensions/browser/service_worker_task_queue.cc
4 files changed, 74 insertions(+), 1 deletion(-)

To view, visit change 3805456. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward