Closed Bug 1793995 Opened 3 years ago Closed 3 years ago

Factor parts of WebExtensionPolicy into a threadsafe core type

Categories

(WebExtensions :: General, enhancement)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox107 fixed)

Status:
RESOLVED FIXED
Milestone:
107 Branch
Tracking Flags:
Tracking Status
firefox107 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(8 files)

Bug 1793995 - Part 1: Move immutable parts of WebExtensionPolicy to a threadsafe core type, r=kmag!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1793995 - Part 2: Make AtomSet immutable and threadsafe, r=kmag!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1793995 - Part 3: Remove the unused includeGlobs and excludeGlobs getters, r=kmag!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1793995 - Part 4: Split out the threadsafe core from MatchGlob, r=kmag!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1793995 - Part 5: Factor out the threadsafe core of MatchPattern and MatchPatternSet, r=kmag!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1793995 - Part 6: Allow looking up a WebExtensionPolicyCore from any thread, r=kmag!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1793995 - Part 7: Use threadsafe refcounting for WebAccessibleResource, r=kmag!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1793995 - Part 8: Move WebAccessibleResources into the threadsafe core, r=kmag!
48 bytes, text/x-phabricator-request
Details | Review

A way to reference a WebExtensionPolicy from any thread will be necessary to make nsIPrincipal threadsafe (bug 1443925). It will both be needed as part of implementing ContentPrincipal::AddonPolicy() as well as part of supporting NS_URIChainHasFlags on any thread (bug 1793463).

The particular fields which we want to be make threadsafe includes WebAccessibleResource in order to implement NS_URIChainHasFlags. This will require replacing the regex backend used by MatchGlob from using the JS engine, so that it can be used on any thread. This will require us to switch to the rust regex library (bug 1693271), as well as split out the WebIDL wrappers for the various types (MatchGlob, MatchPattern, MatchPatternSet), so that the core types can be made threadsafe.

Depends on: 1794012

This threadsafe core type also acts as a weak reference to the main-thread
WebExtensionPolicy when needed. This will be used when information about a
WebExtension is needed to be accessible off-main-thread in the future.

Depends on D158878

The mutator methods were never used, so were easy to remove, allowing the type
to be made threadsafe. The main potential for performance regression is that
the string based Contains method now uses NS_Atomize instead of
NS_AtomizeMainThread, however this is only called in once place
(WebExtensionPolicy::HasPermission). If it turns out to be an issue, we can
move the atomization into the caller to keep it using NS_AtomizeMainThread.

Depends on D158879

These getters are never called and will make making the core of MatchGlob
threadsafe more annoying.

Depends on D158880

The outer cycle-collected wrapper type is unfortunately still required by
WebIDL in order to keep the JS API working.

Depends on D158881

Similar to MatchGlob, this is fairly straightforward, but is complicated
slightly by MatchPatternSet, which allows being destructured into the
contained MatchPattern instances in a [Constant] method. To handle this a
cache is added for the wrappers in the cycle-collected MatchPatternSet.

Depends on D158882

In order to make WebAccessibleResource threadsafe, as well as other places, it
needs to be possible to look up a WebExtensionPolicyCore from any thread.
This is handled by using a static method on the ExtensionPolicyService for this
task, and keeping a seperate mutex-guarded static table under the hood.

Theoretically the table within ExtensionPolicyService() could also be
removed, however I held off on doing that in case it would have a negative
performance impact to take extra locks and follow extra pointers.

Depends on D158883

Now that all fields and methods in WebAccessibleResource have been made
threadsafe, we can make the type itself be threadsafe.

Depends on D158884

This will be required in the future to make getting protocol flags for
moz-extension:// URIs threadsafe.

Depends on D158885

Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5cb4daddadb9 Part 1: Move immutable parts of WebExtensionPolicy to a threadsafe core type, r=kmag https://hg.mozilla.org/integration/autoland/rev/06d7d4823419 Part 2: Make AtomSet immutable and threadsafe, r=kmag https://hg.mozilla.org/integration/autoland/rev/37ccdfa60f9a Part 3: Remove the unused includeGlobs and excludeGlobs getters, r=kmag https://hg.mozilla.org/integration/autoland/rev/f7d78fffc836 Part 4: Split out the threadsafe core from MatchGlob, r=kmag https://hg.mozilla.org/integration/autoland/rev/8484584cc787 Part 5: Factor out the threadsafe core of MatchPattern and MatchPatternSet, r=kmag https://hg.mozilla.org/integration/autoland/rev/61d74f2cf5cf Part 6: Allow looking up a WebExtensionPolicyCore from any thread, r=kmag https://hg.mozilla.org/integration/autoland/rev/3deec78af656 Part 7: Use threadsafe refcounting for WebAccessibleResource, r=kmag https://hg.mozilla.org/integration/autoland/rev/93fa076646e1 Part 8: Move WebAccessibleResources into the threadsafe core, r=kmag

Backed out for causing build bustages on RustRegex.h

Log tier 2: https://treeherder.mozilla.org/logviewer?job_id=392996025&repo=autoland

Flags: needinfo?(nika)
Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/16299839e25e Part 1: Move immutable parts of WebExtensionPolicy to a threadsafe core type, r=kmag https://hg.mozilla.org/integration/autoland/rev/d55a78f3627a Part 2: Make AtomSet immutable and threadsafe, r=kmag https://hg.mozilla.org/integration/autoland/rev/6773cada61a8 Part 3: Remove the unused includeGlobs and excludeGlobs getters, r=kmag https://hg.mozilla.org/integration/autoland/rev/264a0154514e Part 4: Split out the threadsafe core from MatchGlob, r=kmag https://hg.mozilla.org/integration/autoland/rev/a653699a5cef Part 5: Factor out the threadsafe core of MatchPattern and MatchPatternSet, r=kmag https://hg.mozilla.org/integration/autoland/rev/4330821df43f Part 6: Allow looking up a WebExtensionPolicyCore from any thread, r=kmag https://hg.mozilla.org/integration/autoland/rev/79a5023e7822 Part 7: Use threadsafe refcounting for WebAccessibleResource, r=kmag https://hg.mozilla.org/integration/autoland/rev/5d4b0c23342f Part 8: Move WebAccessibleResources into the threadsafe core, r=kmag

Backed out for causing build bustages

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: clang-14: error: linker command failed with exit code 1 (use -v to see invocation)
    gmake[4]: *** [/builds/worker/checkouts/gecko/config/makefiles/rust.mk:433: force-cargo-library-build] Error 101
Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/61417ba55b4c Part 1: Move immutable parts of WebExtensionPolicy to a threadsafe core type, r=kmag https://hg.mozilla.org/integration/autoland/rev/ef9dcb4d17d2 Part 2: Make AtomSet immutable and threadsafe, r=kmag https://hg.mozilla.org/integration/autoland/rev/f1d329c29295 Part 3: Remove the unused includeGlobs and excludeGlobs getters, r=kmag https://hg.mozilla.org/integration/autoland/rev/70fb967a388d Part 4: Split out the threadsafe core from MatchGlob, r=kmag https://hg.mozilla.org/integration/autoland/rev/9e563403fa06 Part 5: Factor out the threadsafe core of MatchPattern and MatchPatternSet, r=kmag https://hg.mozilla.org/integration/autoland/rev/6fb4337b5bc0 Part 6: Allow looking up a WebExtensionPolicyCore from any thread, r=kmag https://hg.mozilla.org/integration/autoland/rev/a891737169cc Part 7: Use threadsafe refcounting for WebAccessibleResource, r=kmag https://hg.mozilla.org/integration/autoland/rev/00ee36d3c47b Part 8: Move WebAccessibleResources into the threadsafe core, r=kmag
Flags: needinfo?(nika)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: