1751346 - Support accessing ObservableArray attributes from XrayWrappers / extension content scripts

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), task)

Comment 1

[diogo]

Hi, we're also having the issue mentioned on 1770592 at Walnut.

Basically, we're getting adoptedStyleSheets as Opaque and can't iterate over it.

Just adding this comment here as requested by @emilio on this Twitter thread.

Thanks again for the support!

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Edgar do you know how difficult this would be to implement? Any hints? It seems this causes some extensions to use some rather terrible workarounds like this, causing performance issues.

Comment 4

[Hsin-Yi Tsai (she/her)[:hsinyi]]

Talking with Edgar about this, unfortunately, we didn't have quick thoughts on the implementation or the complexity, and we were unable to allocate time to think about this thoroughly due to other higher priorities. I added this into the team backlog. Emilio/Rob, please let me know if my team should give this higher priority in our roadmap.

Comment 5

[th3rdtim]

Rob's workaround in #1827104 looks elegant but unfortunately now shadow.adoptedStyleSheets.push is not a function

Comment 6

[Rob Wu [:robwu]]

(In reply to th3rdtim from comment #5)

Rob's workaround in #1827104 looks elegant but unfortunately now shadow.adoptedStyleSheets.push is not a function

I edited the example; .wrappedJSObject was missing, now I added it.

Comment 7

[Rob Wu [:robwu]]

When I saw this issue at first, I thought that the array assigned to adoptedStyleSheets would somehow continue to be observed. That is not the case, as the following snippet demonstrates (in Firefox 150):

sheet = new CSSStyleSheet();
sheet.replaceSync("*{background:pink}");

a = document.adoptedStyleSheets;
b = [sheet];
document.adoptedStyleSheets = b;
console.log(document.adoptedStyleSheets === a); // still true, despite the "array" having been overwritten!
console.log(document.adoptedStyleSheets === b); // false
console.log(document.adoptedStyleSheets[0]=== b[0]); // true
document.adoptedStyleSheets = [];
console.log(document.adoptedStyleSheets === a); // still true, despite the "array" having been overwritten again!
console.log(a.__proto__); // null

Additionally, there is strict type validation:

document.adoptedStyleSheets = [null]
// Uncaught TypeError: Document.adoptedStyleSheets setter: Element of value being assigned is not an object.

document.adoptedStyleSheets = [{}]
// Uncaught TypeError: Document.adoptedStyleSheets setter: Element of value being assigned does not implement interface CSSStyleSheet.

document.adoptedStyleSheets=[Object.create(new CSSStyleSheet())]
// Uncaught TypeError: Document.adoptedStyleSheets setter: Element of value being assigned does not implement interface CSSStyleSheet.

These semantics should be feasible in Xrays.

Comment 8

[Rob Wu [:robwu]]

Attachment: Bug 1751346 - Unit tests for adoptedStyleSheets in content scripts

Comment 9

[Rob Wu [:robwu]]

Above, I added unit tests to verify the behavior of adoptedStyleSheets in extensions. The tests currently fail.
In the test I also added a minimal fixup to get mosts tests to work, by waiving the xrays (with the downside of page modifications to Array.prototype directly affecting the document.adoptedStyleSheets.push(sheet) call):

function patchASS(documentOrShadowRoot) {
  Object.defineProperty(documentOrShadowRoot, "adoptedStyleSheets", {
    configurable: true,
    get() {
      return this.wrappedJSObject.adoptedStyleSheets;
    },
    set(v) {
      v = globalThis.cloneInto(v, window, { wrapReflectors: true });
      this.wrappedJSObject.adoptedStyleSheets = v;
    },
  });
}
patchASS(document);
patchASS(ShadowRoot.prototype);

Now the next step is to figure out how to fix the implementation such that:

• web pages and content script operate on the same view of adoptedStyleSheets
• Array.prototype modifications from the web page do not affect the content script. That the following identities hold:
  • document.adoptedStyleSheets.wrappedJSObject.push === window.wrappedJSObject.Array.prototype.push
  • document.adoptedStyleSheets.push === Array.prototype.push

The entry point for the observable array getter is at GetObservableArrayBackingObject: https://searchfox.org/firefox-main/rev/c836d51da4179d7b456d02d22e2f40c8b1a9b3d7/dom/bindings/BindingUtils.cpp#3647-3653
I am aware that a linked bug's patch (https://phabricator.services.mozilla.com/D145045) improved the situation before, by returning an (opaque) wrapper instead of throwing outright.
The desired effect is for a XrayWrapper to be returned.

As for how that can be implemented? If only I knew...

Comment 10

[Rob Wu [:robwu]]

I think that the correct solution is to update GetXrayType to recognize observable arrays at and then return a xray handler that does the right thing, at https://searchfox.org/firefox-main/rev/c836d51da4179d7b456d02d22e2f40c8b1a9b3d7/js/xpconnect/wrappers/XrayWrapper.cpp#126,146 :

  if (mozilla::dom::IsObservableArrayProxy(obj)) {
    return XrayForOpaqueObject; // TODO: This is the current behavior. Replace with the right one
  }

Optimistically I tried XrayForDOMObject and XrayForJSObject. That triggered assertion failures.
I also tried NotXray. And that seemed to work ?!! Can't be that easy, right?

EDIT: While it caused some tests to pass (notably the document.adoptedStyleSheets[0] = sheet case), it did not fix all of them.

Comment 11

[Rob Wu [:robwu]]

Looks like the above gets all cases to work except for = [] assignment, due to the following error to be thrown:

Accessing from Xray wrapper is not supported.

from https://searchfox.org/firefox-main/rev/c836d51da4179d7b456d02d22e2f40c8b1a9b3d7/dom/bindings/Codegen.py#23595-23596

EDIT: The test_adoptedStyleSheets_protected_by_xrays test fails too, using NotXrays means that the page can interfere with the methods from the proxy, just like the JS-based work-around in comment 9.

Comment 12

[Rob Wu [:robwu]]

To get rid of the Xray wrapper error from comment 11, we need to unwrap the xray and enter its realm. Coincidentally an unrelated refactor landed recently that did just that.

To get the observable array semantics to work without page's modifications affecting the content script, I managed to get XrayWrapper to behave as desired by treating observable arrays as a JS xray, along with a special case to assume the Array prototype (since adoptedStyleSheets does not have a prototype).

It is strange that on the web, document.adoptedStyleSheets.__proto__ is null despite it effectively inheriting from Array.prototype. In Chrome, its __proto__ is Array.prototype as expected, FWIW.

Comment 16

[Rob Wu [:robwu]]

Attachment: Bug 1751346 - Add support for ObservableArray in XrayWrapper

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/068ce8f31336
https://hg.mozilla.org/integration/autoland/rev/af7981f80635
Add support for ObservableArray in XrayWrapper r=edgar,jandem,smaug
https://github.com/mozilla-firefox/firefox/commit/81d2b8cdaa3d
https://hg.mozilla.org/integration/autoland/rev/d76bde339f65
Unit tests for adoptedStyleSheets in content scripts r=rpl

Comment 18

[Rob Wu [:robwu]]

We usually don't document minor bugfixes in MDN, but given the impact on extensions, and the fact that the work-around (.wrappedJSObject) results in slightly less robust code (due to the ability for web pages to intercept API calls on objects after the use of .wrappedJSObject), I have put up a PR to document the fix in the 153 release notes on MDN:

https://github.com/mdn/content/pull/44274

Comment 19

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/af7981f80635
https://hg.mozilla.org/mozilla-central/rev/d76bde339f65