Bug 2068283 - Generic console support - #1339
Conversation
| NS_ENSURE_SUCCESS(rv, rv); | ||
| } | ||
|
|
||
| if (serverUrl.EqualsLiteral(ENTERPRISE_CONSOLE_PLACEHOLDER)) { |
There was a problem hiding this comment.
Out of safety I think this should be reversed: if the value is not the place holder, immediately return NS_ERROR_NOT_AVAILABLE (or maybe NS_ERROR_INVALID_ARG would be more meaningful
There was a problem hiding this comment.
That's not the flow here. If the value is not the place holder we need to continue. NS_ERROR_NOT_AVAILABLE is only returned if there's no URL stored / in evn.
| # FIREFOX_ENTERPRISE_GENERIC is a placeholder: on first launch the console | ||
| # setup dialog asks for the address and persists it in felt.json. Repacks | ||
| # replace it with the actual console address. | ||
| enterprise.console.address=FIREFOX_ENTERPRISE_GENERIC |
There was a problem hiding this comment.
But then we break running local builds without requiring extra step from developpers, which adds friction
There was a problem hiding this comment.
I'd rather prefer that we do not change the default distribution.ini to such placeholder. We may at some point want to point it to a different instance than stage though ?
But I'd prefer if we had a dedicated repack that will set the placeholder value
This way, local build are still immediately actionable and the console setup step works like you want from the specific repack
There was a problem hiding this comment.
It can also make sure that I cannot inadvertenly have a wrong console in memory of my felt.json storage that gets picked up when running random builds, scoping the feature to those generic repacks
There was a problem hiding this comment.
We need to be extra careful about reviewing further changes using those function, as we may end up in a case where FeltStorage.sys.mjs and those are manipulating the same files at the same time. I think for the time being it's OK, even on macOS where we may have running processes without a window.
But maybe a naïve locking mechanism may be good to protect us for the future?
There was a problem hiding this comment.
Yes, we might want to think about this outside of this PR. Multiple Enterprise instances would read/write from that file concurrently right now.
There was a problem hiding this comment.
Can we file a follow up for a trivial locking system (e.g. .lock file) at least? To protect us?
Other option, but it would start to get out of scope so a follow up would be nice, would be to convert the FeltStorage code to directly rely on rust so the locking could be done there?
| if (!(this._feltStorage instanceof lazy.JSONFile)) { | ||
| this._feltStorage = new lazy.JSONFile({ | ||
| path: this.FELT_FILE_PATH, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Should we rather guard with a boolean, e.g. this._initialized ?
There was a problem hiding this comment.
Also why do we need this?
I assume it's because we hit now two call sites of FeltStorage.init() one in ConsoleClient and one in consoleSetup.js ?
| nsAutoString path; | ||
| rv = file->GetPath(path); | ||
| NS_ENSURE_SUCCESS(rv, rv); | ||
| CopyUTF16toUTF8(path, aOutPath); |
There was a problem hiding this comment.
nsCString is safe for windows paths ? What if the path contains UTF-8 or UTF-16 chars?
There was a problem hiding this comment.
| XRE_ParseEnterpriseServerURL(*mAppData); | ||
| // Ignoring nsresult. If console url is not found in a release build, the | ||
| // default server url is empty and crash reports will fail to submit. | ||
| if (CheckArg("reset-console-address") == ARG_FOUND) { |
There was a problem hiding this comment.
So firefox --reset-console-address would allow to trigger again the dialog, but only if its distribution.ini contains the placeholder value.
At my first reading of this part of the code I was afraid, so it may be worth commenting here to make this context crystal clear
There was a problem hiding this comment.
Can you suggest an improvement to my comments?
There was a problem hiding this comment.
I'll try to come up with something yes
| // Never block automation on the interactive setup dialog: test harnesses | ||
| // launch the browser before any console address exists (Marionette's first | ||
| // launch precedes the test setUp that provides one via distribution.ini or | ||
| // MOZ_ENTERPRISE_CONSOLE_ADDRESS) and would time out waiting for it. |
There was a problem hiding this comment.
I think this becomes irrelevant if we move to making such a generic repack instead of changing the default distribution.ini value and the whole checks can be simplified
| const bool enterpriseConsoleSetupAllowed = | ||
| !EnvHasValue("MOZ_AUTOMATION") && !CheckArgExists("marionette"); | ||
| if (gEnterpriseConsoleSetupNeeded && enterpriseConsoleSetupAllowed && | ||
| is_felt_ui() | ||
| # ifdef MOZ_BACKGROUNDTASKS | ||
| && !BackgroundTasks::IsBackgroundTaskMode() | ||
| # endif | ||
| ) { | ||
| rv = ShowEnterpriseConsoleSetup(mNativeApp); | ||
| if (rv == NS_ERROR_LAUNCHED_CHILD_PROCESS || rv == NS_ERROR_ABORT) { | ||
| *aExitFlag = true; | ||
| return 0; | ||
| } | ||
| if (NS_FAILED(rv)) { | ||
| return 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementing via a generic repack instead of changing local builds values, we should be able to simplify to
| const bool enterpriseConsoleSetupAllowed = | |
| !EnvHasValue("MOZ_AUTOMATION") && !CheckArgExists("marionette"); | |
| if (gEnterpriseConsoleSetupNeeded && enterpriseConsoleSetupAllowed && | |
| is_felt_ui() | |
| # ifdef MOZ_BACKGROUNDTASKS | |
| && !BackgroundTasks::IsBackgroundTaskMode() | |
| # endif | |
| ) { | |
| rv = ShowEnterpriseConsoleSetup(mNativeApp); | |
| if (rv == NS_ERROR_LAUNCHED_CHILD_PROCESS || rv == NS_ERROR_ABORT) { | |
| *aExitFlag = true; | |
| return 0; | |
| } | |
| if (NS_FAILED(rv)) { | |
| return 1; | |
| } | |
| } | |
| const bool enterpriseConsoleSetupAllowed = is_felt_ui() | |
| # ifdef MOZ_BACKGROUNDTASKS | |
| && !BackgroundTasks::IsBackgroundTaskMode() | |
| # endif | |
| ; | |
| if (gEnterpriseConsoleSetupNeeded && enterpriseConsoleSetupAllowed) { | |
| rv = ShowEnterpriseConsoleSetup(mNativeApp); | |
| if (rv == NS_ERROR_LAUNCHED_CHILD_PROCESS || rv == NS_ERROR_ABORT) { | |
| *aExitFlag = true; | |
| return 0; | |
| } | |
| if (NS_FAILED(rv)) { | |
| return 1; | |
| } | |
| } |
There was a problem hiding this comment.
I'll look at this part tomorrow.
|
@fiji-flo Thanks for taking care of this, I think it's overall right, I just really want that we do not change the default build output and rather make such a "generic" repack that will run on CI |
6c4d199 to
aecca40
Compare
5268507 to
2340dbd
Compare

Description
I'll edit this later, opening for sharing early.
Bugzilla: Bug-2068283
Every enterprise deployment currently requires a per-customer repack that bakes the console address into the AutoConfig file (
firefox.cfg). This adds a generic build: one artifact that ships without a console address and gets configured at first startup, so the same build can serve any organization (downloads, evaluations, QA).Implementation Details:
--with-enterprise-console-urlstill bakes a real address in for repacks. Without it the build is generic andfirefox.cfgcarries the placeholderFIREFOX_ENTERPRISE_GENERICMOZ_ENTERPRISE_CONSOLE_URLenvironment variable (test harness override),consoleAddresspersisted infelt.json(UAppData) by the setup dialog,chrome://felt/content/consoleSetup.xhtml, FELT UI only; skipped underMOZ_AUTOMATIONand in background-task mode) asks for the address, persists it tofelt.json, and relaunches so the early consumers (crash reporterServerURL, update URL, FELT connection) see the configured value from the start.--reset-console-addressclears the stored address so the dialog runs again.enterprise-consolecrate (toolkit/components/enterprise/rust), shared by all native consumers: browser startup through thefeltcrate's FFI (XRE_ReadEnterpriseConsoleAddress/XRE_ParseEnterpriseServerURLintoolkit/xre/CreateAppData.cpp), and the standalone crash reporter client.ConsoleClient.sys.mjsmirrors it in JS.MOZ_ENTERPRISE_CONSOLE_URLto a closed local port by default so generic builds never block on the dialog under automation.Screenshots
Testing