DistinctSystemPrincipalLoader can unexpectedly result in two instances of an ESM
Categories
(DevTools :: Framework, defect, P3)
Tracking
(Not tracked)
People
(Reporter: robwu, Unassigned)
References
Details
When the DevTools toolbox or remote debugging is used, a distinct module loader is used (source). This can result in two instances of the same ESM file, and is a problem for any ESM that is designed to be a singleton that is presumed to have been initialized elsewhere. Even worse issues may happen if such a module also synchronizes state with an external environment (whether native code or persistent files on disk), because that can result in data corruption and unstable behavior.
While DevTools can use the loadInDevToolsLoader: false option (from bug 1790383) to force direct imports within the devtools/ tree to be loaded through the main module loader, it has no control over the modules imported indirectly (transitively). This is not an issue for modules that are side effect free, but it is a hazard for stateful/singleton modules.
As an example, bug 1837185 was caused by an unexpectedly uninitialized AddonManager instance (a new instance rather than the existing instance). The immediate issue was resolved by using loadInDevToolsLoader: false on imports in devtools/, but that wouldn't work if these modules were imported indirectly from another module. I believe that architectural changes to the module loading logic are necessary to eradicate this class of issues. For example, by maintaining an allowlist of paths that may be loaded in the distinct devtools loader.
|
||
Comment 1•1 year ago
|
||
Sorry about the confusion around this. I did not forecast people outside of devtools would have to fix things around the browser toolbox...
I agree with you, a list of modules used in the C++ Module Loader could probably simplify the story around modules which shouldn't be duplicated in the Browser Toolbox Module Loader.
We could probably pass the loaded module URI to GetContextualESLoader, which would compute the right module loader rather based on module URI rather than this loadInDevToolsLoader flag.
https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3domL21GetContextualESLoaderERKNS0_8OptionalIbEEP8JSObject&redirect=false
Unfortunately, I'm not fluent enough with C++ to know how to implement an efficient and maintainable list of modules URIs?
It would be neat to avoid having to re-build C++ to maintain this list.
|
Reporter | |
Comment 2•1 year ago
|
||
Do you anticipate any usage of this outside of devtools?
If not, then it might be more effective to:
- if loadInDevtoolsLoader is passed, use that value. This allows the entry point to enter the devtools loader, or a specific devtools module already loaded through the devtools loader to opt out of it.
- if not, check if the global is the devtools global, check whether the module URL starts with "resource://devtools/"
- otherwise use the main module loader.
|
||
Comment 3•1 year ago
|
||
For the record I think we should move webext to use something else than --start-debugger-server + RDP requests to install addons. This codepath is mostly intended for the Browser Toolbox.
|
|
||
Comment 4•1 year ago
|
||
(In reply to Rob Wu [:robwu] from comment #2)
Do you anticipate any usage of this outside of devtools?
If not, then it might be more effective to:
- if loadInDevtoolsLoader is passed, use that value. This allows the entry point to enter the devtools loader, or a specific devtools module already loaded through the devtools loader to opt out of it.
Yes the boolean would be important for the first entry point where we pass loadInDevtoolsLoader: true. This is the falsy value which would become useless if we introduce a list of module URIs.
- if not, check if the global is the devtools global, check whether the module URL starts with "resource://devtools/"
I would have done that right away if that was this simple ;)
Unfortunately doing this prevents debugging ESM correctly.
The typical example is about putting a breakpoint in XPCOMUtils. The breakpoint will be trigerred when devtools calls this shared XPCOMUtils module.
With this issue in mind, I believed it was better to special case the singleton modules and allow the best debugging experience for all the other modules by default.
But may be this should be the other way around given that the concept of ESM being singleton is strong. May be DevTools should explicitly mention the ESM that should be forked into the devtools loader??
In any case, using a generic pattern like URI doesn't work. We have to be explicit one way of another.
(In reply to Julian Descottes [:jdescottes] from comment #3)
For the record I think we should move webext to use something else than --start-debugger-server + RDP requests to install addons. This codepath is mostly intended for the Browser Toolbox.
May be it is worth opening a dedicated bug for this?
I think the current discussion about loadInDevtoolsLoader is relevant regardless of webext usage of devtools.
(and the other way around having webext use more reliable way to install addons is relevant regardless of the module loader)
|
|
||
Comment 5•1 year ago
|
||
Given that this only impacts remote debugging or Browser Toolbox, this should usually impact only DevTools peers, but we agree we should improve this. Adding a list of modules which should NOT be loaded in the Distinct loader is probably the way to go. We could also use this information to surface in the Debugger that those modules can not be debugged from the Browser Toolbox.
Description
•