Closed
Bug 1492265
Opened 6 years ago
Closed 6 years ago
Console is completely broken when debugging xpcshell-tests
Categories
(DevTools :: Console, defect, P1)
DevTools
Console
Tracking
(firefox-esr60 unaffected, firefox62 unaffected, firefox63 wontfix, firefox64 fixed)
VERIFIED
FIXED
Firefox 64
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox62 | --- | unaffected |
firefox63 | --- | wontfix |
firefox64 | --- | fixed |
People
(Reporter: standard8, Assigned: ochameau)
References
Details
(Keywords: regression)
Attachments
(3 files, 1 obsolete file)
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}]"
Comment 1•6 years ago
|
||
Might be a regression from Bug 1414597
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment 2•6 years ago
|
||
(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.
Comment 4•6 years ago
|
||
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)
Assignee | ||
Comment 5•6 years ago
|
||
(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, ...
Assignee | ||
Updated•6 years ago
|
Attachment #9010159 -
Flags: feedback?(poirot.alex)
Comment 6•6 years ago
|
||
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;
},
Assignee | ||
Comment 7•6 years ago
|
||
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.
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
(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?
Reporter | ||
Updated•6 years ago
|
Keywords: regression
Comment 10•6 years ago
|
||
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.
Assignee | ||
Comment 11•6 years ago
|
||
(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.
Updated•6 years ago
|
Priority: -- → P1
Assignee | ||
Comment 12•6 years ago
|
||
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.
Assignee | ||
Comment 13•6 years ago
|
||
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
Assignee | ||
Comment 14•6 years ago
|
||
MozReview-Commit-ID: J9XTgC0EBPG
Depends on D7415
Assignee | ||
Comment 15•6 years ago
|
||
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
Comment 16•6 years ago
|
||
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...
Assignee | ||
Comment 17•6 years ago
|
||
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
Assignee | ||
Comment 18•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Attachment #9014409 -
Attachment is obsolete: true
Assignee | ||
Comment 19•6 years ago
|
||
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!
Assignee | ||
Comment 20•6 years ago
|
||
Comment 21•6 years ago
|
||
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9259cfe05c4e
Use a content process target actor for xpcshell debugging r=jdescottes
Comment 22•6 years ago
|
||
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
Comment 23•6 years ago
|
||
Backed this out by mistake, relanded after. I apologize for the confusion, if any.
Comment 24•6 years ago
|
||
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
Assignee | ||
Comment 25•6 years ago
|
||
This bug was dependent on bug 1496025, so I was expecting it to be backed out as well.
Comment 26•6 years ago
|
||
Backed out for failing xpcshell at devtools/server/tests/unit/test_promises_actor_attach.js
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending,running,success,testfailed,busted,exception&classifiedState=unclassified&group_state=expanded&searchStr=linux,x64,debug,xpcshell,tests,test-linux64%2Fdebug-xpcshell-2,x(x2)&selectedJob=204040532&revision=26d2f8e018b50fb4638ac9278d959103729fa507
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=204040532&repo=autoland&lineNumber=1953
Backout: https://hg.mozilla.org/integration/autoland/rev/88544bf70da7924bf86c0f90d6e4d21a5c72fa11
Flags: needinfo?(poirot.alex)
Updated•6 years ago
|
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.
Updated•6 years ago
|
Attachment #9014410 -
Attachment description: Bug 1492265 - Remove xpcshell specifics from BrowsingContextTargetActor and ParentProcessTargetActor → Bug 1492265 - Remove xpcshell specifics from BrowsingContextTargetActor and ParentProcessTargetActor.
Assignee | ||
Comment 27•6 years ago
|
||
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)
Comment 28•6 years ago
|
||
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
Comment 29•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/92eadc28f033
https://hg.mozilla.org/mozilla-central/rev/3135daac034f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Updated•6 years ago
|
status-firefox62:
--- → unaffected
status-firefox63:
--- → wontfix
status-firefox-esr60:
--- → unaffected
Reporter | ||
Comment 30•6 years ago
|
||
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.
Description
•