Hmm. I think that the main ordering requirement here is that |arc_service_launcher_| (which is initialized in ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun) needs to be available when NoteTakingHelper::Initialize is called.

Do you know if we can either call NoteTakingHelper::Initialize later (so it will run in the relaunched browser as well) or call NoteTakingHelper::Shutdown later (so it won't run when the original browser is stopping)?

The 2nd browser instance exits in PreMainMessageLoopRunImpl after it sends the command line to the existing browser [1] and skips PreProfileInit call. This is the desktop chrome feature where you run chrome again with a url, the exiting running chrome opens the url and new chrome process just exit. It's not a use case on chromeos. But the tests are testing the scenario. 

We are not initializing NoteTakingHelper for the 2nd browser, which is good. The problem is the 2nd browser still runs PostMainMessageLoopRun and calls NoteTakingHelper::Shutdown, which is not needed.

Is the DCHECK needed?

[1]: https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?rcl=8f0264a9eb0620b67b746a44b5856eea61245f77&l=1547

It would be easy enough to remove the DCHECK in Shutdown, but I'm a bit uneasy with cases where we're not calling init and deinit methods in pairs. I take it there's no "profile has been deinitialized" spot that the Shutdown call could be moved to though, right?

Yep, profile destruction happens pretty late (think it is in PostDestroyThreads). So could not use that.

And actually, I looked around in ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun. There are a few more code using the Shutdown patter and they are failing on the 2nd instance too:

media::SoundsManager::Shutdown()
WallpaperManager::Shutdown()
AccessibilityManager::Shutdown()
...

They even uses CHECK.

I guess because 2nd browser crash is not counted as test failure and no one noticed them.

Sorry, I didn't forget about this. :-) Do you think it's worthwhile to update NoteTakingHelper? Presumably it won't accomplish much unless all of the other singletons are updated as well, right?

Is there any way we can add the missing PreProfileInit() call, or will that require lots of other changes?

Otherwise, if this is testing a use case that doesn't exist on Chrome OS, how about just disabling the test for OS_CHROMEOS?

I don't think the problem is with NoteTakingHelper. It is just unfortunate to be the current first one in PostMainMessageLoopRun. The problem is PreProfileInit and PostMainMessageLoopRun are not strictly symmetric in the 2nd browser launch case. Maybe set a flag in ChromeBrowserMainPartsChromeos::PreProfileInit and use that to create a PreProfileShutdown (symmetric to PreProfileInit).

Prefer to keep the test case. It is not a valid use case now but it might be a one later, e.g. when browser process could shutdown on chromeos after mash.

I am hitting this crash trace when launching chrome on dev workstation:

warx@warx0:~/chromium/src$ runc
Created new window in existing browser session.
[182870:182870:0124/102119.797170:FATAL:note_taking_helper.cc(152)] Check failed: g_helper.
#0 0x7fd8208f332d base::debug::StackTrace::StackTrace()
#1 0x7fd8208f18ac base::debug::StackTrace::StackTrace()
#2 0x7fd820975a0d logging::LogMessage::~LogMessage()
#3 0x556365d5d451 chromeos::NoteTakingHelper::Shutdown()
#4 0x55636593293f chromeos::ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun()
#5 0x7fd819c4b61d content::BrowserMainLoop::ShutdownThreadsAndCleanUp()
#6 0x7fd819c54fea content::BrowserMainRunnerImpl::Shutdown()
#7 0x7fd819c3e3b7 content::BrowserMain()
#8 0x7fd81bb18fcd content::RunNamedProcessTypeMain()
#9 0x7fd81bb1bb05 content::ContentMainRunnerImpl::Run()
#10 0x7fd81bb130ad content::ContentServiceManagerMainDelegate::RunEmbedderProcess()
#11 0x7fd82115242c service_manager::Main()
#12 0x7fd81bb17efb content::ContentMain()
#13 0x556363c10506 ChromeMain
#14 0x556363c10272 main
#15 0x7fd7ffc392b1 __libc_start_main
#16 0x556363c1014a _start

Received signal 6
#0 0x7fd8208f332d base::debug::StackTrace::StackTrace()
#1 0x7fd8208f18ac base::debug::StackTrace::StackTrace()
#2 0x7fd8208f2d27 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7fd820ed50c0 <unknown>
#4 0x7fd7ffc4bfcf gsignal
#5 0x7fd7ffc4d3fa abort
#6 0x7fd8208eea46 base::debug::(anonymous namespace)::DebugBreak()
#7 0x7fd8208eea28 base::debug::BreakDebugger()
#8 0x7fd8209766e1 logging::LogMessage::~LogMessage()
#9 0x556365d5d451 chromeos::NoteTakingHelper::Shutdown()
#10 0x55636593293f chromeos::ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun()
#11 0x7fd819c4b61d content::BrowserMainLoop::ShutdownThreadsAndCleanUp()
#12 0x7fd819c54fea content::BrowserMainRunnerImpl::Shutdown()
#13 0x7fd819c3e3b7 content::BrowserMain()
#14 0x7fd81bb18fcd content::RunNamedProcessTypeMain()
#15 0x7fd81bb1bb05 content::ContentMainRunnerImpl::Run()
#16 0x7fd81bb130ad content::ContentServiceManagerMainDelegate::RunEmbedderProcess()
#17 0x7fd82115242c service_manager::Main()
#18 0x7fd81bb17efb content::ContentMain()
#19 0x556363c10506 ChromeMain
#20 0x556363c10272 main
#21 0x7fd7ffc392b1 __libc_start_main
#22 0x556363c1014a _start
  r8: 0000000000000000  r9: 00007fff47a4fa50 r10: 0000000000000008 r11: 0000000000000246
 r12: 0000556363c10120 r13: 00007fff47a53260 r14: 0000000000000000 r15: 0000000000000000
  di: 0000000000000002  si: 00007fff47a4fa50  bp: 00007fff47a4fc90  bx: 0000000000000006
  dx: 0000000000000000  ax: 0000000000000000  cx: 00007fd7ffc4bfcf  sp: 00007fff47a4fac8
  ip: 00007fd7ffc4bfcf efl: 0000000000000246 cgf: 002b000000000033 erf: 0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]
Calling _exit(1). Core file will not be generated.

Switching to a new --user-data-dir fixed the problem.

#7: I suspect that the problem wasn't reusing an existing user data dir, but rather connecting to an existing Chrome process (per the "Created new window in existing browser session." message), right?

It does sound like the same root problem of asymmetric initialization and shutdown discussed above, though (although I'm surprised we're hitting it when reusing an existing process -- I was hopeful this was just our tests doing something weird).