[Extensions] Implement chrome.scripting.unregisterContentScripts [chromium/src : main]

Kelvin Jiang (Gerrit)

unread,

Jul 27, 2021, 5:23:05 AM7/27/21

to Devlin, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Devlin.

Kelvin Jiang would like Devlin to review this change.

[Extensions] Implement chrome.scripting.unregisterContentScripts

This CL implements chrome.scripting.unregisterContentScripts which
removes dynamically registered scripts from an extension. While there is
some handling for edge cases where register/unregister may be called for
a script with the same ID concurrently, extensions should still wait for
a previous operation to finish before starting a new one on a script
with a given ID.

Bug: 1054624
Change-Id: Ibab1d89b28183edd22ca8661608e9021fdf9d4b6
---
M chrome/browser/extensions/api/scripting/scripting_api.cc
M chrome/browser/extensions/api/scripting/scripting_api.h
M chrome/common/extensions/api/scripting.idl
A chrome/test/data/extensions/api_test/scripting/dynamic_scripts/inject_element_2.js
M chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js
M extensions/browser/extension_function_histogram_value.h
M extensions/browser/extension_user_script_loader.cc
M extensions/browser/extension_user_script_loader.h
M extensions/browser/user_script_loader.h
M extensions/common/user_script.cc
M extensions/common/user_script.h
M tools/metrics/histograms/enums.xml
12 files changed, 234 insertions(+), 5 deletions(-)

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

Kelvin Jiang (Gerrit)

unread,

Jul 27, 2021, 5:23:12 AM7/27/21

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Devlin.

Patch set 3:Commit-Queue +1

View Change

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

Devlin (Gerrit)

unread,

Jul 29, 2021, 8:24:17 PM7/29/21

to Kelvin Jiang, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Chromium LUCI CQ, Devlin, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Kelvin Jiang.

View Change

8 comments:

Patchset:
- Patch Set #3:
  Nice, Kelvin! This looks really good.
File chrome/browser/extensions/api/scripting/scripting_api.cc:
- Patch Set #3, Line 922: std::unique_ptr<api::scripting::UnregisterContentScripts::Params>
  optional: Could use `auto` here
- Patch Set #3, Line 931:
  Should we validate the IDs here? What about:
  - An invalid ID?
  - An ID that doesn't match a script?
File chrome/common/extensions/api/scripting.idl:
- Patch Set #3, Line 196:
```
Note: attempting to
    // execute multiple register/unregister calls on a given script ID may cause
    // undefined behavior.
```
  The way I read this, it sounds like if I do something like:
  await register(a);
  await unregister(a);
  await unregister(a);
  it will cause undefined behavior. But I think what this is getting at is: "This is an asynchronous operation, and if you try to do stuff synchronously, it can hit at any point during the operation". I think this mostly goes without saying (given it's true of most extension APIs), so I'd probably just omit it. (Unless I'm misinterpreting this comment and it's hinting at something else)
- Patch Set #3, Line 205: optional DOMString[] ids,
  Thought: What if we supported a filter here, too? For now, it's also just `ids`, but if we're going to support matching a filter, it seems like supporting it on removal will be straightforward. And then there's a bit more conceptual overlap / symmetry between unregisterContentScripts() and getContentScripts().
  WDYT?
File chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js:
- Patch Set #3, Line 320: async function unregisterScripts() {
  This is a really well-written test - clear and thorough. Nicely done!
- Patch Set #3, Line 369: },
  One last bit (perhaps as a separate test) - removing a non-existent ID? (Should it throw an error?)
File extensions/browser/extension_user_script_loader.cc:
- Patch Set #3, Line 306: !UserScript::IsIDGenerated(id)
  This seems like something that should be at the API function level, and throw an error to the extension. (And then here, we can DCHECK this)
  More broadly, I think it'd be better to take all this matching into the API function, and then have this class remain pretty simple (basically, boil this method down to a few DCHECKs and lines 320 + 321).

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

Kelvin Jiang (Gerrit)

unread,

Aug 2, 2021, 5:23:29 PM8/2/21

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Chromium LUCI CQ, Devlin, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Devlin.

Patch set 5:Commit-Queue +1

View Change

6 comments:

File chrome/browser/extensions/api/scripting/scripting_api.cc:

- Patch Set #3, Line 922: std::unique_ptr<api::scripting::UnregisterContentScripts::Params>
  optional: Could use `auto` here

- Done

- Patch Set #3, Line 931:

- Should we validate the IDs here? What about: […]
  invalid id (i.e. starts with underscore): we should throw an error,. (trying to filter out these ids but still not delete anything else is awkward to implement)
  id does not match script: the id will be ignored (no scripts are removed if no ids match a script)
File chrome/common/extensions/api/scripting.idl:

Patch Set #3, Line 196:

Note: attempting to
    // execute multiple register/unregister calls on a given script ID may cause
    // undefined behavior.

- The way I read this, it sounds like if I do something like: […]
  Done

- Patch Set #3, Line 205: optional DOMString[] ids,

- Thought: What if we supported a filter here, too? For now, it's also just `ids`, but if we're going […]
  ack, using a filter
File chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js:
- Patch Set #3, Line 369: },

- One last bit (perhaps as a separate test) - removing a non-existent ID? (Should it throw an error?)

- added a test for this. the id should be ignored imo
File extensions/browser/extension_user_script_loader.cc:

- Patch Set #3, Line 306: !UserScript::IsIDGenerated(id)

- This seems like something that should be at the API function level, and throw an error to the extens […]
  Done

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

Devlin (Gerrit)

unread,

Aug 3, 2021, 8:56:27 PM8/3/21

to Kelvin Jiang, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Chromium LUCI CQ, Devlin, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Kelvin Jiang.

View Change

8 comments:

Patchset:
- Patch Set #5:
  Thanks, Kelvin!
File DEPS:
- Patch Set #5, Line 1392: Var('android_git') + '/platform/external/perfetto.git' + '@' + 'a8652744862070b229ae3ac3681d3bdaebf38e56',
  git merge error?
File chrome/browser/extensions/api/scripting/scripting_api.cc:
- Patch Set #3, Line 931:
  invalid id (i.e. starts with underscore): we should throw an error,. […]
  Can you describe why ignoring the ID is better? If developers want to remove all IDs, they just don't specify any, so naively it seems like if they specify a non-existent one, it's likely a mistake. We also have the list of pending dynamic script loads, so it seems like checking them should be pretty straightforward
File chrome/browser/extensions/api/scripting/scripting_api.cc:
- Patch Set #5, Line 925: std::unique_ptr<api::scripting::ContentScriptFilter>&
  nit1: prefer using pointers over smart pointer references when possible
  nit2: this can be const
  `const api::scripting::ContentScriptFilter*`
File chrome/common/extensions/api/scripting.idl:
- Patch Set #5, Line 202: [nodoc, supportsPromises] static void unregisterContentScripts(
  We should restrict this in the features file, too [1], just to be thorough
  [1] https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/extensions/api/_api_features.json;l=749;drc=4e5e089697a8876ceb0cc186a5b8a0cd5c1f55ad
File chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js:
- Patch Set #5, Line 382: chrome.scripting.unregisterContentScripts({ids: [scriptId]}),
  nit: indent +2
File extensions/browser/extension_user_script_loader.cc:
- Patch Set #5, Line 301: ids_to_remove
  const &
- Patch Set #5, Line 412: std::set<std::string>
  const &

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

Kelvin Jiang (Gerrit)

unread,

Aug 4, 2021, 7:05:00 PM8/4/21

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Chromium LUCI CQ, Devlin, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Devlin.

Patch set 6:Commit-Queue +1

View Change

7 comments:

File DEPS:

- Patch Set #5, Line 1392: Var('android_git') + '/platform/external/perfetto.git' + '@' + 'a8652744862070b229ae3ac3681d3bdaebf38e56',
  git merge error?

- Done
File chrome/browser/extensions/api/scripting/scripting_api.cc:

- Patch Set #3, Line 931:

- Can you describe why ignoring the ID is better? If developers want to remove all IDs, they just don […]
  as discussed offline, an error will be throw for a nonexistent id
File chrome/browser/extensions/api/scripting/scripting_api.cc:

- Patch Set #5, Line 925: std::unique_ptr<api::scripting::ContentScriptFilter>&

- nit1: prefer using pointers over smart pointer references when possible […]
  Done
File chrome/common/extensions/api/scripting.idl:

- Patch Set #5, Line 202: [nodoc, supportsPromises] static void unregisterContentScripts(

- We should restrict this in the features file, too [1], just to be thorough […]
  Done
File chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js:

- Patch Set #5, Line 382: chrome.scripting.unregisterContentScripts({ids: [scriptId]}),
  nit: indent +2

- Done
File extensions/browser/extension_user_script_loader.cc:

- Patch Set #5, Line 301: ids_to_remove
  const &

- Done

- Patch Set #5, Line 412: std::set<std::string>
  const &

- Done

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

Devlin (Gerrit)

unread,

Aug 4, 2021, 7:51:35 PM8/4/21

to Kelvin Jiang, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Kelvin Jiang.

Patch set 6:Code-Review +1

View Change

3 comments:

Patchset:
- Patch Set #6:
  LGTM!!
File chrome/browser/extensions/api/scripting/scripting_api.cc:
- Patch Set #6, Line 939: else if
  nit: no else after return:
```
if (IsIDGenerated(id)) {
  return ...;
}
```
```
if (!base::Contains(...)) {
  return ...;
}
```
File chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js:
- Patch Set #6, Line 396: chrome.test.succeed();
  Let's also test that this is an atomic operation, and doesn't remove any scripts if an error is thrown:
  await register('foo1');
  await assertPromiseRejects(unregister(['foo1', 'foo2']));
  assertEq(['foo1'], getRegisteredScripts());

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

Kelvin Jiang (Gerrit)

unread,

Aug 5, 2021, 6:04:41 AM8/5/21

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Patch set 7:Commit-Queue +2

View Change

2 comments:

File chrome/browser/extensions/api/scripting/scripting_api.cc:

- Patch Set #6, Line 939: else if

- nit: no else after return: […]
  Done
File chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js:

- Patch Set #6, Line 396: chrome.test.succeed();

- Let's also test that this is an atomic operation, and doesn't remove any scripts if an error is thro […]
  Done

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

Chromium LUCI CQ (Gerrit)

unread,

Aug 5, 2021, 7:50:22 AM8/5/21

to Kelvin Jiang, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin, Chromium Metrics Reviews, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Devlin: Looks good to me
  Kelvin Jiang: Commit

[Extensions] Implement chrome.scripting.unregisterContentScripts

This CL implements chrome.scripting.unregisterContentScripts which
removes dynamically registered scripts from an extension. While there is
some handling for edge cases where register/unregister may be called for
a script with the same ID concurrently, extensions should still wait for
a previous operation to finish before starting a new one on a script
with a given ID.

Bug: 1054624
Change-Id: Ibab1d89b28183edd22ca8661608e9021fdf9d4b6

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3054297
Commit-Queue: Kelvin Jiang <kelvi...@chromium.org>
Reviewed-by: Devlin <rdevlin...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#908831}


---
M chrome/browser/extensions/api/scripting/scripting_api.cc
M chrome/browser/extensions/api/scripting/scripting_api.h

M chrome/common/extensions/api/_api_features.json


M chrome/common/extensions/api/scripting.idl
A chrome/test/data/extensions/api_test/scripting/dynamic_scripts/inject_element_2.js
M chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js
M extensions/browser/extension_function_histogram_value.h
M extensions/browser/extension_user_script_loader.cc
M extensions/browser/extension_user_script_loader.h
M extensions/browser/user_script_loader.h
M extensions/common/user_script.cc
M extensions/common/user_script.h
M tools/metrics/histograms/enums.xml

13 files changed, 305 insertions(+), 6 deletions(-)

6 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/api/scripting/scripting_api.cc
Insertions: 3, Deletions: 1.

```
@@ -938:939, +938:941 @@
-        } else if (!base::Contains(existing_script_ids, id)) {
+        }
+  
+        if (!base::Contains(existing_script_ids, id)) {

```
The name of the file: chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js
Insertions: 20, Deletions: 3.

```
@@ -391:392, +391:402 @@
-      const scriptId = 'NONEXISTENT';
+      const validId = 'inject_element_1';
+      var scripts = [{
+        id: validId,
+        matches: ['*://*/*'],
+        js: ['inject_element.js'],
+        runAt: 'document_end'
+      }];
+  
+      await chrome.scripting.registerContentScripts(scripts);
+  
+      const nonexistentId = 'NONEXISTENT';

@@ -393:395, +403:412 @@
-          chrome.scripting.unregisterContentScripts({ids: [scriptId]}),
-          `Error: Nonexistent script ID '${scriptId}'`);
+          chrome.scripting.unregisterContentScripts(
+              {ids: [validId, nonexistentId]}),
+          `Error: Nonexistent script ID '${nonexistentId}'`);
+  
+      // UnregisterContentScripts should be a no-op if it fails.
+      scripts = await chrome.scripting.getRegisteredContentScripts();
+      chrome.test.assertEq(1, scripts.length);
+      chrome.test.assertEq(validId, scripts[0].id);
+  

```

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

Reply all

Reply to author

Forward