1439153 - [FIX] Make WebExtensions work with Shadow DOM/WebComponents

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[mrmr1993]

Currently, WebExtensions get the same access to shadow DOMs as the site's own javascript. For WebExtensions not tailored to a (few) individual site(s), this stops them from behaving as expected.

For closed shadow DOMs, WebExtensions can't access the contents of the shadow DOM at all:
* Any content that is in a closed shadow root can't be read/modified/removed, effectively blocking the extension's access to parts of pages, for no user-visible reason.
  - This affects Vimium-ff (can't click links/scroll/search for text etc. with shortcuts in this content), uBlock Origin (can't hide elements/block frames in the content), any text-to-speech addons (can't get text from the content), any styling addons (can't change font-size/font-color/font-family or resize images, etc.), and nearly every other extension that uses page content in some way.
* Site owners can use this to effectively block extensions from accessing the whole page, by wrapping the entire content in a shadow DOM.
  - This will be appealing to some ad-supported publishers, since it will stop ad-blocking extensions from working correctly on their pages.
  - As a side-effect, all but the most trivial extensions will be broken or useless on these pages.
* The true target element of events is hidden.
  - WebExtensions that handle these events may incorrectly handle (or not) events based on this mis-targeting.
  - This affects Vimium-ff and similar extensions (e.g. keyboard shortcuts fire even when a contentEditable/<input>/<textarea> element is focused, if it's in a closed shadow DOM).
  - For open shadow DOMs, the true target is in event.path.

Treating closed shadow DOMs as open shadow DOMs in extension contexts would fix all of these.

Adding an API to access the shadow root (like the chrome API bug 1421568) would solve the most grevious issues. Extensions can then also find the true target of events, as long as they are careful to add event listeners to every shadow root that is the ancestor of the (true) active element before any events are emitted, but this is hard to get right (as for focus events in open shadow DOMs below).



For open shadow DOMs, WebExtensions have (potentially very) fragmented DOM access:
* WebExtensions don't know 'where' the shadow roots are without inspecting every element in the DOM.
  - Further, shadow roots can be nested, so every element of every shadow root also needs to be inspected.
* If the WebExtension is trying to locate a particular element/family of elements (e.g. links/text/images), the usual DOM APIs will not return elements in the shadow DOMs.
  - The WebExtension must inspect every element for a shadow DOM and run the DOM API on each to get the intended result.
  - This affects Vimium-ff and uBlock Origin in the obvious ways.
* WebExtensions can't detect focus moving between elements in a shadow DOM without adding a "focus"/"blur" event listener to each shadow root ancestor of the element. (This would also apply to an API to access closed shadow roots.)
  - This can become complicated (see e.g. [1]).
  - Specifically, it's easy to make a mistake and
    + add 'duplicate' listeners (that aren't truely the same function and so don't get merged into one), causing slow-downs and memory leaks,
    + handle events multiple times as they hit nested shadow roots' event listeners, or
    + miss an edge case and end up missing some events
* WebExtensions can't add any style consistently across the page without walking across the DOM into each shadow DOM.
* WebExtensions that don't look inside shadow DOMs will, obviously, miss chunks of the DOM.

In general, it seems that shadow DOMs are to help page/component developers manage abstractions and separation. For reading and modifying webpages in their entirety à la WebExtensions, I can't see that it provides anything of value for the problems it causes.

For me, the ideal (hard and complex) workaround would be to expose a composed/flattened DOM, possibly as the only DOM as toggled by a manifest entry, to WebExtensions.


Is there anything here that there could be movement on before shadow DOMs are enabled by default?

[1]: https://github.com/philc/vimium/issues/2504

Comment 1

[ExE Boss]

Access into the Shadow DOM is achieved using the Shadow-Piercing Descendant Combinator (`>>>`), implementation of which is tracked in bug 1117572.

# See also:
- https://drafts.csswg.org/css-scoping-1/#deep-combinator

Comment 2

[The 8472]

A (tedious) workaround for closed shadow roots would be to load the extension at document_start and intercept the attachShadow function on the ElementPrototype in the page context and then track which elements have shadow trees with a WeakMap.

Comment 3

[The 8472]

> For reading and modifying webpages in their entirety à la WebExtensions, I can't see that it provides anything of value for the problems it causes.

Webextensions can take advantage of shadow dom to inject content into the page in a less disruptive and less detectable manner than before. Emphasis on *less*, not *un-*

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

So we should somehow expose the stuff implemented in bug 1421568 to addons too.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Kris, you might know who would be the right person to take a look at this.

Comment 6

[Kris Maglione [:kmag]]

If it's just exposing bug 1421568 to extensions, that's easy enough. That would probably be a bug for me or zombie.

At this point, the easiest way to do it would probably be to add a function along the lines of `nsDocument::IsPrivilegedShadowDOMEnabled`. We could go the route of adding a `ChromeOrExtension` WebIDL attribute, but I've been avoiding that mostly to encourage people to expose things to extensions based on specific permissions rather than just whether or not they have an extension principal.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

This is one of the few blockers shipping Shadow DOM, so if anyone could take a look at this, great.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: shadow_dom_extensions.diff

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/eb442efc6451f100fa73bae50eb85fb793884de5
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb442efc6451f100fa73bae50eb85fb793884de5
remote: recorded changegroup in replication log in 0.057s



https://searchfox.org/mozilla-central/source/dom/base/test/test_bug1421568.html still passes with this

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8991476 [details] [diff] [review]
shadow_dom_extensions.diff

Review of attachment 8991476 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, aside from some nits. Thanks

::: caps/BasePrincipal.cpp
@@ +291,5 @@
> +    *aResult = true;
> +  } else if (Kind() == eExpandedPrincipal) {
> +    auto expanded = As<ExpandedPrincipal>();
> +    for (auto& prin : expanded->WhiteList()) {
> +      if (prin->GetIsAddonOrExpandedAddonPrincipal()) {

I think we can just do `*aResult = AddonPolicy() || ContentScriptAddonPolicy()`. The effect will be the same.

::: caps/nsIPrincipal.idl
@@ +324,5 @@
> +    /**
> +     * Returns true iff the principal is either an addon principal or
> +     * an expanded principal, which contains at least one addon principal.
> +     */
> +    [infallible] readonly attribute boolean isAddonOrExpandedAddonPrincipal;

We've been tending to put these kinds of properties on BasePrincipal lately, and devirtualizing them, since they tend to be used a lot by hot code.

I suppose this one shouldn't be called more than a few times per global, though...

::: dom/base/nsDocument.cpp
@@ +2620,5 @@
> +    MOZ_ASSERT(NS_IsMainThread());
> +    JS::Realm* realm = JS::GetCurrentRealmOrNull(aCx);
> +    MOZ_ASSERT(realm);
> +    JS::Compartment* c = JS::GetCompartmentForRealm(realm);
> +    nsIPrincipal* principal = xpc::GetCompartmentPrincipal(c);

Should be able to just use GetRealmPrincipal here. But `SubjectPrincipal(aCx)` would be better.

@@ +2622,5 @@
> +    MOZ_ASSERT(realm);
> +    JS::Compartment* c = JS::GetCompartmentForRealm(realm);
> +    nsIPrincipal* principal = xpc::GetCompartmentPrincipal(c);
> +    return principal &&
> +      (principal->GetIsSystemPrincipal() ||

nsContentUtils::IsSystemPrincipal is faster.

::: toolkit/components/extensions/test/xpcshell/test_ext_shadowdom.js
@@ +28,5 @@
> +      Preferences.reset(pref);
> +    }
> +  });
> +
> +  async function backgroundScript() {

Nit: Remove async.

@@ +34,5 @@
> +                            "Should have openOrClosedShadowRoot in Element in background script.");
> +    browser.test.sendMessage("backgroundScript");
> +  }
> +
> +  async function contentScript() {

Nit: Remove async.

@@ +56,5 @@
> +    },
> +  });
> +
> +  await extension.startup();
> +  await extension.awaitMessage("backgroundScript");

This isn't necessary. `startup()` is guaranteed not to resolve until the background script has finished running. We only use an explicit message when it has some async work to do before the parent side can continue.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: shadow_dom_extensions_3.diff

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62cb6ad78b9b
Make WebExtensions work with Shadow DOM/WebComponents, r=kmag

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc6298fa815
try to fix ESlint failures, CLOSED TREE, r=bustage

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/62cb6ad78b9b
https://hg.mozilla.org/mozilla-central/rev/2bc6298fa815

Comment 14

[mrmr1993]

Thanks for working on this, openOrClosedShadowRoot is a really nice solution for a lot of cases.

Is there any work going on for an always-piercing version of querySelector, or for an alternative to event.composedPath() that includes closed shadow DOMs? These are the main pain points left for extensions dealing with closed shadow DOMs.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Please don't reopen already fixed bugs.

There is no work happening to add /deep/ selector or anything like that. If you need such, please file a new bug.

Comment 16

[mrmr1993]

Apologies, I assumed that they should be kept here, since they were covered by my original bug report. I'll open new issues.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

It is unlikely that /deep/ would be added, but exposing something like event.composedTarget for addons is an option. We have that anyhow already for browser chrome code.
But please file bugs on the needed features, thanks.
(easier to track features per bug)

Comment 18

[mrmr1993]

> It is unlikely that /deep/ would be added

I didn't really mean /deep/, since extensions can't generally know ahead-of-time where pages will decide to put a shadow root. The actual proposal is to let pages query the composed tree as if it were the usual DOM, since the shadow roots will be unhelpful and/or meaningless to a lot of extensions.

I've opened bug 1475869 for this composedQuerySelector idea, and bug 1475870 for an event.composedPath() that includes closed shadow DOMs.

Comment 19

[Vlad Jiman]

Attachment: Verified

Verified using Firefox Nightly 63.0a1 (20180806220216) running on Windows 10 x64 bit OS and MacOS. The issue has been validated using the extensions from the description.

Comment 20

[mrmr1993]

Vlad what you're seeing is a known quirk of how Vimium generates its hints, and isn't related to this. Please have a look at the relevant issues in the Vimium issue tracker[0] (e.g. #2493 [1]), and open a new issue there if none of those are helpful.

[0]: https://github.com/philc/vimium/issues
[1]: https://github.com/philc/vimium/issues/2493