We need to pin Chrome Frame so that it doesn't get stuck on releasing Breakpa...

We need to pin Chrome Frame so that it doesn't get stuck on releasing Breakpad.

The Broker interacts with Chrome Frame from a Worker Thread, and if this thread is the last one to release the Chrome Frame COM object, then the Chrome Frame DLL will be relased during CoUninitialize and will try to release Breakpad while the breakpad thread is stuck on the loader lock.

BUG=65116
TEST=Make sure the Broker exists cleanly and in a reasonable amount of time.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68042

[MAD, 2010-12-02 16:34:16]

Potential small fix to be integrated in M9...

BYE
MAD

[Jói, 2010-12-02 16:39:19]

LGTM, assuming it works and passes tests :)

http://codereview.chromium.org/5522002/diff/3001/ceee/ie/broker/chrome_postma...
File ceee/ie/broker/chrome_postman.cc (right):

http://codereview.chromium.org/5522002/diff/3001/ceee/ie/broker/chrome_postma...
ceee/ie/broker/chrome_postman.cc:250: static bool s_pinned = false;
Note that there would be no harm incrementing the load count multiple times
(you're already adding one "AddRef" that will never get released, so adding
multiple would have the same effect), but fine as is.

[Sigurður Ásgeirsson, 2010-12-02 16:40:52]

We really shouldn't maintain references to any COM object to static
uninitialization.
This leads to shutdown crashes, and if crash reporting is shut down at that
point (which is should be) we'll never know. Let's discuss.

On Thu, Dec 2, 2010 at 11:34 AM, <mad@chromium.org> wrote:

> Reviewers: Jói, Ruðrugis,
>
> Message:
> Potential small fix to be integrated in M9...
>
> BYE
> MAD
>
> Description:
> We need to pin Chrome Frame so that it doesn't get stuck on releasing
> Breakpad.
>
> The Broker interacts with Chrome Frame from a Worker Thread, and if this
> thread
> is the last one to release the Chrome Frame COM object, then the Chrome
> Frame
> DLL will be relased during CoUninitialize and will try to release Breakpad
> while
> the breakpad thread is stuck on the loader lock.
>
> BUG=64388
> TEST=Make sure the Broker exists cleanly and in a reasonable amount of
> time.
>
>
> Please review this at http://codereview.chromium.org/5522002/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     ceee/ie/broker/chrome_postman.cc
>
>
> Index: ceee/ie/broker/chrome_postman.cc
> ===================================================================
> --- ceee/ie/broker/chrome_postman.cc    (revision 68003)
> +++ ceee/ie/broker/chrome_postman.cc    (working copy)
> @@ -244,6 +244,19 @@
>     LOG(ERROR) << "Failed to start chrome frame " << com::LogHr(hr);
>     return hr;
>   }
> +  // We need to pin the Chrome Frame DLL so that it doesn't get unloaded
> +  // while we call CoUninitialize in our thread cleanup, otherwise we
> would
> +  // get stuck on breakpad termination as filed in http://crbug.com/64388
> .
> +  static bool s_pinned = false;
> +  if (!s_pinned) {
> +    HMODULE unused;
> +    if (!GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_PIN,
> L"npchrome_frame.dll",
> +                           &unused)) {
> +      NOTREACHED() << "Failed to pin Chrome Frame. " << com::LogWe();
> +    } else {
> +      s_pinned = true;
> +    }
> +  }
>   return hr;
>  }
>
>
>
>

[Sigurður Ásgeirsson, 2010-12-02 17:43:38]

lgtm as a short-term low-impact fix, but please file a bug against fixing
teardown for real. Also, this shouldn't be a problem in the field as we should
be using the Omaha crash service, which doesn't require breakpad to spin up the
thread in the first place.

[MAD, 2010-12-02 18:22:16]

On 2010/12/02 17:43:38, Ruðrugis wrote:
> lgtm as a short-term low-impact fix, but please file a bug against fixing
> teardown for real. Also, this shouldn't be a problem in the field as we should
> be using the Omaha crash service, which doesn't require breakpad to spin up
the
> thread in the first place.

OK, thanks, I'll commit after a successful run of the smoke tests...
Note that I changed the BUG id to a more appropriate one...