1599962 - Omniscient Browser Toolbox can't open remote <browser> elements when debugging non-browser windows

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[thomasmo]

Thank you for helping make Firefox better. If you are reporting a defect, please complete the following:

What were you doing?

Please tell us what site you were on, and what steps led to the error you are reporting

1. Turn on Omniscient mode
2. Run "firefox.exe --chrome chrome://fxr/content/fxrui.html --jsdebugger --console"
   (--console is just to see the exception stack since toolbox cannot display it)

What happened?

devtoolbox window opens, but no UI is shown (white screen)

Stack: _getChildBrowsingContexts@resource://devtools/server/actors/root.js:605:9
onListRemoteFrames@resource://devtools/server/actors/root.js:619:39
onPacket@resource://devtools/server/debugger-server-connection.js:378:58
_onJSONObjectReady/<@resource://devtools/shared/transport/transport.js:464:22
exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:111:22
exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:111:22
Line: 605, column: 9" "Protocol error (unknownError): error occurred while processing 'listRemoteFrames': TypeError: window is null
Stack: _getChildBrowsingContexts@resource://devtools/server/actors/root.js:605:9
onListRemoteFrames@resource://devtools/server/actors/root.js:619:39
onPacket@resource://devtools/server/debugger-server-connection.js:378:58
_onJSONObjectReady/<@resource://devtools/shared/transport/transport.js:464:22
exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:111:22
exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:111:22
Line: 605, column: 9"
undefined

What should have happened?

devtoolbox starts UI for debugging all processes

Note: repros on Windows nightly build. not confirmed on other platforms

Comment 1

[thomasmo]

Note: there was some discussion of this issue here
https://phabricator.services.mozilla.com/D54652#inline-332156

Comment 2

[thomasmo]

Also--note, simply checking for existence of window at least unblocks the scenario locally (not sure of any side effects)

Comment 3

[:Gijs (he/him)]

(In reply to thomasmo from comment #1)

Note: there was some discussion of this issue here
https://phabricator.services.mozilla.com/D54652#inline-332156

I'll echo my comment here to keep the discussion in one place. Re: https://searchfox.org/mozilla-central/rev/4df8821c1b824db5f40f381f48432f219d99ae36/devtools/server/actors/root.js#599-609

This is because it's looking for specific window types. So in the Firefox desktop case, it'll be looking for "normal" browser windows, but if we're running FXR then the FXR window isn't one of those.

It does look like this code expects there to always be at least one of these windows, which can also break in other ways, e.g. on macOS you can close all open browser windows but keep the app open. The nullcheck will work around all this for now, but I don't really understand why the specialcase for browser windows exists. It'd seem like we could use the BrowsingContext.get(id) version of the code (below) everywhere, but there must have been a reason to add the specialcase, only the comment doesn't say - it just says we "need to ... in a special way", which doesn't help... It looks like @yulia wrote this - perhaps she can clarify?

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #3)

(In reply to thomasmo from comment #1)

Note: there was some discussion of this issue here
https://phabricator.services.mozilla.com/D54652#inline-332156

I'll echo my comment here to keep the discussion in one place. Re: https://searchfox.org/mozilla-central/rev/4df8821c1b824db5f40f381f48432f219d99ae36/devtools/server/actors/root.js#599-609

This is because it's looking for specific window types. So in the Firefox desktop case, it'll be looking for "normal" browser windows, but if we're running FXR then the FXR window isn't one of those.

It does look like this code expects there to always be at least one of these windows, which can also break in other ways, e.g. on macOS you can close all open browser windows but keep the app open. The nullcheck will work around all this for now, but I don't really understand why the specialcase for browser windows exists. It'd seem like we could use the BrowsingContext.get(id) version of the code (below) everywhere, but there must have been a reason to add the specialcase, only the comment doesn't say - it just says we "need to ... in a special way", which doesn't help... It looks like @yulia wrote this - perhaps she can clarify?

Yulia replied on phabricator:

Hmm there is a comment, but i agree -- it is too vague.

The purpose of this code is to list all children of a given browsing context. The reason this code does not use browsingContext.getChildren() is that at the time, browsingContext.getChildren() only worked on retrieving children of the same type as the parent. So if you called it from a parent window, you would not receive a nested iframe in the content window. That is what this comment was trying to say, but was unclear about:

// If we have the id of the parent, then we need to get the child
// contexts in a special way. We have a method on the descriptor
// to take care of this.

If the parent was not the top level window, we can just fall back on browsingContext.getChildren

BrowsingContext.get(id) has a different purpose, but it looks like the bug is not necessarily with this cross content type code?

Hm, so I wasn't aware of this, but yes, if you have a browsing context for the toplevel window, getChildren doesn't return the remote browsing contexts in that window (for e.g. tabs).

Could we do something along the lines of:

let browsingContext = BrowsingContext.get(id);
// If this is a parent-process window, and it's top-level (not embedded in a browser),
// collect all the remote browsers in the window ourselves. getChildren() will not return
// these contexts otherwise.
if (browsingContext.window && !browsingContext.embedderElement) {
  let { window } = browsingContext;
  return [
    ...window.document.querySelectorAll(`browser[remote="true"]`),
  ].map(browser => browser.browsingContext);
}

?

Separately the extant code (and the above strawman) currently do not collect non-remote browsers. Is that intentional? This means that e.g. if you have a browser window with the prefs open, the prefs' browsing context will not be included, as it's not remote. Ditto for about:addons and a few other about: pages, and potentially the sidebar (I forget, off-hand, when that is/isn't remote.).

Comment 5

[Julian Descottes [:jdescottes]]

Can you check if this is still an issue? I believe Bug 1599037 fixed the main crasher.

Comment 6

[:Gijs (he/him)]

(In reply to Julian Descottes [:jdescottes] from comment #5)

Can you check if this is still an issue? I believe Bug 1599037 fixed the main crasher.

FWIW, although it looks like that change would this this (I'll let Thomas confirm) I still suspect that my suggestion in comment #4 would be useful so the toolbox would actually allow selecting the FXR window and its content browser, and potentially for things like the layout debugger. Would that make sense or am I missing something?

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to :Gijs (he/him) from comment #6)

(In reply to Julian Descottes [:jdescottes] from comment #5)

Can you check if this is still an issue? I believe Bug 1599037 fixed the main crasher.

FWIW, although it looks like that change would this this (I'll let Thomas confirm) I still suspect that my suggestion in comment #4 would be useful so the toolbox would actually allow selecting the FXR window and its content browser, and potentially for things like the layout debugger. Would that make sense or am I missing something?

Sorry! I don't mean to close the bug as duplicate, mostly wanted to clarify the status for triage.

With the latest Nightly, the OBT can inspect the FXR window and navbar, although it can't go inside the browser element.
Quickly tried your suggestion and it does enable expanding the <browser> element.

Comment 8

[thomasmo]

(FWIW-- yes, it looks like Bug 1599037 addresses the initial crash reported)

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1599962 - OBT should list remote <browser> elements when debugging non browser windows

Code change suggested by :Gijs to support non browser windows

Comment 10

[Alexandre Poirot [:ochameau]]

Note that bug 1593937 may address even more edge cases around this particular code.
But the attached patch should address this one edge case immediately.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d86fd2d397dc
OBT should list remote <browser> elements when debugging non browser windows r=ochameau

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d86fd2d397dc