Open Bug 1697404 Opened 1 month ago Updated 1 month ago

Webcompat reporter add-on loads Services.jsm in various ways on the experimental API global, leading to conflicts

Categories

(Web Compatibility :: Tooling & Investigations, defect)

Desktop
All
defect

Tracking

(Not tracked)

People

(Reporter: Gijs, Unassigned)

References

Details

can't redefine non-configurable property "Services" 2 helpMenu.js:9
    <anonymous> file://url/to/objdir/browser/features/webcompat-reporter@mozilla.org/experimentalAPIs/helpMenu.js:9
    asyncLoaded resource://gre/modules/ExtensionCommon.jsm:1667

:rpl pointed out this probably breaks because of a similar definition in tabExtras.js
:mixedpuppy pointed out we load a bunch of stuff by default, and could add Services there if it's not there already: https://searchfox.org/mozilla-central/rev/62ddcc03195036b355cb23b6f2e2b88d3f8099ad/toolkit/components/extensions/ExtensionCommon.jsm#1749

Summary: Webcompat reporter add-on loads Services.jsm in various ways in the experimental compartment, leading to conflicts → Webcompat reporter add-on loads Services.jsm in various ways on the experimental API global, leading to conflicts

I did double-check how Services is imported or lazily imported in all other API modules which are part of Firefox and we do have all these three variations:

All these three shouldn't be conflicting with each other because:

  • we do cover this by loading all WebExtensions API modules natively integrated in Firefox itself to be sure we don't have conflicts like this
  • and also because var { Services } ... doesn't prevent ChromeUtils.defineModuleGetter to redefine the same variable (and XPCOMUtils.defineLazyModuleGetter does use ChromeUtils.defineModuleGetter)

My guess is that there is another experimental API that is likely doing const { Services } = ChromeUtils.import(...), because I'm pretty sure that would trigger that error.

I do also suspect that the experimental API that is actually doing that may actually be out of the mozilla-central tree because:

  • it doesn't seem there is any code doing const { Services... in tree: https://searchfox.org/mozilla-central/search?q=const+%7B+Services&path=ext-* (to be fair if the statement is in a multiline form my quick search of searchfox may be missing it, but I'm not sure that would be something used in tree because Services.jsm doesn't export anything else and I would expect prettier to enforce that statement to be formatted on a single line)

  • I was initially able to reproduce it on today's vanilla mozilla-central tip on every ./mach run, but then while investigating it I wasn't able to reproduce it anymore (which made me think that the experimental API introducing the conflict may be actually something that instance got by other means (maybe a privileged addon installed through normandy?), and once it was gone (I have likely recreated the tmp profile from scratch a couple of times while investigating the issue) the issue wasn't triggered anymore

(I regret that I didn't look and save the about:support and about:studies details when I was able to reproduce it, so that I could then compare it to the one got after I wasn't able to reproduce it again).

Hi Gijs, can you still reproduce this?
would you mind to look in about:support and in about:studies for the list of extensions installed?
(and possibly also in the system addons locations to double-check if there is any addon not part of the build and not installed explicitly)

Flags: needinfo?(gijskruitbosch+bugs)

I digged into this a bit more after briefly discussing about it with Shane yesterday.

Given that I'm unable to reproduce the issue anymore (and I don't yet see why), I opted to manually verify which conditions may be able to trigger that issue in practice (hopefully based on this I may get some more ideas about how was it reproducible, and suddenly it wasn't anymore).

Based of the manual verification I did (described in more detail below):

  • the two conflicting experimental APIs have to be part of the same privileged extension
  • that error should be triggered if an API does define Services using var { Services } = ... first and then an experimental API loaded after that one does use ChromeUtils.defineLazyModule directly (and not through the XPCOMUtils helper).

Manually verified cases:

  • as I was originally assuming, a privileged extension's experimental API cannot introduce a conflict like this in an experimental API part of a different extension (and it can't introduce the same kind of conflict with a natively integrated API, and the opposite is also true).

  • a privileged extension's experimental API can introduce that kind of conflict with another experimental API that is part of that same privileged extension

  • var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); does create a Services property that isn't configurable (verified by looking to the property descriptor retrieved using Object.getOwnPropertyDescriptor)

  • if var { Services } = ... is part of the experimental API loaded first, and another experimental API loaded after that does use ChromeUtils.defineModuleGetter, that does trigger the error TypeError: can't redefine non-configurable property "Services" (when `chromeUtils.defineModuleGetter is being executed)

  • if the second experimental API does use XPCOMUtils.defineLazyModuleGetter/Getters, the same TypeError: can't redefine non-configurable property "Services" error will be trigger but the stack trace would differ (because the error is thrown from XPCOMUtils.jsm and not the experimental API module)

  • const { Services } = ... has different scoping behavior from the scoping that var { Services } ... does have, technically Services doesn't become a property of this but a binding in the lexical scope

  • if const { Services } = ... is part of the experimental API loaded first, and another experimental API loaded after that does use var { Services } ..., a conflict it is triggered by with a different error message: redeclaration of const Services

This bug was really trying to annoying me, I have been able to reproduce it again intermittently by switching between an older mozilla-central revision (I did go back to ae94c83c78f4) and the tip from today (I'm currently on bd8121e747b5) and put some more temporary logging in SchemaAPIManager's asyncLoadModule method and compare what is the order of receiving the asyncLoadModule call and the module actually being executed after the related ChromeUtils.compileScript call did resolve.

  • when the issue isn't triggered, the following is happening in this exact order:

    • asyncLoadModule call for "helpMenu" experimental API is received
    • asyncLoadModule call for "aboutConfigPrefs" experimental API is received
    • "helpMenu" script compilation is completed and the script is executed executed (in the experimental API global related to the webcompat-reporter extension)
    • "aboutConfigPrefs" script compilation is completed and it is executed in the same global right after the "helpMenu" one
    • no error is triggered (for the reasons described in comment 3, "helpMenu" does use ChromeUtils.defineModuleGetter and then "aboutConfigPrefs" override that with its var { Services } = ...
  • instead when the issue is triggered, the following is happening in this exact order:

    • asyncLoadModule call for "helpMenu" experimental API is received
    • asyncLoadModule call for "aboutConfigPrefs" experimental API is received
    • "aboutConfigPrefs" script compilation is completed earlier and it is executed first
    • "helpMenu" script compilation is completed a bit after "aboutConfigPrefs" and it is executed in that global after "aboutConfigPrefs" did
    • the error is triggered (for the reasons described in comment 3, "aboutConfigPrefs" defined var { Services } = ... and then "helpMenu" fails on ChromeUtils.defineModuleGetter)

I believe that the main reason why switching between those two commits increase the changes to trigger it may just be related to something that may be affecting the task scheduling (likely because the profile is being upgraded from two different builds), but that's just a theory and I don't have any actualy proof.

Anyway, to prevent this particular error in webcompat-reporter we may just land a patch to make sure that all its experimental APIs do define Services using the exact same way.

Separately from fixing the issue in webcompat-reporter it seems reasonable to see if we may improve this (as in "making it less risky") with some work on the WebExtensions internals side.

Flags: needinfo?(gijskruitbosch+bugs)

Dumb question: what would break if we added Services as a property on the global from within the webextensions API loader and removed all the getters/definitions in the in-tree system add-ons and rev'd their identifiers? Are there any other places where something would break / that we'd need to update?

(In reply to :Gijs (he/him) from comment #5)

Dumb question: what would break if we added Services as a property on the global from within the webextensions API loader and removed all the getters/definitions in the in-tree system add-ons and rev'd their identifiers? Are there any other places where something would break / that we'd need to update?

That is not really a dumb question from my perspective, it is a more than reasonable one given that Services is already imported all over the place in the API modules integrated and in the experimental ones, and it is imported in multiple potentially conflicting ways.

Based on the combination I tried and described in comment 3, I think that by defining Services as a lazy module getter when we create the global we shouldn't really increase the chances of this kind of issues.

It would not prevent extensions that live out of the tree from breaking themselves by still importing it and triggering the conflict, but that is not worst than the current situation and it would allow us to reduce the chances of conflicts in the future.

Obviously adding Services doesn't not resolve the issue in general, the experimental APIs may import other jsm files (but there should be a bit less chances to import the same one if it is a jsm that isn't as common as Services.jsm is).

I'm personally ok to add Services to the global.

I filed Bug 1698158 to add Services as a global provided by default to all WebExtensions API scripts, and then tack down and replace all the redundant imports, let's see how the push to try goes.

I'm adding Bug 1698158 as a see also for now, if we are going to proceed with landing that set of patches we may add it link it with this bug as a dependency and then close this (after we have double-checked that it cannot be reproduced anymore as we would expect).

See Also: → 1698158

Bug 1698158 is now on central and so I did just try to reproduce it to confirm that isn't triggered anymore as expected (and that I wasn't able to trigger some other issue that could have been previously hidden behind the Services conflict issue tracked here).

The issue was intermittent, but I was able to reproduce it by running an older Firefox build and a new one of the same test profile (as described in comment 4), and so I did try my STR a couple of times and no new error was triggered and "Report Site Issues..." menu item was also working as expected.

I think that we may close this issue (and eventually file a new one if we will notice any other intermittent issue on the webcompat reporter builtin extension), or have QA to also verify it explicitly if we prefer (but warning them about the intermittency of the original issue, and the fact that they would need to verify it as part of a profile upgraded between two Firefox version to increase the chances to trigger the intermittent error on the versions that are supposed to be broken).

Flags: needinfo?(kdubost)

I will let Thomas handle it.

Flags: needinfo?(kdubost) → needinfo?(twisniewski)

I can update the webcompat addons as needed to do the right thing, but what's the "right" way to import to prevent this? Just to use var?

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

(In reply to Thomas Wisniewski [:twisniewski] from comment #10)

I can update the webcompat addons as needed to do the right thing, but what's the "right" way to import to prevent this? Just to use var?

We actually fixed the conflict in Bug 1699234 (which exposed Services by default in the API script global and then removed the redefinitions of that global from all experimental APIs in tree) and so the only thing remaining here would be to independently verify the fix (I did verify it as I mentioned in comment 8, but I didn't close the issue as fixed in case you preferred to let QA to also verify it).

Flags: needinfo?(lgreco)
You need to log in before you can comment on or make changes to this bug.