Import fails or hangs when Brave is already open

[bsclifton]

Description

Originally captured with comment in #2438 (comment)

There is a potential race-condition because the importer requires Muon Brave to be closed:

1. Muon Brave launches
2. Brave Core is installed + launched (import runs as soon as it loads)
3. Muon Brave quits

If step 2 ran the importer too quickly, it could have hit a lock. If that happens, I'm not sure what the user experience looks like. They may have been shown the "Please close Brave and try again" or maybe it fails silently

[garrettr]

The importer fails silently if it is run automatically, via --upgrade-from-muon: see #2438 (comment) for a more detailed explanation.

@bsclifton I thought the b-l updater was responsible for preventing the case where b-l is still running when b-c is launched for the first time; does that condition not end up being enforced for some reason?

[bsclifton]

@garrettr I suspect Muon Brave isn't quitting fast enough...

We could (on the Brave Core side) build a delay in... or possibly check for the Brave process and have a timeout (like 10 seconds) where we wait for it to exit

I can also look at the exit code and see if there's a way to terminate the process (even if uncleanly)

Why is the lock file needed? Just to ensure the import doesn't get written over? (ex: data integrity) Or does it actually put file locks in place during import, which it wouldn't be able to acquire if Muon was open?

edit: just noticed the comments in #2438 (comment)

[garrettr]

Why is the lock file needed?

Only one browser process should be able to access a Chromium/Muon user data directory at a time; a lot of code relies on this assumption. When b-l is running, it obtains a lock for the user data directory; likewise, while the b-c importer is running, it (temporarily, briefly) obtains a lock for the user data directory. In both cases, we want to ensure:

1. We can import from some data sources that independently manage exclusive access to themselves, e.g. LoginDatabase.
2. We avoid potential data corruption issues; e.g. what happens if b-l is running and modifies some of its on-disk state while b-c is midway through an import? It's easier to avoid this scenario than to try to handle it.

We could (on the Brave Core side) build a delay in... or possibly check for the Brave process and have a timeout (like 10 seconds) where we wait for it to exit

Is there any reason not to handle this in the updater?

[garrettr]

Distilling some internal discussion:

Background

Based on reports from users who've encountered this issue, our current hypothesis is that in some cases, Muon's profile lockfiles are not cleaned up properly when the automatic import runs during Brave Core's first run, which causes the automatic import to fail in BraveExternalProcessImporterHost::CheckForChromeOrBraveLock.

During a manual import, we check lock files to determine if the browser to import data from is currently in use; if it is, we show a dialog that prompts the user to close the other browser. The code then waits for the user to either close the other browser and continue the import, or to cancel the import.

During an automatic import, we cannot show the user such a dialog, so the importer cancels itself.

It appears there are some cases in which Muon shuts down without cleaning up its lock files. If this happens, and then:

• The user then initiates a manual import, the user will be prompted to close Muon, but they will not have any open instances of Muon, which is confusing and appears to leave them in an unrecoverable state (although in reality we never automatically delete Muon's state and it is always recoverable).
• The Muon updater runs Brave Core with --upgrade-from-muon, the import will fail silently.

Debugging

Assuming this is the root cause, it can be simulated for debugging purposes by:

1. Launching Muon
   • Note that this creates several new files, Singleton{Cookie,Lock,Socket}, in Muon's user data directory.
2. kill -9 <muon PID>
   • Note that the Singleton files remain in the user data directory, and are not automatically cleaned up as they would be if Muon had exited cleanly.
3. Launch Brave Core and try to import from Muon. You should see the "Close Brave (old)" dialog. The only way to dismiss it is to cancel the import; since there is no Muon process for the user to close, clicking "Try Again" will return you to the same stuck state, ad infinitum.

Next steps

1. Add additional logging around the profile lock check so we can have more information when debugging any future issues.
2. Add additional logic to BraveProfileLock to attempt to detect and ignore stale lockfiles. ProcessSingleton already has some relevant methods we can probably use.

[LaurenWags]

Verified passed with

Brave	0.58.15 Chromium: 71.0.3578.98 (Official Build) (64-bit)
Revision	15234034d19b85dcd9a03b164ae89d04145d8368-refs/branch-heads/3578@{#897}
OS	Mac OS X

• Verified Test Plan in Handle stale lockfiles in Muon browser profile brave-core#1105
• Verified if muon is actually open when I go to import in b-c, I do get the below prompt
  
• Verified if muon is open when attempting import, when I closed muon I was able to perform the import successfully.

Verification passed on

Brave	0.58.15 Chromium: 71.0.3578.98 (Official Build) (64-bit)
Revision	15234034d19b85dcd9a03b164ae89d04145d8368-refs/branch-heads/3578@{#897}
OS	Windows

• Unable to test PR Handle stale lockfiles in Muon browser profile brave-core#1105 as there is no STR for Win

• When muon already opened and try to import in brave core prompts the pop up message
  

• Verified if muon is open when attempting import, when I closed muon I was able to perform the import successfully.