1766909 - ObservableArray constructable stylesheets breaks Dark Reader add-on (Error: Accessing from Xray wrapper is not supported.)

Status: RESOLVED FIXED

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

Description

[Shane Hughes [:aminomancer]]

I'm still trying to work out details but here's what I noticed. Patch for bug 1766227 causes Dark Reader to fail to apply styles on some websites. I noticed it on phabricator which is the most obvious example I found.

To test you just need Nightly and to enable layout.css.constructable-stylesheets.enabled and go to https://phabricator.services.mozilla.com and then reload the page. It seems to succeed on initial load and fail on reload or history navigation (whereupon I guess it falls back to some baseline styles that make the page monochrome gray and white). If you force reload and skip cache (Shift+Accel+R) it seems to fail to apply the fallback styles too, resulting in the normal phabricator theme. You can also try building from D144547

Bisection: mozregression --addon .../[e-mail] --pref layout.css.constructable-stylesheets.enabled:true --arg="https://phabricator.services.mozilla.com" --good=99

Pushlog

Comment 1

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1766227

Comment 3

[Emilio Cobos Álvarez [:emilio]]

I don't see any of the adopted stylesheets code in DocumentOrShadowRoot.cpp getting hit, which makes this particularly hilarious.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Ok, so this took a ridiculous amount of time to find (well, since my last comment), because dark reader is catching all the exceptions. But the TLDR is that accessing adoptedStyleSheets from an extension now throws:

Error: Accessing from Xray wrapper is not supported.

And that breaks the extension awfully since they do expect adoptedStyleSheets to work.

Edgar, Peter, do you know if we can relax this restriction?

Maybe we can turn the ObservableArray into a plain sequence on getting, at least? Or should I just send a patch to the darkreader add-on to wrap observableArray access in a try-catch? That could probably do for now, but it will bite us in the future, I suspect...

Comment 5

[Emilio Cobos Álvarez [:emilio]]

[Tracking Requested - why for this release]: If this is not easy to fix or work around we should consider not shipping this feature in 101.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1766909 - Wrap ObservableArray proxy on getter, rather than throwing. r=peterv,edgar

This will still prevent them from accessing stuff (.length will be
undefined, etc), but seems better than unexpectedly throwing. This fixes
the issue at hand at least.

With this patch, we reject length accesses here:

https://searchfox.org/mozilla-central/rev/86c98c486f03b598d0f80356b69163fd400ec8aa/js/xpconnect/wrappers/XrayWrapper.cpp#229-233

Your call on whether this patch is enough as-is, or more work is needed.
Also your call on whether if more work is needed that needs to happen on
this bug or somewhere else.

I'm not sure what we'd need to do to support this more "properly",
presumably we'd need to add special XRay support to
ObservableArrayProxyHandler or so? Pointers (or patches of course ;))
welcome.

Also unsure about the setter situation, I think it's fine not to throw
given the code I read, but please sanity-check.

Comment 8

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1766227

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bindings diff.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Attachment: DocumentBinding.cpp after patch.

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1766909 - Assert we have a backing object in non-getter/setters. r=peterv

Instead of generating complicated code to deal with something that can't
happen.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8eab3bfc4ef
Wrap ObservableArray proxy on getter, rather than throwing. r=peterv

Comment 14

[bszekely]

https://hg.mozilla.org/mozilla-central/rev/e8eab3bfc4ef

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this ready for a Beta approval request?

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9274364 [details]
Bug 1766909 - Wrap ObservableArray proxy on getter, rather than throwing. r=peterv,edgar

Beta/Release Uplift Approval Request

• User impact if declined: None known (the relevant add-on was fixed), but potential fallout from other add-ons.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple patch returning an opaque object rather than throwing.
• String changes made/needed: none
• Is Android affected?: Unknown

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9274364 [details]
Bug 1766909 - Wrap ObservableArray proxy on getter, rather than throwing. r=peterv,edgar

Approved for 101.0b8.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6107881b08b1