Bugzilla

(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 against `aGlobal`?
> 
> 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:
> 
> https://searchfox.org/mozilla-central/rev/d45dd05bf412e7468b3770a52519e9d546d6325c/dom/ipc/jsactor/JSActorManager.cpp#23-25

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 you suggestion of 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?

(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 against `aGlobal`?
> 
> 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:
> 
> https://searchfox.org/mozilla-central/rev/d45dd05bf412e7468b3770a52519e9d546d6325c/dom/ipc/jsactor/JSActorManager.cpp#23-25

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?