1371548 - No Browser Toolbox

Status: VERIFIED FIXED

(DevTools :: General, defect, P1)

Description

[atlanto]

STR:
1. menu->Developer->Browser Toolbox

AR
Nothing happened.

ER:
Browser Toolbox window appears.

mozregression says
 6:09.02 INFO: Last good revision: 196bfa86b5a63c20f7f6fdf3e30d9ff3d8839de3
 6:09.02 INFO: First bad revision: 0d16a08bbcfa181535789a43856ef176cc9829c8
 6:09.02 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=196bfa86b5a63c20f7f6fdf3e30d9ff3d8839de3&tochange=0d16a08bbcfa181535789a43856ef176cc9829c8

 6:10.52 INFO: Looks like the following bug has the  changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1370027

OS:
Windows10 1703 (15063.296)

Under the profile folder, "chrome_debugger_profile" is being created but there is "prefs.js" only.

Comment 1

[magicp]

I have confirmed this issue in Windows7 and 10. unaffected in Ubuntu and macOS.

Comment 2

[Brian Grinstead [:bgrins]]

Hi Kris, we have a merge day coming up (June 12) and I don't want the Browser Toolbox to break on windows beta.  Would it be possible to either look into this prior to the merge or temporarily back out Bug 1370027 until we can make sure Windows is fixed?

Comment 3

[Kris Maglione [:kmag]]

I'll look into it today. In the worst case we can back out part 2 of bug 1370027, or fallback to the old code on Windows 10.

Comment 4

[Kris Maglione [:kmag]]

OK, so apparently the problem here is that we're creating the new toolbox process without redirecting stderr, so it only works when running with -console, and there's a console to inherit stderr from.

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1371548: Accept invalid stderr handles when spawning subprocesses.

Review commit: https://reviewboard.mozilla.org/r/147790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147790/

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8876451 [details]
Bug 1371548: Accept invalid stderr handles when spawning subprocesses.

https://reviewboard.mozilla.org/r/147790/#review152172

looks okay to me.  what about adding a test?

::: toolkit/modules/subprocess/subprocess_worker_win.js:384
(Diff revision 1)
> +      if (String(srcHandle) == win32.INVALID_HANDLE_VALUE ||
> +          String(srcHandle) == win32.NULL_HANDLE_VALUE) {

I don't totally understand this, these constants were declared with ctypes but you're comparing them as strings?

::: toolkit/modules/subprocess/subprocess_worker_win.js:449
(Diff revision 1)
>  
>      return `"${escaped}"`;
>    }
>  
>    spawn(options) {
> +  debug(`SPAWN PROCESS ${uneval(options)}`);

did you mean to leave this in?

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8876451 [details]
Bug 1371548: Accept invalid stderr handles when spawning subprocesses.

https://reviewboard.mozilla.org/r/147790/#review152172

We already have xpcshell tests for this, but they pass either way because they always have a console. I don't really want to add mochitests for this module.

> I don't totally understand this, these constants were declared with ctypes but you're comparing them as strings?

ctypes pointer comparisons need to be done as string comparisons, for complicated and stupid reasons.

> did you mean to leave this in?

Nope.

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7987c98de3ecb90a07e58a1337e10ae2d83bb4e
Bug 1371548: Accept invalid stderr handles when spawning subprocesses. r=aswan

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Brian Grinstead [:bgrins]]

Can you please verify the fix on m-c or in the next Nightly?

Comment 11

[magicp]

I have verified this issue was solved in the latest Nightly build (20170612030208). Thanks!