Start and stop crash reporting outside of the loader lock.

chrome_frame/chrome_tab.cc

Index: chrome_frame/chrome_tab.cc
diff --git a/chrome_frame/chrome_tab.cc b/chrome_frame/chrome_tab.cc
index 8b25a054daa7169bcf04aff5e838e1e385dde406..670531e556f423b0d95cbf21bb7ad5388b5b1f8b 100644
--- a/chrome_frame/chrome_tab.cc
+++ b/chrome_frame/chrome_tab.cc
@@ -11,6 +11,7 @@
 #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"
@@ -48,16 +49,6 @@
 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";
@@ -83,7 +74,7 @@ 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 } };
 
@@ -95,6 +86,17 @@ 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;
@@ -829,7 +831,6 @@ extern "C" BOOL WINAPI DllMain(HINSTANCE instance,
 
     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,
@@ -874,19 +875,24 @@ extern "C" BOOL WINAPI DllMain(HINSTANCE instance,
 
     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) {
@@ -895,15 +901,22 @@ STDAPI DllGetClassObject(REFCLSID rclsid, REFIID riid, LPVOID* 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;
 
@@ -917,12 +930,14 @@ STDAPI DllRegisterServer() {
 
 // 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;
 
@@ -936,6 +951,7 @@ STDAPI DllRegisterUserServer() {
 
 // DllUnregisterUserServer - Removes entries from the HKCU hive in the registry.
 STDAPI DllUnregisterUserServer() {
+  ScopedCrashReporting crash_reporting;
   HRESULT hr = CustomRegistration(ALL, FALSE, false);
   return hr;
 }