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

NEW
Unassigned

Status

defect
5 years ago
Last month

People

(Reporter: markh, Unassigned)

Tracking

(Blocks 1 bug, {reproducible})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

5 years ago
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.
Reporter

Comment 1

5 years ago
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.
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?"

Updated

2 years ago
Has STR: --- → yes
Keywords: reproducible
OS: Windows 7 → All
Hardware: x86_64 → All
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.
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?
Flags: needinfo?(dtownsend)
Matt might know.
Flags: needinfo?(dtownsend) → needinfo?(mhowell)
No, we don't currently have a way to disable processing updates on startup.
Flags: needinfo?(mhowell)
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).
(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?
Would it be sufficient to set the pref `app.update.enabled` to `false` in BT?
(Oh, sorry, just saw comment 9...)
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.
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.
Flags: needinfo?(mhowell)
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
Flags: needinfo?(mhowell)
(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.
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
(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].
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
Duplicate of this bug: 1424566
Duplicate of this bug: 1437239

Updated

Last year
Product: Firefox → DevTools
(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.
You need to log in before you can comment on or make changes to this bug.