Start and stop crash reporting outside of the loader lock.

chrome_frame/chrome_tab.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // chrome_tab.cc : Implementation of DLL Exports.
 
 // Need to include this before the ATL headers below.
 #include "chrome_frame/chrome_tab.h"
 
 #include <atlsecurity.h>
 #include <objbase.h>
 
 #include "base/at_exit.h"
+#include "base/basictypes.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/file_version_info.h"
 #include "base/logging.h"
 #include "base/logging_win.h"
 #include "base/metrics/field_trial.h"
 #include "base/path_service.h"
 #include "base/string16.h"
 #include "base/string_number_conversions.h"
 #include "base/string_piece.h"
@@ skipping 17 matching lines @@
 #include "chrome_frame/exception_barrier.h"
 #include "chrome_frame/metrics_service.h"
 #include "chrome_frame/resource.h"
 #include "chrome_frame/utils.h"
 #include "googleurl/src/url_util.h"
 #include "grit/chrome_frame_resources.h"
 
 using base::win::RegKey;
 
 namespace {
-// This function has the side effect of initializing an unprotected
-// vector pointer inside GoogleUrl. If this is called during DLL loading,
-// it has the effect of avoiding an initialization race on that pointer.
-// TODO(siggi): fix GoogleUrl.
-void InitGoogleUrl() {
-  static const char kDummyUrl[] = "http://www.google.com";
-
-  url_util::IsStandard(kDummyUrl,
-                       url_parse::MakeRange(0, arraysize(kDummyUrl)));
-}
 
 const wchar_t kInternetSettings[] =
     L"Software\\Microsoft\\Windows\\CurrentVersion\\Internet Settings";
 
 const wchar_t kProtocolHandlers[] =
     L"Software\\Classes\\Protocols\\Handler";
 
 const wchar_t kRunOnce[] =
     L"Software\\Microsoft\\Windows\\CurrentVersion\\RunOnce";
 
 const wchar_t kRunKeyName[] = L"ChromeFrameHelper";
 
 const wchar_t kChromeFrameHelperExe[] = L"chrome_frame_helper.exe";
 const wchar_t kChromeFrameHelperStartupArg[] = L"--startup";
 
 // Window class and window names.
 // TODO(robertshield): These and other constants need to be refactored into
 // a common chrome_frame_constants.h|cc and built into a separate lib
 // (either chrome_frame_utils or make another one).
 const wchar_t kChromeFrameHelperWindowClassName[] =
     L"ChromeFrameHelperWindowClass";
 const wchar_t kChromeFrameHelperWindowName[] =
     L"ChromeFrameHelperWindowName";
 
 // {0562BFC3-2550-45b4-BD8E-A310583D3A6F}
-static const GUID kChromeFrameProvider =
+const GUID kChromeFrameProvider =
     { 0x562bfc3, 0x2550, 0x45b4,
         { 0xbd, 0x8e, 0xa3, 0x10, 0x58, 0x3d, 0x3a, 0x6f } };
 
 const wchar_t kPostPlatformUAKey[] =
     L"Software\\Microsoft\\Windows\\CurrentVersion\\Internet Settings\\"
     L"User Agent\\Post Platform";
 const wchar_t kChromeFramePrefix[] = L"chromeframe/";
 
 // See comments in DllGetClassObject.
 LPFNGETCLASSOBJECT g_dll_get_class_object_redir_ptr = NULL;
 
+// This function has the side effect of initializing an unprotected
+// vector pointer inside GoogleUrl. If this is called during DLL loading,
+// it has the effect of avoiding an initialization race on that pointer.
+// TODO(siggi): fix GoogleUrl.
+void InitGoogleUrl() {
+  static const char kDummyUrl[] = "http://www.google.com";
+
+  url_util::IsStandard(kDummyUrl,
+                       url_parse::MakeRange(0, arraysize(kDummyUrl)));
+}
+
 class ChromeTabModule : public CAtlDllModuleT<ChromeTabModule> {
  public:
   typedef CAtlDllModuleT<ChromeTabModule> ParentClass;
 
   ChromeTabModule() : do_system_registration_(true) {}
 
   DECLARE_LIBID(LIBID_ChromeTabLib)
   DECLARE_REGISTRY_APPID_RESOURCEID(IDR_CHROMETAB,
                                     "{FD9B1B31-F4D8-436A-8F4F-D3C2E36733D3}")
 
@@ skipping 714 matching lines @@
   if (reason == DLL_PROCESS_ATTACH) {
 #ifndef NDEBUG
     // Silence traces from the ATL registrar to reduce the log noise.
     ATL::CTrace::s_trace.ChangeCategory(atlTraceRegistrar, 0,
                                         ATLTRACESTATUS_DISABLED);
 #endif
     InitGoogleUrl();
 
     g_exit_manager = new base::AtExitManager();
     CommandLine::Init(0, NULL);
-    InitializeCrashReporting();

[robertshield, 2013/03/06 13:50:32]

Can we move any of the rest of this code out of DllMain as well? I worry that we
will now miss crashes in the redirector and field trial code below, does that
stuff need to happen in DllMain or could we move it to DllGetClassObject?

[grt (UTC plus 2), 2013/03/11 15:35:11]

I left the rest here for some reason that I can't remember at the moment. I'll
take another look and see what makes sense to move.

     logging::InitLogging(
         NULL,
         logging::LOG_ONLY_TO_SYSTEM_DEBUG_LOG,
         logging::LOCK_LOG_FILE,
         logging::DELETE_OLD_LOG_FILE,
         logging::DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS);
 
     // Log the same items as Chrome.
     logging::SetLogItems(true,  // enable_process_id
                          true,  // enable_thread_id
@@ skipping 24 matching lines @@
     g_field_trial_list = NULL;
 
     DllRedirector* dll_redirector = DllRedirector::GetInstance();
     DCHECK(dll_redirector);
     dll_redirector->UnregisterAsFirstCFModule();
 
     g_patch_helper.UnpatchIfNeeded();
 
     delete g_exit_manager;
     g_exit_manager = NULL;
-
-    ShutdownCrashReporting();
   }
   return _AtlModule.DllMain(reason, reserved);
 }
 
 // Used to determine whether the DLL can be unloaded by OLE
 STDAPI DllCanUnloadNow() {
-  return _AtlModule.DllCanUnloadNow();
+  if (!_AtlModule.DllCanUnloadNow())
+    return false;
+
+  // Stop crash reporting now, or when the last ScopedCrashReporting destructs.
+  CrashReportingManager::Decommit();
+  return true;

[robertshield, 2013/03/06 13:50:32]

Why shutdown here rather than in DllMain? Does shutting down crash reporting
under the loader lock also cause a hang?

For the case where npchrome_frame.dll is invoked by setup.exe, I would expect
that calling Decommit on process_detach would no-op based on the scoped stuff
below.

For the case where npchrome_frame is loaded by something that calls
DllGetClassObject(), we may still be better off doing this on process_detach as
I don't believe there's any guarantee that DllCanUnloadNow() won't be called
spuriously. 

[grt (UTC plus 2), 2013/03/11 15:35:11]

Yes. The hang observed happens during shutdown, in fact.
For registration and unregistration, Commit/Decommit is never called. Rather,
reporting is started when Dll{R,Unr}egister{,User}Server enters and stopped when
it leaves.
What do you mean when you say "spuriously"? I'm assuming that DllCanUnloadNow
may be called at any time and that the ATL module will only return true when
there are no outstanding objects. I suppose it's entirely possible that the
process will never call CoFreeUnusedLibraries so CanUnloadNow will never be
called. I can look for a way to automatically Decommit when the last outstanding
object goes away. Do you think that's needed? How bad is it if GCF never hits
the shutdown codepath?

 }
 
 // Returns a class factory to create an object of the requested type
 STDAPI DllGetClassObject(REFCLSID rclsid, REFIID riid, LPVOID* ppv) {
+  ScopedCrashReporting crash_reporting;
+
   // If we found another module present when we were loaded, then delegate to
   // that:
   if (g_dll_get_class_object_redir_ptr) {
     return g_dll_get_class_object_redir_ptr(rclsid, riid, ppv);
   }
 
   // Enable sniffing and switching only if asked for BHO
   // (we use BHO to get loaded in IE).
-  if (rclsid == CLSID_ChromeFrameBHO) {
+  if (rclsid == CLSID_ChromeFrameBHO)
     g_patch_helper.InitializeAndPatchProtocolsIfNeeded();
-  }
 
-  return _AtlModule.DllGetClassObject(rclsid, riid, ppv);
+  HRESULT hr = _AtlModule.DllGetClassObject(rclsid, riid, ppv);
+
+  // Keep crash reporting going if an instance is being returned. It will be
+  // stopped in DllCanUnloadNow.
+  if (SUCCEEDED(hr) && *ppv)
+    crash_reporting.Commit();
+
+  return hr;
 }
 
 // DllRegisterServer - Adds entries to the system registry
 STDAPI DllRegisterServer() {
+  ScopedCrashReporting crash_reporting;
   uint16 flags =  ACTIVEX | ACTIVEDOC | TYPELIB | GCF_PROTOCOL |
                   BHO_CLSID | BHO_REGISTRATION;
 
   HRESULT hr = CustomRegistration(flags, true, true);
   if (SUCCEEDED(hr)) {
     SetupRunOnce();
   }
 
   return hr;
 }
 
 // DllUnregisterServer - Removes entries from the system registry
 STDAPI DllUnregisterServer() {
+  ScopedCrashReporting crash_reporting;
   HRESULT hr = CustomRegistration(ALL, false, true);
   return hr;
 }
 
 // DllRegisterUserServer - Adds entries to the HKCU hive in the registry.
 STDAPI DllRegisterUserServer() {
+  ScopedCrashReporting crash_reporting;
   UINT flags =  ACTIVEX | ACTIVEDOC | TYPELIB | GCF_PROTOCOL |
                 BHO_CLSID | BHO_REGISTRATION;
 
   HRESULT hr = CustomRegistration(flags, TRUE, false);
   if (SUCCEEDED(hr)) {
     SetupRunOnce();
   }
 
   return hr;
 }
 
 // DllUnregisterUserServer - Removes entries from the HKCU hive in the registry.
 STDAPI DllUnregisterUserServer() {
+  ScopedCrashReporting crash_reporting;
   HRESULT hr = CustomRegistration(ALL, FALSE, false);
   return hr;
 }
 
 // Object entries go here instead of with each object, so that we can move
 // the objects to a lib. Also reduces magic.
 OBJECT_ENTRY_AUTO(CLSID_ChromeFrameBHO, Bho)
 OBJECT_ENTRY_AUTO(__uuidof(ChromeActiveDocument), ChromeActiveDocument)
 OBJECT_ENTRY_AUTO(__uuidof(ChromeFrame), ChromeFrameActivex)
 OBJECT_ENTRY_AUTO(__uuidof(ChromeProtocol), ChromeProtocol)