Closed Bug 1120863 Opened 10 years ago Closed 4 years ago

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

Categories

(DevTools :: General, defect, P3)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(firefox88 fixed)

Status:
RESOLVED FIXED
Milestone:
88 Branch
Tracking Flags:
Tracking Status
firefox88 --- fixed

People

(Reporter: markh, Assigned: nalexander)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: reproducible)

Attachments

(1 file, 1 obsolete file)

Bug 1120863 - Do not process updates when launching devtools. r?mhowell
(deleted), text/x-phabricator-request
Details
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.
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?"
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?
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
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.

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.

Flags: needinfo?(mhowell)
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1583806

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.

Flags: needinfo?(mhowell) → needinfo?(robert.strong.bugs)

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

Flags: needinfo?(robert.strong.bugs)
Severity: normal → S3
Priority: -- → P3

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.

Assignee: nobody → nalexander
Status: NEW → ASSIGNED

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.

Attachment #8929575 - Attachment is obsolete: true
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0f6f9ff1c44f Do not process updates when launching devtools. r=mhowell,ochameau

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox88: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: