Allow loading ESM in a distinct system compartment
Categories
(DevTools :: General, enhancement)
Tracking
(firefox107 fixed)
| Tracking | Status | |
|---|---|---|
| firefox107 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
In order to convert backend modules of DevTools to ESM we would need a way to load these modules in a distinct system compartment.
For now we are using either freshCompartment: true or invisibleToDebugger: true arguments of Sandbox in order to load all CommonJS modules in a distinct system compartment.
This is only important in the case of the Browser Toolbox, i.e. when we debug the browser.
When we debug the browser itself, we have to:
- load DevTools modules independantly of the ones loaded for regular DevTools. We don't want to share any possible state.
- have a way to distinguish the two set of modules and ignore the one related to the brower toolbox. This is especially important for the debugger in order to avoid pausing in code related to the browser toolbox.
For now, the CommonJS modules are loaded via loadSubScript in hand-crafted JS Sandbox().
We could possibly use a regumar import method from a hand-crafted document, using the right principal. Similaryl to attachment 9041865 [details].
But I suspect that any inner usage of ChromeUtils.defineESModuleGetters is going to fallback to load the ESM in the shared system global instead of the one from the caller...
(There is also possible usage of ChromeUtils.import, but we could say we have to convert them to regular import x from;)
I see two possible call for actions here:
- make
ChromeUtils.defineESModuleGetterstry to use the callsite's module loader, if called from a ESM. Does that sound feasible/reasonable? - may be also expose an API to easily load a ESM in this "distinct system compartment". So that it is simplier to load the very first devtools backend JSM.
But I'm open to suggestions and alternative plan here.
I think that WebExtension might need something alongside these lines. Any bug already opened?
|
Assignee | |
Updated•3 years ago
|
|
||
Comment 1•3 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #0)
But I suspect that any inner usage of
ChromeUtils.defineESModuleGettersis going to fallback to load the ESM in the shared system global instead of the one from the caller...
Yes, currently there's only one shared global and module loader, and ChromeUtils.importESModule and ChromeUtils.defineESModuleGetters use them, regardless of the caller.
void mozJSModuleLoader::CreateLoaderGlobal(JSContext* aCx,
const nsACString& aLocation,
MutableHandleObject aGlobal) {
...
nsresult rv = xpc::InitClassesWithNewWrappedGlobal(
aCx, static_cast<nsIGlobalObject*>(backstagePass),
nsContentUtils::GetSystemPrincipal(), xpc::DONT_FIRE_ONNEWGLOBALHOOK,
options, &global);
...
MOZ_ASSERT(!mModuleLoader);
RefPtr<ComponentScriptLoader> scriptLoader = new ComponentScriptLoader;
mModuleLoader = new ComponentModuleLoader(scriptLoader, backstagePass);
backstagePass->InitModuleLoader(mModuleLoader);
- make
ChromeUtils.defineESModuleGetterstry to use the callsite's module loader, if called from a ESM. Does that sound feasible/reasonable?
We could add support for multiple shared globals and module loaders,
and a map from global to loader, and use corresponding loader for each
ChromeUtils.importESModule and ChromeUtils.defineESModuleGetters call.
class mozJSModuleLoader final : public nsIMemoryReporter {
...
JS::PersistentRooted<JSObject*> mLoaderGlobal;
...
RefPtr<mozilla::loader::ComponentModuleLoader> mModuleLoader;
I'll look into the design.
- may be also expose an API to easily load a ESM in this "distinct system compartment". So that it is simplier to load the very first devtools backend JSM.
Adding extra options parameter would work.
e.g.
const { XPCOMUtils } = ChromeUtils.importESModule(
"resource://gre/modules/XPCOMUtils.sys.mjs", { newGlobal: true }
);
ChromeUtils.defineESModuleGetters(lazy, {
UrlbarPrefs: "resource:///modules/UrlbarPrefs.sys.mjs",
}, { newGlobal: true });
If there'll be only one distinct global other than the normal one, the option can be one of:
- import into the current shared global (default)
- import into the normal shared global (maybe unnecessary)
- import into the distinct shared global
If there can be multiple distinct globals other than the normal one, the option would need to be ID or something.
I think that WebExtension might need something alongside these lines. Any bug already opened?
So far no bugs are opened, and iiuc migrating the subscript was not planned when we were discussing the APIs.
|
|
||
Comment 2•3 years ago
|
||
perhaps adding yet another mozJSModuleLoader instance might be simpler
static mozilla::StaticRefPtr<mozJSModuleLoader> sSelf;
|
|
||
Comment 3•3 years ago
|
||
a problem I can think of is, the edge case around how to associate the global/loader.
inside the system module, the current global tells which loader it corresponds to, given there's only one shared global per each loader (or loader's context or whatever).
So, once we add entry point to use different shared global, any module-import inside system module loads the module into the current module loader.
however, once the execution goes out of the system module's global, for example, by running sandboxed code, the current global doesn't tell the correct loader, or even, the "correct loader" needs definition.
for example, if the following code is executed inside a module loaded into the different shared global, where should the A.sys.mjs be loaded?
var sandbox = Cu.Sandbox(...);
Cu.evalInSandbox(`
ChromeUtils.importESModule("A.sys.mjs");
`, sandbox);
possible options are:
- (a) load into the regular shared global
- (b) look into the call stack or something, and load into the innermost shared global if found
(a) is the default behavior when we don't add special case, except for using current global's loader.
(b) would work as expected in some case, but it may need yet another way to leave the different shared global.
|
|
||
Comment 4•3 years ago
|
||
in the browser toolbox's case, what's the interaction between the code inside the distinct system compartment and others?
can there be a case like the above example?
|
|
Assignee | |
Comment 5•3 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #3)
a problem I can think of is, the edge case around how to associate the global/loader.
inside the system module, the current global tells which loader it corresponds to, given there's only one shared global per each loader (or loader's context or whatever).
So, once we add entry point to use different shared global, any module-import inside system module loads the module into the current module loader.
This sounds exactly like what we would need!
Any import foo from "foo.js"; would be loaded in the same loader/global/principal.
In theory, we wouldn't use ChromeUtils.importESModule. Consider that we will replace all of them with regular ES import.
But there is lazy imports, which have to be going through ChromeUtils.defineESModuleGetters.
We should make it so that this magic method reuse the contextual loader. The loader of the module where we called the function.
however, once the execution goes out of the system module's global, for example, by running sandboxed code, the current global doesn't tell the correct loader, or even, the "correct loader" needs definition.
for example, if the following code is executed inside a module loaded into the different shared global, where should the
A.sys.mjsbe loaded?var sandbox = Cu.Sandbox(...); Cu.evalInSandbox(` ChromeUtils.importESModule("A.sys.mjs"); `, sandbox);possible options are:
- (a) load into the regular shared global
- (b) look into the call stack or something, and load into the innermost shared global if found
(a) is the default behavior when we don't add special case, except for using current global's loader.
(b) would work as expected in some case, but it may need yet another way to leave the different shared global.
We do not need anything special here. importESModule should keep its current behavior, so (a).
May be we should be conservative here and only consider the very specific special of DevTools:
ChromeUtils.importESModuleisn't contextual, but accepts a parameter to be able to load the very first ESM in the devtool's specific loader.
Any other usage of this method, without the new param, will load in the regular shared global.- Only tweak the behavior of
ChromeUtils.defineESModuleGettersto be contextual, and only for the precise usecase of devtools:- only if the callsite is from a module using the devtools specific loader,
- then load all lazy symbol in the devtools specific loader
WDYT?
|
|
Assignee | |
Comment 6•3 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #4)
in the browser toolbox's case, what's the interaction between the code inside the distinct system compartment and others?
can there be a case like the above example?
Overall, the typical scenarios is that ESM/JSM of the regular shared global will use ChromeUtils.importESModule with a new flag to load some modules in the special devtool global. They will set the flag explicitely. Typically when we start the DevToolsServer for the Browser Toolbox.
For example:
https://searchfox.org/mozilla-central/rev/9769b513e38ee4f5df9d5d1eff55ff7cdc8cbf81/devtools/client/framework/browser-toolbox/Launcher.sys.mjs#132-140
could become:
const { DevToolsServer } = ChromeUtils.importESModule(
"resource://devtools/server/devtools-server.sys.mjs", { devtoolsGlobal: true } // Just a proposal, there might be better API/name than "devtoolsGlobal".
);
Then, any ESM loaded in this special devtools global should load its dependencies in the same devtools global (via import foo from "foo.js", or ChromeUtils.importESModule).
Leftovers of ChromeUtils.import and ChromeUtils.importESModule will load from the regular shared global.
That's fine. And it might actually be useful in order to escape the devtools global.
We might have cases where we would like to use a module from the shared global, then, we would use ChromeUtils.importESModule.
I can't think about any scenario where we are using another Sandbox.
But if we do have one, and we would like to load a ESM in the devtools global, we would use the new param of ChromeUtils.importESModule.
|
|
||
Comment 7•3 years ago
|
||
sorry, my example about the sandbox was misleading.
- it's also about
ChromeUtils.defineESModuleGettersinside the sandbox case, or any other case where the execution goes out of the devtools shared global - the target code can be non-module script, and in that case static
importis not available
So, the question is that, is there any case that following happens?
- a code "A" inside "devtools shared global" runs another code "B" in another global (for example, calls function, performs eval, loads subscript, etc)
- the code "B" (or any other code called/executed from there) calls
ChromeUtils.importESModuleorChromeUtils.defineESModuleGetters - the code "A" expects the
ChromeUtils.importESModuleorChromeUtils.defineESModuleGettersinside code "B" to use the same shared global as "A"
In the current code, I guess the almost-equivalent question would be, is there any case that the this.#loader in the above link is implicitly used by a code outside of the special global?
if this never happens, then we don't have to worry about module loading outside of system global.
(In reply to Alexandre Poirot [:ochameau] from comment #5)
(In reply to Tooru Fujisawa [:arai] from comment #3)
In theory, we wouldn't useChromeUtils.importESModule. Consider that we will replace all of them with regular ES import.
static import is available only at the top-level of the module script.
So if there's ChromeUtils.import inside a block (conditionally imported) or inside a function (import only when called), that should be rewritten with ChromeUtils.importESModule, not the static import (or dynamic import() which is not supported yet).
Thus, I think we should also make ChromeUtils.importESModule contextual, to avoid confusion.
(In reply to Alexandre Poirot [:ochameau] from comment #6)
Leftovers of
ChromeUtils.importandChromeUtils.importESModulewill load from the regular shared global.
That's fine. And it might actually be useful in order to escape the devtools global.
We might have cases where we would like to use a module from the shared global, then, we would use ChromeUtils.importESModule.
Given above, if we need a way to escape the devtools global, that will also need special flag, maybe { devtoolsGlobal: false }.
IMO, it's better making the boundary explicit.
I can't think about any scenario where we are using another Sandbox.
But if we do have one, and we would like to load a ESM in the devtools global, we would use the new param of ChromeUtils.importESModule.
okay, if all cases can be explicit about "non-devtools-global to devtools-global" import, it sounds good :)
|
|
Assignee | |
Comment 8•3 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #7)
So, the question is that, is there any case that following happens?
- a code "A" inside "devtools shared global" runs another code "B" in another global (for example, calls function, performs eval, loads subscript, etc)
We do use Sandbox:
https://searchfox.org/mozilla-central/search?q=Sandbox%28&path=devtools%2Fserver%2F&case=false®exp=false
But none of them are meant to load ESM. The main usage of Sandbox is to evaluate code provided by the user in a safe sandbox.
(ignore the first usage, this code should be removed)
We use loadSubScript only in the loader (which will go away once we move to ESM):
https://searchfox.org/mozilla-central/search?q=loadSubScript&path=devtools%2Fserver%2F&case=false®exp=false
We don't do any straight "eval()". We do eval in the web page, typically, for the web console. But again, this won't imply loading any devtools ESM.
We might use import, but this is meant to use the web page's loader.
- the code "B" (or any other code called/executed from there) calls
ChromeUtils.importESModuleorChromeUtils.defineESModuleGetters
I think we stop here. We will only use these two methods from ESM/JSM. The ESM callsites will be either loaded via ChromeUtils methods -or- via regular import. It can be ESM loaded from the regular shared global -or- the special devtools global.
(In reply to Alexandre Poirot [:ochameau] from comment #5)
(In reply to Tooru Fujisawa [:arai] from comment #3)
In theory, we wouldn't useChromeUtils.importESModule. Consider that we will replace all of them with regular ES import.static
importis available only at the top-level of the module script.
So if there'sChromeUtils.importinside a block (conditionally imported) or inside a function (import only when called), that should be rewritten withChromeUtils.importESModule, not the staticimport(or dynamicimport()which is not supported yet).Thus, I think we should also make
ChromeUtils.importESModulecontextual, to avoid confusion.
Ah yes, I forgot about imports done from a block/function.
I considered that imports are either done from the top level, or using the lazy API.
But yes, that sounds fine making importESModule contextual.
(In reply to Alexandre Poirot [:ochameau] from comment #6)
Leftovers of
ChromeUtils.importandChromeUtils.importESModulewill load from the regular shared global.
That's fine. And it might actually be useful in order to escape the devtools global.
We might have cases where we would like to use a module from the shared global, then, we would use ChromeUtils.importESModule.Given above, if we need a way to escape the devtools global, that will also need special flag, maybe
{ devtoolsGlobal: false }.IMO, it's better making the boundary explicit.
Yes, that's a good idea.
I can't think about any scenario where we are using another Sandbox.
But if we do have one, and we would like to load a ESM in the devtools global, we would use the new param of ChromeUtils.importESModule.okay, if all cases can be explicit about "non-devtools-global to devtools-global" import, it sounds good :)
Yes, I think both "non-devtools-global to devtools-global" and "devtools-global to non-devtools-global" will/could be explicit.
Note that I'm not really a C++ developer, but I'd be happy to be mentored if you are busy with other things.
I imagine the starting point would be conditionaly use a distinct loader over there:
https://searchfox.org/mozilla-central/rev/9769b513e38ee4f5df9d5d1eff55ff7cdc8cbf81/dom/base/ChromeUtils.cpp#580
- RefPtr moduleloader = mozJSModuleLoader::Get();
+ RefPtr moduleloader = devtoolsGlobal ? mozJSModuleLoader::GetDevToolsLoader() : mozJSModuleLoader::Get();
Then, this second mozJSModuleLoader would actualy spawn another distinct shared global:
https://searchfox.org/mozilla-central/source/js/xpconnect/loader/mozJSModuleLoader.cpp#622-639
We may only tweak this function to create a global with another name over there:
https://searchfox.org/mozilla-central/rev/9769b513e38ee4f5df9d5d1eff55ff7cdc8cbf81/js/xpconnect/loader/mozJSModuleLoader.cpp#625
- CreateLoaderGlobal(aCx, "shared JSM global"_ns, &globalObj);
+ CreateLoaderGlobal(aCx, "DevTools server global"_ns, &globalObj);
And if necessary tweak realms settings:
https://searchfox.org/mozilla-central/rev/9769b513e38ee4f5df9d5d1eff55ff7cdc8cbf81/js/xpconnect/loader/mozJSModuleLoader.cpp#570-573
Then, making the API contextual is still unclear to me.
https://searchfox.org/mozilla-central/rev/9769b513e38ee4f5df9d5d1eff55ff7cdc8cbf81/dom/base/ChromeUtils.cpp#576
void ChromeUtils::ImportESModule(const GlobalObject& aGlobal,
const nsAString& aResourceURI,
JS::MutableHandle<JSObject*> aRetval,
ErrorResult& aRv) {
RefPtr moduleloader = mozJSModuleLoader::Get();
I imagine we should query aGlobal?
void ChromeUtils::ImportESModule(const GlobalObject& aGlobal,
const nsAString& aResourceURI,
bool devtoolsGlobal, // We probably benefit from having a better API than just a boolean, but let's say we pass a boolean
JS::MutableHandle<JSObject*> aRetval,
ErrorResult& aRv) {
// Either we explicitely requested to load from devtools gloal, or we are called from a devtools global.
// We would also have to handle the case where we pass an explicit devtoolsGlobal = false and should load from the shared global.
RefPtr moduleloader = devtoolsGlobal || mozJSModuleLoader::GetDevToolsLoader()->GetSharedGlobal(aGlobal.Context()) == aGlobal ? mozJSModuleLoader::GetDevToolsLoader() : mozJSModuleLoader::Get();
And then apply something similar to defineESModuleGetters.
|
|
||
Comment 9•3 years ago
|
||
Okay, sounds good :)
I'm happy to mentor.
(In reply to Alexandre Poirot [:ochameau] from comment #8)
I imagine the starting point would be conditionaly use a distinct loader over there:
https://searchfox.org/mozilla-central/rev/9769b513e38ee4f5df9d5d1eff55ff7cdc8cbf81/dom/base/ChromeUtils.cpp#580- RefPtr moduleloader = mozJSModuleLoader::Get(); + RefPtr moduleloader = devtoolsGlobal ? mozJSModuleLoader::GetDevToolsLoader() : mozJSModuleLoader::Get();
Yes, that's for ChromeUtils.importESModule, and the following for ChromeUtils.defineESModuleGetters.
static bool ModuleGetterImpl(JSContext* aCx, unsigned aArgc, JS::Value* aVp,
ModuleType aType) {
...
RefPtr moduleloader = mozJSModuleLoader::Get();
This function is shared between ChromeUtils.defineModuleGetter (aType == ModuleType::JSM) and ChromeUtils.defineESModuleGetters (aType == ModuleType::ESM).
Here's the full list of the places the loader is used, and corresponding API
| Callsite | API | Note |
|---|---|---|
| ChromeUtils::Import | ChromeUtils.import |
|
| ChromeUtils::ImportESModule | ChromeUtils.importESModule |
|
| module_getter::ModuleGetterImpl | ChromeUtils.defineModuleGetter and ChromeUtils.defineESModuleGetters |
|
| JSActorManager::GetActor | ChromeUtils.registerWindowActor and ChromeUtils.registerProcessActor |
unrelated if window/process actor is not used in devtools global |
| OSFileConstantsService::Init | OS.File |
unnecessary as long as OS.File is not used in devtools global |
| EvalStencil and mozJSSubScriptLoader::DoLoadSubScriptWithOptions | Services.scriptloader.loadSubScript |
unnecessary if subscript loader is unused |
| loader::ImportModule | - | unrelated |
| nsXPCComponents_Utils::IsModuleLoaded | Cu.isModuleLoaded |
unnecessary is there's no consumer in devtools global |
| nsXPCComponents_Utils::IsJSModuleLoaded | Cu.isJSModuleLoaded |
(same) |
| nsXPCComponents_Utils::IsESModuleLoaded | Cu.isESModuleLoaded |
(same) |
| nsXPCComponents_Utils::GetLoadedModules | Cu.loadedModules |
(same) |
| nsXPCComponents_Utils::GetLoadedJSModules | Cu.loadedJSModules |
(same) |
| nsXPCComponents_Utils::GetLoadedESModules | Cu.loadedESModules |
(same) |
| nsXPCComponents_Utils::GetModuleImportStack | Cu.getModuleImportStack |
might be nice to support, for debugging purpose |
| nsXPCComponents_Utils::Unload | Cu.unload |
unnecessary as long as JSM is not used |
| xpc::JSReporter::CollectReports | Memory reporter | This needs to report both regular one and devtools one |
| XPCJSRuntime::LoaderGlobal | ? | this is used by many places and needs special attention |
| XPCWrappedNativeScope::AttachJSServices | Cu.Sandbox and other sandbox APIs |
unnecessary if all sandboxes are going away. otherwise Services in the sandbox is mixed up |
| xpc::InitGlobalObject | creating global or document | unnecessary as long as either: (a) devtools global does not create any global, or (b) devtools global creates global but does not expect it to use its Services |
| ctypes::Module::Call | ctypes.jsm |
unnecessary as long as ctypes is not used |
| ConstructJSMOrESMComponent | static component loader | unrelated |
About XPCJSRuntime::LoaderGlobal, we might have to check all the following consumers to see if any of them are explicitly/implicitly used from devtools global:
- https://searchfox.org/mozilla-central/search?q=symbol:_ZN12XPCJSRuntime12LoaderGlobalEv&redirect=false
- https://searchfox.org/mozilla-central/search?q=symbol:_ZN3xpc19PrivilegedJunkScopeEv&redirect=false
- https://searchfox.org/mozilla-central/search?q=symbol:_ZN3xpc16CompilationScopeEv&redirect=false
Then, this second mozJSModuleLoader would actualy spawn another distinct shared global:
https://searchfox.org/mozilla-central/source/js/xpconnect/loader/mozJSModuleLoader.cpp#622-639
We may only tweak this function to create a global with another name over there:
https://searchfox.org/mozilla-central/rev/9769b513e38ee4f5df9d5d1eff55ff7cdc8cbf81/js/xpconnect/loader/mozJSModuleLoader.cpp#625- CreateLoaderGlobal(aCx, "shared JSM global"_ns, &globalObj); + CreateLoaderGlobal(aCx, "DevTools server global"_ns, &globalObj);
Yes, exactly.
Then, making the API contextual is still unclear to me.
https://searchfox.org/mozilla-central/rev/9769b513e38ee4f5df9d5d1eff55ff7cdc8cbf81/dom/base/ChromeUtils.cpp#576void ChromeUtils::ImportESModule(const GlobalObject& aGlobal, const nsAString& aResourceURI, JS::MutableHandle<JSObject*> aRetval, ErrorResult& aRv) { RefPtr moduleloader = mozJSModuleLoader::Get();I imagine we should query
aGlobal?void ChromeUtils::ImportESModule(const GlobalObject& aGlobal, const nsAString& aResourceURI, bool devtoolsGlobal, // We probably benefit from having a better API than just a boolean, but let's say we pass a boolean JS::MutableHandle<JSObject*> aRetval, ErrorResult& aRv) { // Either we explicitely requested to load from devtools gloal, or we are called from a devtools global. // We would also have to handle the case where we pass an explicit devtoolsGlobal = false and should load from the shared global. RefPtr moduleloader = devtoolsGlobal || mozJSModuleLoader::GetDevToolsLoader()->GetSharedGlobal(aGlobal.Context()) == aGlobal ? mozJSModuleLoader::GetDevToolsLoader() : mozJSModuleLoader::Get();And then apply something similar to
defineESModuleGetters.
Yes, using the GlobalObject or underlying JSObject* as a key would work.
But I think it's better not to call GetSharedGlobal there, given it will create the global even if the global is not used in the process.
So, just comparing against the possibly-null member would be nice.
JS::PersistentRooted<JSObject*> mLoaderGlobal;
+JS::PersistentRooted<JSObject*> mDevToolsLoaderGlobal;
and
> RefPtr moduleloader = devtoolsGlobal || aGlobal.Get() == mDevToolsLoaderGlobal ? mozJSModuleLoader::GetDevToolsLoader() : mozJSModuleLoader::Get();
|
|
Assignee | |
Comment 10•3 years ago
|
||
|
|
Assignee | |
Comment 11•3 years ago
|
||
I still need to go through the table of comment 9, but I'd appreciate feedback on this first patch.
Consider that I'm not a C++ developer and feel free to suggest any major modification!
Typically, I'm not sure the following practice is acceptable?
if (this == sSelf) {
sSelf = nullptr;
} else if (this == sDevToolsLoader) {
sDevToolsLoader = nullptr;
}
|
|
Assignee | |
Comment 12•3 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #9
Here's the full list of the places the loader is used, and corresponding API
Out of curiosity, this table is super handy... are you using a particular tool to built such table??
Callsite API Note ChromeUtils::Import ChromeUtils.importDon't want to impact JSM ChromeUtils::ImportESModule ChromeUtils.importESModuleMigrated module_getter::ModuleGetterImpl ChromeUtils.defineModuleGetterandChromeUtils.defineESModuleGettersMigrated JSActorManager::GetActor ChromeUtils.registerWindowActorandChromeUtils.registerProcessActorWould be a nice to have. Will open a followup. OSFileConstantsService::Init OS.FileThe is one usage left, I'll try to get rid of it! EvalStencil and mozJSSubScriptLoader::DoLoadSubScriptWithOptions Services.scriptloader.loadSubScriptsubscript loader is only used to implement the common js module loader. Or this is used in the frontend where we want to use the regular shared global. loader::ImportModule - unrelated nsXPCComponents_Utils::IsModuleLoaded Cu.isModuleLoadedOnly used to check is a JSM is loaded to avoid loading it. This JSM is one from WebExtension, so we only care about the shared loader. nsXPCComponents_Utils::IsJSModuleLoaded Cu.isJSModuleLoaded(same) nsXPCComponents_Utils::IsESModuleLoaded Cu.isESModuleLoaded(same) nsXPCComponents_Utils::GetLoadedModules Cu.loadedModulesNot used nsXPCComponents_Utils::GetLoadedJSModules Cu.loadedJSModules(same) nsXPCComponents_Utils::GetLoadedESModules Cu.loadedESModules(same) nsXPCComponents_Utils::GetModuleImportStack Cu.getModuleImportStackmight be nice to support, for debugging purpose nsXPCComponents_Utils::Unload Cu.unloadWe won't be using JSM. The goal is to migrate JSM and CommonJS to ESM. xpc::JSReporter::CollectReports Memory reporter This needs to report both regular one and devtools one XPCJSRuntime::LoaderGlobal ? Investigated below XPCWrappedNativeScope::AttachJSServices Cu.Sandboxand other sandbox APIsSandboxes are mostly used to implement the CommonJS loader. But there is three usage of Sandbox in the server. None of them use wantComponents. So we are all good? xpc::InitGlobalObject creating global or document This seems to be used only for backstagepass or DOM Windows. We shouldn't be instantiating DOM documents related to the special devtools global. ctypes::Module::Call ctypes.jsmWe aren't using ctypes directly. May be via external deps. We do use SubProcess, but this isn't loaded via the DevTools loader. We load it via JSM and it would be fine loading via the shared JSM global as ESM. ConstructJSMOrESMComponent static component loader unrelated
Note that we don't need to load all DevTools modules in the special compartment flagged with invisibleToDebugger.
We only need to load server modules which are loaded when a Browser Toolbox debug firefox itself and we don't want to see or interupt these server modules while debugging.
This is why this feature and patch applies only to a subset of the devtools/ folder. Mostly the devtools/server/ folder and some stuff from devtools/shared/.
About
XPCJSRuntime::LoaderGlobal, we might have to check all the following consumers to see if any of them are explicitly/implicitly used from devtools global:
If we do care to spawn Sandbox in the same compartment as DevTools, it might be interesting looking into
https://searchfox.org/mozilla-central/rev/6f9fdc5c3b164a46004c98a5baaf55b05e2ad329/js/xpconnect/src/Sandbox.cpp#1328
But I'm not sure it is really useful.
TBH, I don't know what to think about this one AutoJSAPI::ReportException()
Same about JSActor. I would like to followup to allow loading JSWindowActor in the devtools global. But it sounds like the aGlobal will be passed.
Looking at test coverage it is very rare when we fallback to the priviledged scope. Might be something to verify while following up on JSActors?
Otherwise, the vast majority of the usages of PrivilegedJunk is to create JS Objects.
It sounds fine creating the objects in the JSM global. Typically all the following pattern xpc::NativeGlobal(xpc::PrivilegedJunkScope()); looks fine.
I don't know what to think about the ScriptPreloader.
I imagine it is fine not having any preload for modules loaded by the browser toolbox.
But could that be an issue if we expect the scripts to be preloaded in the right devtools loader scope?
Otherwise CompilationScope is used only for mesage manager, xul and subscript loader. All this sounds fine being specific to the shared JSM loader.
|
|
Assignee | |
Updated•3 years ago
|
|
|
||
Comment 13•3 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #12)
(In reply to Tooru Fujisawa [:arai] from comment #9
Here's the full list of the places the loader is used, and corresponding API
Out of curiosity, this table is super handy... are you using a particular tool to built such table??
Mostly manually, with searching each consumer in searchfox.
If we do care to spawn Sandbox in the same compartment as DevTools, it might be interesting looking into
https://searchfox.org/mozilla-central/rev/6f9fdc5c3b164a46004c98a5baaf55b05e2ad329/js/xpconnect/src/Sandbox.cpp#1328
But I'm not sure it is really useful.
If we're not going to spawn sandboxes from devtools global, it would be nice to add assertion to related code that checks the current global is not devtools global.
If we're going to spawn sandboxes from devtools global but it's supposed to be using shared global, then the assertion isn't necessary.
TBH, I don't know what to think about this one AutoJSAPI::ReportException()
This doesn't seem to require consistency between shared global vs devtools global, but just need to some random global to enter, and it should be fine to keep using shared global unconditionally.
Same about JSActor. I would like to followup to allow loading JSWindowActor in the devtools global. But it sounds like the aGlobal will be passed.
Looking at test coverage it is very rare when we fallback to the priviledged scope. Might be something to verify while following up on JSActors?
If this path is not used right now in devtools global, I think it's fine to address this in a followup bug.
(optional) it might be nice to add assertion here as well to check if this is really not used by devtools global.
Otherwise, the vast majority of the usages of PrivilegedJunk is to create JS Objects.
It sounds fine creating the objects in the JSM global. Typically all the following patternxpc::NativeGlobal(xpc::PrivilegedJunkScope());looks fine.
Okay, sounds good :)
I don't know what to think about the ScriptPreloader.
I imagine it is fine not having any preload for modules loaded by the browser toolbox.
But could that be an issue if we expect the scripts to be preloaded in the right devtools loader scope?
The scope is used mostly for getting realm options, for decoding cache, and as long as those options are same between shared global and devtools global, which is true in the current patch, it doesn't matter which one to use.
Also, this part doesn't actually allocate objects in the give scope/global, it's not necessary to handle devtools global here.
Also, we're going to remove JSContext-dependency from cache and decoding API in long term.
Otherwise CompilationScope is used only for mesage manager, xul and subscript loader. All this sounds fine being specific to the shared JSM loader.
Thank you for investigating!
|
|
Assignee | |
Comment 14•3 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #13)
If we're not going to spawn sandboxes from devtools global, it would be nice to add assertion to related code that checks the current global is not devtools global.
If we're going to spawn sandboxes from devtools global but it's supposed to be using shared global, then the assertion isn't necessary.
We do spawn 3 sandboxes.
A first one in the content process targets actor. We expect it to relate to the content process privileged scope, so the shared global in each content process.
Then the two others are only used to fetch symbols out of the Sandbox object (indexedDB, and list of native global names).
Theoritically, we probably expect these symbols to be related to the debugee, which can be a content document, or the privileged scope (=shared jsm global). But at the end, I don't think binding them to one global or the other will have much impact.
So, we do spawn sandboxes. And at least one, the one in content-process.js is expected to be using the shared global for its imports.
TBH, I don't know what to think about this one AutoJSAPI::ReportException()
This doesn't seem to require consistency between shared global vs devtools global, but just need to some random global to enter, and it should be fine to keep using shared global unconditionally.
I'm relieved as I didn't know how to handle this one!
Same about JSActor. I would like to followup to allow loading JSWindowActor in the devtools global. But it sounds like the aGlobal will be passed.
Looking at test coverage it is very rare when we fallback to the priviledged scope. Might be something to verify while following up on JSActors?If this path is not used right now in devtools global, I think it's fine to address this in a followup bug.
(optional) it might be nice to add assertion here as well to check if this is really not used by devtools global.
I opened bug 1792683 to followup on this.
Do you mean to add an assertion against aGlobal?
Something like this in JSActor's contructor:
if (aGlobal) {
mozJSModuleLoader* loader = mozJSModuleLoader::Get();
MOZ_ASSERT(loader->IsLoaderGlobal(aGlobal));
}
Or something asserting the contextual global of the callsite?
|
|
||
Comment 15•3 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #14)
I opened bug 1792683 to followup on this.
Do you mean to add an assertion againstaGlobal?
Yes, but checks if the global is not devtools loader's one if it's already instantiated.
Also, JSActor class doesn't necessarily know the current global.
So I think it should be placed at the consumer of JSActor, maybe around the following:
already_AddRefed<JSActor> JSActorManager::GetActor(JSContext* aCx,
const nsACString& aName,
ErrorResult& aRv) {
Something like:
MOZ_ASSERT(mozJSModuleLoader::IsNotDevToolsGlobal(JS::CurrentGlobalOrNull(aCx)));
With:
static bool IsNotDevToolsGlobal(JSObject* aGlobal) {
if (!sDevToolsLoader) {
return true;
}
return aGlobal != sDevToolsLoader->mLoaderGlobal;
}
|
|
Assignee | |
Comment 16•3 years ago
•
|
||
(In reply to Tooru Fujisawa [:arai] from comment #15)
(In reply to Alexandre Poirot [:ochameau] from comment #14)
I opened bug 1792683 to followup on this.
Do you mean to add an assertion againstaGlobal?Yes, but checks if the global is not devtools loader's one if it's already instantiated.
Also,
JSActorclass doesn't necessarily know the current global.
So I think it should be placed at the consumer ofJSActor, maybe around the following:
Ahh ok. But then, we can't add such assertion.
DevTools do use getActor extensively, and will do so from the DevTools ESMs.
For now JSWindow Actors are buggy in the browser toolbox and setting breakpoint in them can easily break/freeze the browser toolbox.
That's because they aren't running in the CommonJS DevTools loader and so aren't running in the invisibleToDebugger global.
Bug 1792683 will actually finally be able to fix that by loading the ESM in the DevTools loader.
In bug 1792683, I was considering introducing a new flag in ChromeUtils.registerWindowActor / WindowActorOptions in order to force loading a particular JSWindowActor into the DevTools loader.
But with your suggested assertion... that makes me wonder if we can actually make that contextual based on getActor() callsite.
That would be even easier!
So. No need of an assertion in Sandbox, nor here in getActor. In bug 1792683, I'll make either make it contextual, or allow to load in a precise module loader based on WindowActorOptions. In any case getActor will be called from the two loaders.
Does that mean I am go to go with this patch?
|
|
Assignee | |
Comment 17•3 years ago
|
||
But I will at least wait for bug 1792404 to land and rebase to support the devtools loader in import stack.
|
|
||
Comment 18•3 years ago
|
||
Okay, then I think the current patch is ready, once it's rebased onto bug 1792404.
|
||
Updated•3 years ago
|
|
|
||
Updated•3 years ago
|
|
|
||
Updated•3 years ago
|
|
||
Comment 19•3 years ago
|
||
|
||
Comment 20•3 years ago
|
||
| bugherder | ||
Description
•