1120863 - Browser toolbox starts blank and doesn't function with pending updates

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Mark Hammond [:markh] [:mhammond]]

STR:
* Wait for an update to be pending.
* Devtools -> Browser Toolbox
* Notice Firefox remains open and another "installing update" progress dialog appears (presumably due to the second firefox.exe starting to host the toolbox)
* Toolbox finally opens, but is completely blank (no title, no content, no theme) and obviously nothing works.

I've seen this for a few months now and is on DevEd so e10s is disabled.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Oh - and FTR, closing the toolbox (other firefox.exe goes away) and reopening it (another starts) without restarting the "parent" process seems to work fine.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The browser toolbox uses a separate instance of Firefox so I can see why this happens.

Maybe we should detect pending updates and show a dialog, something like:
"The browser needs to be restarted in order to open the browser toolbox due to pending updates. Would you like to restart your browser?"

Comment 4

[Ted Campbell [:tcampbell]]

Now that nightly updates twice a day, it seems likely people are going to hit this more an more. I like Michael's suggestion of have a dialog informing the user to restart. I frequently find that it just silently fails to do anything at all and had no idea I required resolving updates first.

Comment 5

[Brian Grinstead [:bgrins]]

Mossop, is there a command line option / environment variable / preference we could set to prevent installing a pending update when the Browser Toolbox profile starts up?

Comment 6

[Brian Grinstead [:bgrins]]

The process starts (using a separate profile) via https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm#280

Comment 7

[Dave Townsend [:mossop]]

Matt might know.

Comment 8

[Molly Howell (she/her) (mostly inactive)]

No, we don't currently have a way to disable processing updates on startup.

Comment 9

[Molly Howell (she/her) (mostly inactive)]

Which is not to say we couldn't add a command-line parameter or environment variable if that's what's needed to fix this (it couldn't be a pref because we process updates before we have a profile).

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Matt Howell [:mhowell] from comment #8)
> No, we don't currently have a way to disable processing updates on startup.

I'm guessing the easiest thing for the Browser Toolbox would be to set an environment variable telling us not to update, but it's also possible that there's a fix on the Browser Toolbox side. Is there a good way to simulate an update in a local build so I can figure out why this ends up breaking the browser toolbox?

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Would it be sufficient to set the pref `app.update.enabled` to `false` in BT?

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(Oh, sorry, just saw comment 9...)

Comment 13

[Molly Howell (she/her) (mostly inactive)]

Hmm, there's not an easy way to simulate an update that well; to trigger this bug you have to get an update to actually apply, and it's hard to forcibly convince the updater to go that far. We often test things that require actually running updates by pushing them to a project tree (we have the tree called oak informally reserved for that purpose), so that the CI builds us real updates. The easiest way to do this locally is probably to create the necessary files manually and then drop them in place to make the toolbox process think there's an update for it to apply when it starts, simulating what the update service would have done; I can help you out with that, if that's how you want to go.

Comment 14

[Brian Grinstead [:bgrins]]

If you think it'd be relatively easy to add support for an environment variable or command line flag that skips the update process, that would probably be the best path forward since it may end up being the required solution even after debugging the issue further.

Comment 15

[Molly Howell (she/her) (mostly inactive)]

I think all we should need to do is wrap the section of code at [0] in the env var check, and also keep the update service in that process from trying to... do much of anything really, which I think we could accomplish by also including a check of that variable in canCheckForUpdates [1].

Would there be any way to limit this to the toolbox, though? Adding a new method of disabling updates universally is a little more than I'm comfortable doing this lightly.

[0] https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/toolkit/xre/nsAppRunner.cpp#4095-4150
[1] https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/toolkit/mozapps/update/nsUpdateService.js#532

Comment 16

[Molly Howell (she/her) (mostly inactive)]

(In reply to Matt Howell [:mhowell] from comment #15)
> Would there be any way to limit this to the toolbox, though? Adding a new
> method of disabling updates universally is a little more than I'm
> comfortable doing this lightly.

In fact it just occurred to me this would probably be a substantial security problem, since it would give an unprivileged malicious actor a way to disable security updates.

Comment 17

[Brian Grinstead [:bgrins]]

Here are the args we currently pass to the Browser Toolbox process [0]:

  -no-remote
  -foreground
  -chrome chrome://devtools/content/framework/toolbox-process-window.xul
  -profile [profile directory + chrome_debugger_profile]

And the environment [1]:

  MOZ_DISABLE_SAFE_MODE_KEY,
  MOZ_BROWSER_TOOLBOX_PORT

Perhaps checking for the toolbox-process-window.xul chrome argument would be a safe way to check without exposing a new argument / environment variable.

[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm#253
[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm#259

Comment 18

[Brian Grinstead [:bgrins]]

(In reply to Matt Howell [:mhowell] from comment #15)
> I think all we should need to do is wrap the section of code at [0] in the
> env var check, and also keep the update service in that process from trying
> to... do much of anything really, which I think we could accomplish by also
> including a check of that variable in canCheckForUpdates [1].

For [1], it looks like we could set the existing app.update.enabled pref for the BT profile (which we can do here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox-process-window.js#69). So we'd only need a solution for [0].

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: Bug 1120863 - Don't allow updates during runtime for the Browser Toolbox profile

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

Comment 20

[Molly Howell (she/her) (mostly inactive)]

What I'm worried about is how the post-update logic that runs on startup [0] is going to handle this; setting app.update.enabled in the profile doesn't disable that (the pref has to be locked), and it won't be expecting to see an update in this state; what I think it's likely to do is just delete the in-progress update and force downloading it again.

[0] https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/toolkit/mozapps/update/nsUpdateService.js#1690

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Matt Howell [:mhowell] from comment #16)
> (In reply to Matt Howell [:mhowell] from comment #15)
> > Would there be any way to limit this to the toolbox, though? Adding a new
> > method of disabling updates universally is a little more than I'm
> > comfortable doing this lightly.
> 
> In fact it just occurred to me this would probably be a substantial security
> problem, since it would give an unprivileged malicious actor a way to
> disable security updates.

I'd be OK to disable only based on the chrome URL (passed via the -chrome flag as "chrome://devtools/content/framework/toolbox-process-window.xul") from https://searchfox.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm.

Comment 24

[Brian Grinstead [:bgrins]]

This is more relevant now that you can pass the nightly binary to -jsdebugger (Bug 1583806). I just ran into the broken case in with ./mach run -jsdebugger /Applications/FirefoxNightly.app/Contents/MacOS/firefox when my Nightly build had a pending update.

Molly, can you think of a path forward here especially regarding the suggestion in Comment 23 as a workaround for Comment 15? If I'm reading things correctly it also would work around Comment 20 by making the suggested change in Comment 19 obsolete.

Comment 25

[Molly Howell (she/her) (mostly inactive)]

Yeah, I agree with comment 23; adding an environment variable or something else seems superfluous because I don't think we would ever want to process updates in the dev tools, so we don't need some other flag to tell us whether or not to process updates, we can disable it based entirely on the value of the -chrome parameter. The one thing I would want to make sure of is that the correct URL value gets exported somehow so that we only have one copy of the string, because if there's more than one I don't want to run the risk of them getting out of sync.

I also think we could make disabling the update service easier by hooking into its disabledByPolicy property, similarly to how the disabledForTesting property works there.

Leaving an ni? for Robert because I also want to make sure this gets run by them before anything lands.

Comment 26

[Robert Strong (they/them - no direct email)]

Adding the chrome param and the url check in nsAppRunner.cpp sounds fine to me.

Comment 28

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1120863 - Do not process updates when launching devtools. r?mhowell

This implements the simple approach advocated for in
https://bugzilla.mozilla.org/show_bug.cgi?id=1120863#c26.

It's not easy to lift the duplicated chrome document URL to a shared
location, so we instead annotate both locations.

It's also not easy to test this functionality, but I've provided an
environment variable that will allow a process that did not itself
process updates to witness that fact. I intend to use it in a similar
ticket, Bug 1695797.

Comment 29

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8929575 [details]
Bug 1120863 - Don't allow updates during runtime for the Browser Toolbox profile

Marking this obsolete since I'm about to land a fine-grained approach to this particular problem.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f6f9ff1c44f
Do not process updates when launching devtools. r=mhowell,ochameau

Comment 31

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/0f6f9ff1c44f