Console is completely broken when debugging xpcshell-tests

VERIFIED FIXED in Firefox 64

Status

defect
P1
major
VERIFIED FIXED
10 months ago
9 months ago

People

(Reporter: standard8, Assigned: ochameau)

Tracking

({regression})

unspecified
Firefox 64
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 wontfix, firefox64 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

When I use the jsdebugger to debug xpcshell-tests, attempting to view the console completely fails to display anything.

This is either via selecting the pane, or by selecting the split console.

There's various errors output:

 0:04.62 INFO "Debugger connected, starting test execution"
 0:04.65 pid:55069 JavaScript warning: resource://devtools/shared/base-loader.js -> resource://devtools/shared/builtin-modules.js, line 194: DebuggeeWouldRun: debuggee `resource://devtools/shared/base-loader.js:427' would run
 0:04.65 pid:55069 JavaScript warning: resource://devtools/shared/base-loader.js -> resource://devtools/shared/builtin-modules.js, line 195: DebuggeeWouldRun: debuggee `resource://devtools/shared/base-loader.js:427' would run
 0:04.67 pid:55069 JavaScript warning: resource://devtools/shared/base-loader.js -> resource://devtools/shared/builtin-modules.js, line 194: DebuggeeWouldRun: debuggee `resource://devtools/shared/base-loader.js:427' would run
 0:04.68 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "DebuggeeWouldRun: debuggee `resource://devtools/shared/base-loader.js:427' would run" {file: "resource://devtools/shared/base-loader.js -> resource://devtools/shared/builtin-modules.js" line: 194}]"
 0:04.68 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "DebuggeeWouldRun: debuggee `resource://devtools/shared/base-loader.js:427' would run" {file: "resource://devtools/shared/base-loader.js -> resource://devtools/shared/builtin-modules.js" line: 195}]"
 0:04.68 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "DebuggeeWouldRun: debuggee `resource://devtools/shared/base-loader.js:427' would run" {file: "resource://devtools/shared/base-loader.js -> resource://devtools/shared/builtin-modules.js" line: 194}]"
 0:04.74 pid:55069 console.error: "Error while calling actor 'accessibility's method 'getWalker'" "window is null; can't access its \\"document\\" property"
 0:04.75 pid:55069 console.error: "isXUL@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/highlighters/utils/markup.js:96:3\\ninitialize@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/accessibility.js:393:53\\ncls@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1243:5\\ngetWalker@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/accessibility.js:1191:21\\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1178:21\\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1716:15\\n_onJSONObjectReady/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:451:9\\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\\nenter@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/utils/event-loop.js:118:5\\n_pushThreadPause@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/thread.js:182:5\\n_pauseAndRespond@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/thread.js:406:7\\nhit@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/breakpoint.js:189:12\\n@/Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/browser/components/urlbar/tests/unit/test_UrlbarController_unit.js:9:1\\nload_file@/Users/mark/dev/gecko/testing/xpcshell/head.js:641:7\\n_load_files@/Users/mark/dev/gecko/testing/xpcshell/head.js:653:3\\n_execute_test@/Users/mark/dev/gecko/testing/xpcshell/head.js:507:3\\n@-e:1:1\\n"
 0:05.08 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "error occurred while processing 'getCachedMessages: TypeError: argument is not a global object
Stack: makeDebuggeeValue@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:439:23
prepareConsoleMessageForRemote/result.arguments<@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:1662:22
prepareConsoleMessageForRemote@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:1661:24
getCachedMessages/<@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:882:29
getCachedMessages@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:874:11
onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1716:15
_onJSONObjectReady/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:451:9
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
enter@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/utils/event-loop.js:118:5
_pushThreadPause@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/thread.js:182:5
_pauseAndRespond@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/thread.js:406:7
hit@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/breakpoint.js:189:12
@/Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/browser/components/urlbar/tests/unit/test_UrlbarController_unit.js:9:1
load_file@/Users/mark/dev/gecko/testing/xpcshell/head.js:641:7
_load_files@/Users/mark/dev/gecko/testing/xpcshell/head.js:653:3
_execute_test@/Users/mark/dev/gecko/testing/xpcshell/head.js:507:3
@-e:1:1
Line: 439, column: 23" {file: "resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js" line: 1571}]
_unknownError@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1571:5
onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1719:29
_onJSONObjectReady/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:451:9
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
enter@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/utils/event-loop.js:118:5
_pushThreadPause@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/thread.js:182:5
_pauseAndRespond@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/thread.js:406:7
hit@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/breakpoint.js:189:12
@/Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/browser/components/urlbar/tests/unit/test_UrlbarController_unit.js:9:1
load_file@/Users/mark/dev/gecko/testing/xpcshell/head.js:641:7
_load_files@/Users/mark/dev/gecko/testing/xpcshell/head.js:653:3
_execute_test@/Users/mark/dev/gecko/testing/xpcshell/head.js:507:3
@-e:1:1
"
 0:11.46 pid:55069 console.error: "Error while calling actor 'inspector's method 'getWalker'" "this.targetActor.window is null; can't access its \\"document\\" property"
 0:11.46 pid:55069 JavaScript error: resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js, line 439: TypeError: argument is not a global object
 0:11.46 pid:55069 console.error: "getWalker@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/inspector.js:110:1\\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1178:21\\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1716:15\\n_onJSONObjectReady/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:451:9\\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\\nenter@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/utils/event-loop.js:118:5\\n_pushThreadPause@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/thread.js:182:5\\n_pauseAndRespond@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/thread.js:406:7\\nhit@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/breakpoint.js:189:12\\n@/Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/browser/components/urlbar/tests/unit/test_UrlbarController_unit.js:9:1\\nload_file@/Users/mark/dev/gecko/testing/xpcshell/head.js:641:7\\n_load_files@/Users/mark/dev/gecko/testing/xpcshell/head.js:653:3\\n_execute_test@/Users/mark/dev/gecko/testing/xpcshell/head.js:507:3\\n@-e:1:1\\n"
 0:11.46 pid:55069 JavaScript error: resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js, line 439: TypeError: argument is not a global object
 0:11.46 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "TypeError: argument is not a global object" {file: "resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js" line: 439}]"
 0:11.46 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "TypeError: argument is not a global object" {file: "resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js" line: 439}]"
Might be a regression from Bug 1414597
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
See Also: → 1414597
(In reply to Julian Descottes [:jdescottes][:julian] from comment #1)
> Might be a regression from Bug 1414597

Scratch that, already broken in DevEdition, getting a regression window.
See Also: 1414597
Regression from Bug 1482454
Blocks: 1482454
Posted patch patchSplinter Review
Alex, do you think we should reinforce our "isTargetSupported" checks? We currently get the inspector and the accessibility panel when debugging xpcshell tests. And the accessibility panel happens to break when the target doesn't have a window.

Maybe we want to make the accessibility panel stable enough to not impact other panels, but I guess we also don't want it to show up on such targets. What would be a good way to check if the actors can have access to a window? 
 - target.tab
 - !target.chrome
 - something else?
Attachment #9010159 - Flags: feedback?(poirot.alex)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #4)
> Created attachment 9010159 [details] [diff] [review]
> patch
> 
> Alex, do you think we should reinforce our "isTargetSupported" checks? We
> currently get the inspector and the accessibility panel when debugging
> xpcshell tests. And the accessibility panel happens to break when the target
> doesn't have a window.
> 
> Maybe we want to make the accessibility panel stable enough to not impact
> other panels, but I guess we also don't want it to show up on such targets.
> What would be a good way to check if the actors can have access to a window? 
>  - target.tab

This would disable for all toolbox, but webpages in firefox ones. i.e. the typical web toolboxes.

>  - !target.chrome

checking for !target.chrome would disable panels in the browser toolbox, and may be also webext one.

>  - something else?

I think you are looking for target.isBrowsingContext:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#307-309
It tells us if the target actor inherits from BrowsingContextActor, (previously named TabActor).
That's the base case that has access to a document, supports attach/detach, supports frame switching, ...
Attachment #9010159 - Flags: feedback?(poirot.alex)
Thanks. I usually discard using this flag because of the warnings in the comment, but I suppose this is the only option here.

  // TODO: This flag is quite confusing, try to find a better way.
  // Bug 1465635 hopes to blow up these classes entirely.
  get isBrowsingContext() {
    return this._isBrowsingContext;
  },
Bug 1465635 is being actively worked on, so I hope this flag is going to be much clearer soon!
I feel that it became clearer with the recent renames/file-reorg from jryans to name all the base "target" and better document which ones where inheriting from BrowsingContext.
I am facing some issues with isBrowsingContext.

`isBrowsingContext` is true when debugging xpcshell tests from the Connect page. This is because we default isBrowsingContext to true when the parameter is not a boolean [1] and the Connect page is passing undefined for the "debug process" link [2].

[1] https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#140
[2] https://searchfox.org/mozilla-central/source/devtools/client/framework/connect/connect.js#133

So I fixed that, by using `openToolbox(aResponse.form, true, "webconsole", false);`

After that the whole toolbox is blank. The connect page is always creating a "tab-target-like" targets [3].

[3] https://searchfox.org/mozilla-central/source/devtools/client/framework/connect/connect.js#233

With isBrowsingContext set to false, we crash with the following stack trace:

request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:525:13
attachThread@resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:435:12
attachThread/<@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/attach-thread.js:106:7
attachThread@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/attach-thread.js:58:10
open/<@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js:464:34
Async*open@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js:431:19
createToolbox@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/devtools.js:564:11
async*showToolbox@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/devtools.js:477:30
async*openToolbox/<@chrome://devtools/content/framework/connect/connect.js:235:5
promise callback*openToolbox@chrome://devtools/content/framework/connect/connect.js:233:39
onConnectionReady/a.onclick/<@chrome://devtools/content/framework/connect/connect.js:133:18

We are falling into the third branch of the following if/else of attach-thread.js:

    if (target.isBrowsingContext) {
      // Attaching a tab, a browser process, or a WebExtensions add-on.
      target.activeTab.attachThread(threadOptions).then(handleResponse);
    } else if (target.isAddon) {
      // Attaching a legacy addon.
      target.client.attachAddon(actor).then(([res]) => {
        target.client.attachThread(res.threadActor).then(handleResponse);
      });
    } else {
      // Attaching an old browser debugger or a content process.
      target.client.attachThread(chromeDebugger).then(handleResponse);
    }

Where chromeDebugger is expected to be in the form and to be the actorID of the thread we want to be attached to. For the connect page, we receive the form from a call to client.getProcess() and it doesn't have this chromeDebugger property. If I decorate the form with a chromeDebugger property as follows in connect.js

function openToolbox(form, chrome = false, tool = "webconsole", isBrowsingContext) {
  if (chrome) {
    form = Object.assign({}, form, { chromeDebugger: form.actor });
  }
  // ...
}

then some tools show up but nothing is functional (console not working, blank debugger etc...).

Part of the issue might be that we are trying to debug a ParentProcessTarget which extends BrowsingContextTarget, but when debugging xpcshell tests this ParentProcessTarget does not actually have a window and cannot really behave as a BrowsingContextTarget.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #8)
> I am facing some issues with isBrowsingContext.
> 
> `isBrowsingContext` is true when debugging xpcshell tests from the Connect
> page. This is because we default isBrowsingContext to true when the
> parameter is not a boolean [1] and the Connect page is passing undefined for
> the "debug process" link [2].

Oh, that 3 branches code with undefined = true is solely there for backward compatibility with old addons,
we should get rid of that complexity!

> So I fixed that, by using `openToolbox(aResponse.form, true, "webconsole",
> false);`

By getting rid of that complexity, we should at the end do that, and be explicit about this flag in all callsites I think.

> After that the whole toolbox is blank. The connect page is always creating a
> "tab-target-like" targets [3].
> 
> [3]
> https://searchfox.org/mozilla-central/source/devtools/client/framework/
> connect/connect.js#233

Actually, except for workers, all target are created via TargetFactory.forRemoteTab.
See for example the browser content toolbox target:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#326-332
That's one thing, I hope to clarify after fission refactoring:
* have a common base class for all targets (including workers)
* then have inherited classes for each that require some specifics

But for now, I'm focusing on converting TabClient to protocol.js front and trying to merge TabClient with TabTarget.

https://searchfox.org/mozilla-central/source/testing/xpcshell/head.js#397
> 
> With isBrowsingContext set to false, we crash with the following stack trace:
> 
> request@resource://devtools/shared/base-loader.js ->
> resource://devtools/shared/client/debugger-client.js:525:13
> attachThread@resource://devtools/shared/base-loader.js ->
> resource://devtools/shared/client/debugger-client.js:435:12
> attachThread/<@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/framework/attach-thread.js:106:7
> attachThread@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/framework/attach-thread.js:58:10
> open/<@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/framework/toolbox.js:464:34
> Async*open@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/framework/toolbox.js:431:19
> createToolbox@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/framework/devtools.js:564:11
> async*showToolbox@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/framework/devtools.js:477:30
> async*openToolbox/<@chrome://devtools/content/framework/connect/connect.js:
> 235:5
> promise
> callback*openToolbox@chrome://devtools/content/framework/connect/connect.js:
> 233:39
> onConnectionReady/a.onclick/<@chrome://devtools/content/framework/connect/
> connect.js:133:18
> 
> We are falling into the third branch of the following if/else of
> attach-thread.js:

Yes, we should not set isBrowsingContext=false is the target actor is a browsing context one...
So we should either keep it true and fix things elsewhere
or make it false and fetch another kind of target actor (most likely a new kind, similar to ContentProcessTargetActor)

> Where chromeDebugger is expected to be in the form and to be the actorID of
> the thread we want to be attached to. For the connect page, we receive the
> form from a call to client.getProcess() and it doesn't have this
> chromeDebugger property. If I decorate the form with a chromeDebugger
> property as follows in connect.js

I wouldn't go down that road!
 
> Part of the issue might be that we are trying to debug a ParentProcessTarget
> which extends BrowsingContextTarget, but when debugging xpcshell tests this
> ParentProcessTarget does not actually have a window and cannot really behave
> as a BrowsingContextTarget.

Yes, that's the precise issue here!
For some reason I thought that xpcshell was using a non-browsing-context target actor, similar to content process one...

It relates to this very specific code right here:
https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/parent-process.js#78-83
  // On XPCShell, there is no window/docshell
  const docShell = window ? window.docShell : null;
  Object.defineProperty(this, "docShell", {
    value: docShell,
    configurable: true
  });

It ends up being weird to have a BrowsingContextActor without any related document...
It feels quite expected to have exceptions after that. I'm wondering why the inspector wasn't throwing already?

I see two options here:
1) Make it so that RootActor.getProcess(0) returns another non-browsing-context-actor:
  https://searchfox.org/mozilla-central/source/devtools/server/actors/root.js#512-529
  If xpcshell, then return an actor similar to ContentProcessActor.
  Then, somehow ensure that target.isBrowsingContext is false for xpcshell or, introduce a new flag on target: target.isXPCShell, like target.isAddon, that matches the addon id:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#348-359

2) Introduce a trait on browsing context actor:
  https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#246-258
  a traits to say if the actor has a valid window:
  this.traits = {
    ...
    hasValidWindow: !!this.docShell,
    ...
  };
  But that isn't that easy, as ParentProcecssTargetActor calls BrowsingContextActor.initialize *after* setting "docshell" property...
  https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/parent-process.js#48-83
  So it would be better to set this trait property on attach:
  https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#920-928
  We may also name this trait "isBrowsingContext". But it may be more confusing that helping. The target actor would still be a browsing context actor but a kind of degraded one...

  Also, I don't know why the traits aren't passed in the actor's form:
    https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#459-492
  It would have been so much more helpful... because it would help knowing if the target actor is a browsing context one right here:
    https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#489-498
    i.e. before we have to know if we have to call attach on the actor or not...
  May be we should start having traits on the form rather than the attach's results???
  If we do that, we could do:
    target.isBrowsingContext = !!this._form.isBrowsingContext;
  And stop having to pass it as forTab/forRemoteTab argument.
  I opened bug 1492434 to followup on this.

May be we should do both?
Thanks for the detailed answer! 

> Oh, that 3 branches code with undefined = true is solely there for
> backward compatibility with old addons, we should get rid of that complexity!

Great to hear, I assume this can be in scope of bug 1492434? 

> I wouldn't go down that road!

My comment was a bit of a travel log :) by the end I realized it was wrong to try and force isBrowsingContext=false here (without deeper refactoring as you suggested).

> I'm wondering why the inspector wasn't throwing already?

The inspector is not working at all, but it doesn't seem to impact the console. Not sure why the accessibility panel makes the console blank though.

> We may also name this trait "isBrowsingContext". But it may be more confusing that helping.

I like your "hasValidWindow" or even "hasWindow". As a consumer, it makes sense to check this flag before calling something that will need a valid window to be used. Much more than isBrowsingContext.  

> Also, I don't know why the traits aren't passed in the actor's form:
> ... 
> May be we should start having traits on the form rather than the attach's results???

That sounds good. Unless we find a technical issue, returning the traits in the form feels more consistent with the rest of our architecture.

> 1) Make it so that RootActor.getProcess(0) returns another non-browsing-context-actor:
> 2) Introduce a trait on browsing context actor:
> May be we should do both?

I think it makes sense:
1. introduce the hasValidWindow trait
2. update consumers of isBrowsingContext to use this trait when possible
3. create a new type of target for xpcshell parent process, that doesn't extend browsing context target
4. simplify browsing context target to remove the xpcshell-only code paths

1 and 2 seem straightforward to do, for 3 and 4 I think we have to think a bit about the common interface shared by all targets and how they are used by the various actors. It would be nice to have a clear set of mandatory methods/properties that must exist on all targets, and others that can exist and can be detected. It seems like it's exactly what is on your radar for fission so that's great.

So I would really start with the trait, and change the target in a second step.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #10)
> Thanks for the detailed answer! 
> 
> > Oh, that 3 branches code with undefined = true is solely there for
> > backward compatibility with old addons, we should get rid of that complexity!
> 
> Great to hear, I assume this can be in scope of bug 1492434?

yes.

> > We may also name this trait "isBrowsingContext". But it may be more confusing that helping.
> 
> I like your "hasValidWindow" or even "hasWindow". As a consumer, it makes
> sense to check this flag before calling something that will need a valid
> window to be used. Much more than isBrowsingContext.


> > 1) Make it so that RootActor.getProcess(0) returns another non-browsing-context-actor:
> > 2) Introduce a trait on browsing context actor:
> > May be we should do both?
> 
> I think it makes sense:
> 1. introduce the hasValidWindow trait
> 2. update consumers of isBrowsingContext to use this trait when possible
> 3. create a new type of target for xpcshell parent process, that doesn't
> extend browsing context target
> 4. simplify browsing context target to remove the xpcshell-only code paths

Don't you think 4 (bug 1492434) is going to conflict/reimplement 1?
Should we really have two traits?
One for identifying "browsing context targets" and another one for identifying "browsing-context-less browsing context targets" ? :p

> 1 and 2 seem straightforward to do, for 3 and 4 I think we have to think a
> bit about the common interface shared by all targets

That issue of not having a common interface already exists. Piling up yet another such actor won't change much the existing complexity.

> and how they are used by the various actors. It would be nice to have a clear set of mandatory
> methods/properties that must exist on all targets, and others that can exist
> and can be detected. It seems like it's exactly what is on your radar for
> fission so that's great.

Yes, it is on the radar, but I don't have the exhaustive plan yet.
I would like to have this base target class, defining the minimum base target API.
But it is not clear yet how to accommodate that with protocol.js front classes, as target will be a front.
For now, I'm focusing on getting classes to be merged (TabClient/TabTarget) while also converting TabClient to a p.js front.

> So I would really start with the trait, and change the target in a second
> step.

Sounds good to me. It just looks like you want to do a partial part of bug 1492434. If that's helpful you may introduce the same trait, but use it only for xpcshell codepath if you don't want to do the full patch, including handling backward compatibility.
Priority: -- → P1
Hum. It looks like xpcshell debugging is broken again.
I get this exception now:
  query object's 'source' property is not undefined nor a Debugger.Source object
And the toolbox immediately closes.

I opened bug 1495673 about that.
I would like to work on this bug as I started looking at bug 1492434, but well, everything is broken for now.
As Mark said on the other bug, this failure seems to be specific to one test.
So I'll try to fix the console.

I would like to suggest switching to ContentProcessTargetActor as it looks very hacky to use a ParentProcessTargetActor/BrowsingContextTargetActor for xpcshell as we really won't have any browsing context to debug.
The only thing is that it looks confusing to use it, but just because of its name.
We use the ContentProcessTargetActor to debug xpcshell "parent" process... But passed that, it makes a lot of sense.
We may fork it for sole xpcshell purpose, but that would almost be a copy paste to maintain.
Assignee: jdescottes → poirot.alex
MozReview-Commit-ID: J9XTgC0EBPG

Depends on D7415
The attached patch depends on 1492434, but not because of `isBrowsingContext: !this.docShell` from actors/targets/browsing-context.js.
Instead, I make it so that RootActor.getProcess(0), for xpcshell only, returns a ContentProcessTargetActor.
I'm not sure using a BrowsingContextTargetActor is a good idea... and ContentProcessTargetActor seems to match xpcshell usage very well. Only the name is confusing...

It is hard to find a better and concise name.
We could share it under these names:
* ProcessBrowsingContextLessTargetActor
* BrowsingContextLessProcessTargetActor
* ContextLessProcessTargetActor
* DocumentLessProcessTargetActor
* WindowLessProcessTargetActor
* ProcessTargetActor
* ...?

That's a target actor to inspect any privileged resource from a given process, that, without trying to attach to any document.
May be that's not such a big deal to use it as-is with its current name if we put the necessary comments in RootActor.getProcess?
Depends on: 1492434
I think for now we can live with ContentProcessTargetActor + comments. In the long run I think ProcessTargetActor would be ok, as long as we have the target actors hierarchy in our docs. So that we understand quickly which extends which etc...
Depends on: 1496025
Now that xpcshell no longer uses ParentProcessTargetActor, we can remove comments about it using it.
We can also remove a couple of null checks against docShell that were specific to this usecase.

MozReview-Commit-ID: 67sugv4bZC3

Depends on D7416
Now that xpcshell no longer uses ParentProcessTargetActor, we can remove comments about it using it.
We can also remove a couple of null checks against docShell that were specific to this usecase.

MozReview-Commit-ID: 67sugv4bZC3

Depends on D7416
Attachment #9014409 - Attachment is obsolete: true
So I rebased on top of promises actor removal and piled up another changeset to remove xpcshell specifics from BrowsingContextTargetActor. It looks like tests are happy with this change!
Blocks: 1497226
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9259cfe05c4e
Use a content process target actor for xpcshell debugging r=jdescottes
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/26d2f8e018b5
Use a content process target actor for xpcshell debugging r=jdescottes on a CLOSED TREE
Backed this out by mistake, relanded after. I apologize for the confusion, if any.
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/632c0d2d8c24
Backed out 3 changesets (bug 1492265, bug 1496025) for failing org.mozilla.geckoview.test.AccessibilityTest.testMoveByCharacter on a CLOSED TREE
This bug was dependent on bug 1496025, so I was expecting it to be backed out as well.
Attachment #9013588 - Attachment description: Bug 1492265 - Use a content process target actor for xpcshell debugging → Bug 1492265 - Use a content process target actor for xpcshell debugging.
Attachment #9014410 - Attachment description: Bug 1492265 - Remove xpcshell specifics from BrowsingContextTargetActor and ParentProcessTargetActor → Bug 1492265 - Remove xpcshell specifics from BrowsingContextTargetActor and ParentProcessTargetActor.
I'm reverting my patch to no longer depend on removing the promises actor as it is harder than expected to remove it...

try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b325d607c348341662e3cad624bd1512b8d130e
No longer depends on: 1496025
Flags: needinfo?(poirot.alex)
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92eadc28f033
Use a content process target actor for xpcshell debugging. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/3135daac034f
Remove xpcshell specifics from BrowsingContextTargetActor and ParentProcessTargetActor. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/92eadc28f033
https://hg.mozilla.org/mozilla-central/rev/3135daac034f
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Thank you for fixing this. I needed it today and it worked great!

Even better, I think this also fixed bug 1441956.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.