Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Windows silent installer truly silent #8364

Merged
merged 6 commits into from
Apr 13, 2021

Conversation

mherrmann
Copy link
Collaborator

@mherrmann mherrmann commented Mar 25, 2021

Resolves brave/brave-browser#6240

Before this PR, there were two silent installers: a 1 MB online ("stub") installer that fetched the latest version of Brave from the update server, and a full, offline installer. Both of these installers were not truly silent because they opened Brave.

In implementing this PR, it was decided that a 1 MB silent online installer is not needed. Part of the work thus consisted of removing this installer.

The remaining work was to make sure that the standalone silent installer does not open a browser window. This was achieved by updating Brave's Omaha update client to pass the flag --do-not-launch-chrome to Brave's installer. The corresponding PR was brave/omaha#31.

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Test that all Windows installers work as expected.

@mherrmann mherrmann requested a review from bridiver as a code owner March 25, 2021 19:07
@mherrmann mherrmann added CI/skip-android Do not run CI builds for Android CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Mar 25, 2021
@mherrmann mherrmann force-pushed the mherrmann-fix-silent-installer-bb-6240 branch from 8c13120 to dc370bb Compare April 2, 2021 09:36
@mherrmann mherrmann requested review from iefremov, tmancey and a team as code owners April 5, 2021 15:41
@mherrmann mherrmann force-pushed the mherrmann-fix-silent-installer-bb-6240 branch from 8dd4983 to fee3ddf Compare April 5, 2021 15:50
@iefremov iefremov removed their request for review April 6, 2021 16:25
@mherrmann
Copy link
Collaborator Author

@mihaiplesa the s3-upload step of CI failed because I removed BraveBrowserSilentNightlySetup_...exe. I can't find the code for s3-upload in brave-core. Is it in another repo?

@mihaiplesa
Copy link
Collaborator

mihaiplesa commented Apr 6, 2021

That is correct. I can fix that after we merge. Lint and some unit tests have failed for now.

@mherrmann mherrmann force-pushed the mherrmann-fix-silent-installer-bb-6240 branch from fee3ddf to 42a0161 Compare April 7, 2021 05:48
@mherrmann
Copy link
Collaborator Author

mherrmann commented Apr 7, 2021

Debugging the CI failures:

windows/pr-head: When I click on the failed CI link above, I get a red screen of all green steps:

image

I suspect something went wrong in the associated brave-browser build. There, it says:

hudson.plugins.git.GitException: Failed to fetch from https://github.com/brave/devops.git
	at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:999)
	at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1240)
	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1300)
	at org.jenkinsci.plugins.workflow.steps.scm.SCMStep.checkout(SCMStep.java:125)
	at org.jenkinsci.plugins.workflow.steps.scm.SCMStep$StepExecutionImpl.run(SCMStep.java:93)
	at org.jenkinsci.plugins.workflow.steps.scm.SCMStep$StepExecutionImpl.run(SCMStep.java:80)
	at org.jenkinsci.plugins.workflow.steps.SynchronousNonBlockingStepExecution.lambda$start$0(SynchronousNonBlockingStepExecution.java:47)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: hudson.plugins.git.GitException: Command "git fetch --tags --force --progress -- https://github.com/brave/devops.git +refs/heads/*:refs/remotes/origin/*" returned status code 128:
stdout: 
stderr: remote: Invalid username or password.

Assuming the failure of "my" pr-head was caused by this, then it would have saved me 10 minutes of searching if the b-b build were listed as a triggered build. (Just as a point of potential optimization for the future.)

pre-init/pr-head: This says it parsed pylint-report.txt. I can't find that file on CI but looking at the full log reveals some lines in the close vicinity that might be the cause of the problem (?):

[Pylint] -> found 65 issues (skipped 6 duplicates)
no-unused-variable is deprecated. Since TypeScript 2.9. Please use the built-in compiler checks instead.
no-use-before-declare is deprecated. Since TypeScript 2.9. Please use the built-in compiler checks instead.

I can't find no-unused-variable or no-use-before-declare in any of the files I changed. Maybe it was somebody else's change? Will rebase and rebuild to see.

@mherrmann
Copy link
Collaborator Author

@mihaiplesa I'm having a hard time understanding why pre-init/pr-head fails. Do you have a tip? I already wrote a little about my findings at the end of my previous comment above.

@mihaiplesa
Copy link
Collaborator

So pre-init failed because of pylint, but those files are not used anymore so we're good. Windows looks good but I have to fix the pipelines.

@mherrmann mherrmann force-pushed the mherrmann-fix-silent-installer-bb-6240 branch from 42a0161 to ebd8005 Compare April 7, 2021 17:12
@mihaiplesa mihaiplesa changed the title (WIP) Make Windows silent installer truly silent Make Windows silent installer truly silent Apr 7, 2021
@mherrmann
Copy link
Collaborator Author

mherrmann commented Apr 8, 2021

Test results, running Brave's Windows installer from the CI build:

  • NightlySetup.exe works as expected
  • StandaloneSilentNightlySetup.exe works as expected: installs Brave completely silently, also when there is no active internet connection
  • StandaloneNightlySetup.exe does install Brave (also without internet) but ends with an error screen:
    image

The associated error message in Brave's update log seems to be:

[Registered version does not match expected][{C6CB981E-DB30-4876-8639-109F8933582C}][90.1.25.1][90.1.25.4]

So: Brave's Update mechanism expected version .4 to be installed, but received version .1 instead in the registry. What's very weird is that brave://settings/help does show .4:

image

The installer files all reference version .4 in their name (eg. BraveBrowserNightlySetup_90_1_25_4.exe). Interestingly, StandaloneSilentNightlySetup.exe does install .4. So the question is: Why does StandaloneNightlySetup.exe install .1? I'll investigate further.

@mherrmann mherrmann force-pushed the mherrmann-fix-silent-installer-bb-6240 branch from ebd8005 to 8ee9414 Compare April 8, 2021 15:03
@mherrmann
Copy link
Collaborator Author

Re-testing with re-compiled binaries from CI still exhibited the same problem. It is very strange: StandaloneNightlySetup.exe appears as version 90.1.25.1 in the registry and the file system (C:\Program Files\Brave Software\Brave\90.1.25.1) but shows 1.25.4 in brave://settings.help. I re-read all my code changes and have not been able to find where I could have gone wrong. My best vector of attack at this point would be brute force: For each of the 9 commits I made to brave/omaha, kick off a CI build and see which is the first to exhibit the problem.

@mherrmann
Copy link
Collaborator Author

mherrmann commented Apr 9, 2021

Actually, while StandaloneSilentNightlySetup.exe outwardly works as expected, behind the scenes it also encounters the expected version error:

[Registered version does not match expected][{C6CB981E-DB30-4876-8639-109F8933582C}][90.1.25.1][90.1.25.4]

In light of this, it appears to me that the installers (StandaloneSilentNightlySetup.exe, StandaloneNightlySetup.exe) were built from an old version (1.25.1 instead of 1.25.4) of Brave's mini_installer. I presume the file was called brave_installer_90.1.25.1.exe. But what I still don't understand is why Brave gets installed as 1.25.1 in registry and on the file system, but shows itself as 1.25.4 in brave://settings/help.

@mherrmann mherrmann force-pushed the mherrmann-fix-silent-installer-bb-6240 branch from 8ee9414 to c7a89c4 Compare April 12, 2021 06:35
@mherrmann
Copy link
Collaborator Author

Tested the latest CI build StandaloneNightlySetup.exe and it works flawlessly now. My best explanation at this point is that the previous two CI builds were somehow in an unclean state. I have no other explanation why it would work now, and why previously old .1 binaries would show up in my .4 build.

Previously, it still opened a browser window.
This installer, BraveBrowserSilent{Channel}Setup.exe was a 1 MB online
installer that fetched the latest version of Brave from the update
server. The problem was that this installer wasn't truly silent because
it opened a browser window. It was decided that the stub silent
installer is not needed because there is also a standalone (i.e.
offline) silent installer that does not open a browser window.
@mherrmann mherrmann force-pushed the mherrmann-fix-silent-installer-bb-6240 branch from c7a89c4 to 31daf66 Compare April 12, 2021 16:07
@mherrmann
Copy link
Collaborator Author

Again tested new CI builds on Win 10 and the installers StandaloneSetup.exe and StandaloneSilentSetup.exe worked flawlessly, both with and without an active connection. It was also possible to check for updates on brave://settings/help which I take as sufficient indication that the Omaha update client was installed successfully.

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEPS changes ok

@mherrmann
Copy link
Collaborator Author

I just updated the PR description if you'd like a bit more context @bridiver.

@mherrmann
Copy link
Collaborator Author

I can't find the labels QA/Yes, QA/No, release-notes/include, OS/... that are mentioned in the "Submitter Checklist".

@mherrmann mherrmann added this to the 1.25.x - Nightly milestone Apr 13, 2021
@mherrmann mherrmann added the dependencies Pull requests that update a dependency file label Apr 13, 2021
@mherrmann mherrmann merged commit 2f6f020 into master Apr 13, 2021
@mherrmann mherrmann deleted the mherrmann-fix-silent-installer-bb-6240 branch April 13, 2021 16:01
kylehickinson added a commit that referenced this pull request Jan 4, 2024
#8367)

Missing this constraint would cause the web view container view to have ambiguous height and result in a 0-height web page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

brave silent setup is not silent
3 participants