Closed Bug 1697829 Opened 3 years ago Closed 3 years ago

Dark Reader extension only partially working after Bug 1360715

Categories

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

Firefox 88
defect

Tracking

()

RESOLVED FIXED
89 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox86 --- unaffected
firefox87 --- unaffected
firefox88 --- disabled
firefox89 --- fixed

People

(Reporter: ke5trel, Assigned: saschanaz)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files)

STR:

  1. Install Dark Reader extension on latest Nightly 88.0a1.
  2. Visit https://github.com/darkreader/darkreader/issues/5182.

Background should be fully dark but is mostly light. Workaround is to change dom.webidl.crosscontext_hasinstance.enabled to true.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=74f818e48e1a30d4062a56309609fa34cb3d2038&tochange=182c7bf906b0fa04c49fdf85e9f2096ac1e8d421

Regressed by Bug 1360715.

I see Dark Reader is using instanceof heavily. Will try debugging...

Assignee: nobody → krosylight

NIing :rpl about content scripts in devtools, for better visibility 😉:

I don't see any moz-extension:// in the tree. Has there any relevant change there?

Flags: needinfo?(lgreco)

(In reply to Kagami :saschanaz from comment #2)

NIing :rpl about content scripts in devtools, for better visibility 😉:

As I promised on Matrix yesterday I double-checked why I didn't have any issue on getting the dark reader content script listed in the debugging panel while you were not seeing the extensions content script listed in that panel.
This debugging panel issue is a bit off-topic in this bug, and so I opted to file a separate (Bug 1698068), in the first comment on that bug I described in more detail how to make it work (and the bug will also serve the purpose of evaluating with the devtools team how to improve that in the long run, in the short run I'll update the doc page to point out the setting that need to be enabled).

Flags: needinfo?(lgreco)

Hi Kagami,
Based on a quick look to this part of Bug 1360715 - Part 1: Hide @@hasInstance for IDL interfaces behind a flag r=edgar it looks that we are still keeping the previous behavior of instanceof for chrome callers.

would it be reasonable from your perspective to keep the old behavior also for the extensions content scripts?

To be fair, It may be actually even better if we could keep the old behavior also for expanded principals that only include the webpage principal and not the addon principal one because, besides the content script (that do use an expanded principal including both webpage and extension principals), we do also support a userScripts webextensions API that uses sandboxes with an expanded principal that does only include the webpage principal.

That would likely be enough to be able to leave the new behavior introduced by Bug 1360715 to ride the train without triggering a regression in multiple extensions that may be doing that (I think that Dark Reader is going to be only one of the extensions that are going to be affected by this and expecting it to work, also considering that in Chrome the content scripts are using the Chrome-specific "isolated world" mechanisms which different enough from our xrays mechanism and I'm guessing that instanceof is working from content scripts as if it is called from the webpage js code).

Flags: needinfo?(krosylight)

👍 devtools.chrome.enabled does the work for me!

it looks that we are still keeping the previous behavior of instanceof for chrome callers.

would it be reasonable from your perspective to keep the old behavior also for the extensions content scripts?

The plan is to disable it also in chrome callers (bug 1695435) since we don't want an implicit behavior difference here between contexts. ((InterfaceName).isInstance() will be kept for chrome context and should be preferred for the cross-context check purpose.)

Deferring the change for extensions could make sense, but I'm not sure why this is a problem yet in Dark Reader since I couldn't detect any instanceof failure in inject/index.js from my debugging. Do you have a good theory here?

Flags: needinfo?(krosylight) → needinfo?(lgreco)

Deferring the change for extensions could make sense, but I'm not sure why this is a problem yet in Dark Reader since I couldn't detect any instanceof failure in inject/index.js from my debugging. Do you have a good theory here?

Not yet unfortunately, I did just took a pretty quick look but I wasn't able to quickly identify which one of the many instanceof usages in the injected scripts is the one that is making it to not work correctly, I'll need to dig a bit more.

I will take another look, but I'm still a bit concerned that there may be more than one case that could lead extensions to not work correctly (and they could break in a way that is less visible than how Dark Reader breaks) and that's the reason why I was asking if we could start by exempting the extensions from the new behavior (even through another pref, so that we could keep dig into it and evaluate if we could extend the same behavior to all contexts in a near future).

If we are going to deprecate the previous behavior for the extensions, we still have give developers enough time to adapt their extension (our current WebExtensions/DeprecationPolicy does mention 3 Firefox releases between the announcement and the full official deprecation).

Flags: needinfo?(lgreco)

I will take another look, but I'm still a bit concerned that there may be more than one case that could lead extensions to not work correctly (and they could break in a way that is less visible than how Dark Reader breaks) and that's the reason why I was asking if we could start by exempting the extensions from the new behavior (even through another pref, so that we could keep dig into it and evaluate if we could extend the same behavior to all contexts in a near future).

Yeah, we definitely cannot introduce the change to stable in this stage. I want to know what specifically is causing the issue before making a decision here. I'll also take a look.

Okay, I got a failing line: it's iterateCSSRules() in inject/index.js, where CSSRule === window.CSSRule becomes false. But it's weird:

  1. When I add a breakpoint there and refresh, it initially evaluates to true.
  2. And then when I switch to the light mode and then back to dark mode (within the extension), it becomes false.
  3. But Element === window.Element is always true and same for StyleSheet/CSSRuleList/etc.

So it seems CSSRule is somehow redefined, but not sure why it happens. Is this some kind of webextension-specific dynamic sandboxing, or could it be another bug in IDL side?

Does anything here give an idea to you?

Flags: needinfo?(lgreco)

Ran the mozregression tool, for this, and this is what mine came up with on my machine:

2021-03-13T09:40:24.159000: DEBUG : Found commit message:
Bug 1360715 - Part 3: Remove remaining cross-context instanceof from tests r=edgar

Differential Revision: https://phabricator.services.mozilla.com/D106663

NIing Edgar for possible IDL issue in comment #8.

This is more noticebable in Pocket pages; the initial loading applies black background but disabling-and-enabling-again does not.

Flags: needinfo?(echen)

(In reply to Kagami :saschanaz from comment #8)

Okay, I got a failing line: it's iterateCSSRules() in inject/index.js, where CSSRule === window.CSSRule becomes false. But it's weird:

  1. When I add a breakpoint there and refresh, it initially evaluates to true.
  2. And then when I switch to the light mode and then back to dark mode (within the extension), it becomes false.
  3. But Element === window.Element is always true and same for StyleSheet/CSSRuleList/etc.

So it seems CSSRule is somehow redefined, but not sure why it happens. Is this some kind of webextension-specific dynamic sandboxing, or could it be another bug in IDL side?

Does anything here give an idea to you?

I haven't be able to verify it, but an idea that did come to my mind is that maybe the extension are iterating over some rules that the extension has injected by itself and some rules that are coming from the original webpage and that instanceof may behave differently in the two cases.

one question:
is CSSRule === window.CSSRule what you typed in the webconsole while investigating the issue or an expression that is actually part of the DarkReader content script? (I did look for it, but I wasn't able to find it, I did find some instanceof calls in iterateCSSRules and so I wanted to be sure I wasn't missing something).

Flags: needinfo?(lgreco)

is CSSRule === window.CSSRule what you typed in the webconsole while investigating the issue or an expression that is actually part of the DarkReader content script? (I did look for it, but I wasn't able to find it, I did find some instanceof calls in iterateCSSRules and so I wanted to be sure I wasn't missing something).

I typed it on web console, and instanceof CSSStyleRule is one of the actual check in iterateCSSRules() as you already saw. (The evaluation result of CSSStyleRule === window.CSSStyleRule also changes. 🤔)

https://searchfox.org/mozilla-central/rev/a848cde8a73065fcc41902ac58c65821eb9a44ca/dom/base/nsContentUtils.cpp#10330-10335

When Extensions run content scripts inside a sandbox, it uses sandboxPrototype to make them appear as though they're running in the scope of the page. So when a content script invokes postMessage, it expects the |source| of the received message to be the window set as the sandboxPrototype. This used to work incidentally for unrelated reasons, but now we need to do some special handling to support it.

This makes me think our implementation is designed to prevent this kind of bug, BTW.

Note for me: How to detect content script principal

Note for me: How to detect content script principal

The correct way to detect the content script principal is to use ContentScriptAddonPolicy. AddonPolicy only works for background pages and other addon pages.

Reminder that the soft-freeze is in two days.

This is Nightly-only so I guess soft-freeze is not too relevant here, right?

Anyway, my further debugging says line 2402-2403 in inject/index.js is causing the issue.

vars.insertRule(cssTextWithVariables);
varsRule = vars.cssRules[0];

Here, accessing vars.cssRules[0] immediately creates globalThis.CSSStyleRule and thus CSSStyleRule === window.CSSStyleRule becomes false. What weird here is that vars.insertRule() somehow creates a CSSStyleRule instance in the content script context (vars.cssRules[0].constructor === globalThis.CSSStyleRule) while vars is in the web page context (vars.constructor === window.CSSStyleSheet). I did a quick test with regular iframes but the same thing doesn't happen; calling .insertRule() from the outer window creates an instance in the iframe context.

Emilio, is insertRule() expected to behave as such, or do you have some idea about this?

Flags: needinfo?(emilio)

Here, accessing vars.cssRules[0] immediately creates globalThis.CSSStyleRule and thus CSSStyleRule === window.CSSStyleRule becomes false.

While wrapping the JS reflector of vars.cssRules[0], if the interface object doesn't yet be created on the associated global, we will create one.
It seems like we think the associated global of vars.cssRules[0] is content script context, instead of the web page context.
And the associated global is from https://searchfox.org/mozilla-central/rev/1a47a74bd5ba89f2474aa27c40bd478e853f3276/layout/style/Rule.h#102.

Yeah, the code Edgar linked to is correct. StyleSheet now has a mRelevantGlobal member that we might be able to use. The relevant code creating the sheet is:

    function getTempCSSStyleSheet() {
        if (tempStyle) {
            return tempStyle;
        }
        if (isCSSStyleSheetConstructorSupported) {
            tempStyle = new CSSStyleSheet();
            return tempStyle;
        }
        else {
            const tempStyleElement = document.createElement('style');
            document.head.append(tempStyleElement);
            tempStyle = tempStyleElement.sheet;
            document.head.removeChild(tempStyleElement);
            return tempStyle;
        }
    }

So, two things:

  • GetParentObject() is returning null, but maybe it shouldn't.
  • Does layout.css.constructable-stylesheets.enabled=true fix this?
Flags: needinfo?(emilio)
Attached file test.html

It also happens with regular iframes. Yeah, maybe CssRule should use mRelevantGlobal as associated global.

Flags: needinfo?(echen)

Does layout.css.constructable-stylesheets.enabled=true fix this?

It does 👍

Attachment #9209886 - Attachment description: Bug 1697829 - Use StyleSheet::mRelevantGlobal to get its associated doc r=emilio → Bug 1697829 - Get relevant global of the parent sheet in Rule::GetParentElement r=emilio
Attachment #9209886 - Attachment description: Bug 1697829 - Get relevant global of the parent sheet in Rule::GetParentElement r=emilio → Bug 1697829 - Get relevant global for disassociated CSS rule objects r=emilio
Attachment #9209886 - Attachment description: Bug 1697829 - Get relevant global for disassociated CSS rule objects r=emilio → Bug 1697829 - Part 1: Get relevant global for disassociated CSS rule objects r=emilio
Pushed by krosylight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef2d559eec61
Part 1: Get relevant global for disassociated CSS rule objects r=emilio
https://hg.mozilla.org/integration/autoland/rev/6f18d7009bb5
Part 2: Make sure mRelevantGlobal exists for constructed stylesheets r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28179 for changes under testing/web-platform/tests
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch
Upstream PR merged by moz-wptsync-bot

Please nominate this for Beta approval when you get a chance.

Flags: needinfo?(krosylight)

Although the underlying issue affected all versions, this specific regression was Nightly-only, so I think uplifting is not needed here. I guess we can safely mark version 88 as unaffected, does this make sense to you?

Flags: needinfo?(krosylight) → needinfo?(ryanvm)

Let's go with disabled. Thanks for the quick response.

Flags: needinfo?(ryanvm)
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: