Start and stop crash reporting outside of the loader lock.

Start and stop crash reporting outside of the loader lock.

Instead of using DllMain to start/stop crash reporting, it is now done by way of a specialization of a new ScopedInitializationManager template. Instances of this specialization are created on the stack in entrypoints to the DLL (for registration or to get a COM object). The lifetime of crash reporting is ordinarily bound to the lifetime of the ATL module. The exception to this is when the module is pinned, at which point crash reporting is also pinned.

This change removes the breakpad_handler_dll target (by reverting http://crrev.com/70898) since it is no longer needed.

BUG=163455
TEST=install chrome frame and notice that installation doesn't block for 1 minute while npchrome_frame.dll is registered.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188207

[grt, 2013-03-06 13:40:59]

I thought a bit about the asymmetry with the Decommit method, and think its
cleaner for the to be accessed via the scoper. I will move the manager
class as a private implementation detail of the scoper and have
DllCanUnloadNow make a scoper on the stack and call its Decommit method.
With that in mind, feel free to either hold off on you review, or do it
knowing that I plan to make this change. Thanks.

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

Thanks for doing this, generally awesome though with a couple of concerns below.

https://codereview.chromium.org/12521002/diff/1004/chrome_frame/chrome_frame_...
File chrome_frame/chrome_frame_reporting.h (right):

https://codereview.chromium.org/12521002/diff/1004/chrome_frame/chrome_frame_...
chrome_frame/chrome_frame_reporting.h:34: static void Decommit();
nit: it seems slightly odd to have the public Commit() method on the scoped
objects and the Decommit() on the manager. Why not make them both static
methods?

https://codereview.chromium.org/12521002/diff/1004/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (left):

https://codereview.chromium.org/12521002/diff/1004/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:832: InitializeCrashReporting();
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?

https://codereview.chromium.org/12521002/diff/1004/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

https://codereview.chromium.org/12521002/diff/1004/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:889: return true;
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.

[robertshield, 2013-03-06 13:51:57]

Our comments passed in the ether. Feel free to ignore my comment relating
to this then :-)

On Wed, Mar 6, 2013 at 8:40 AM, Greg Thompson <grt@google.com> wrote:

> I thought a bit about the asymmetry with the Decommit method, and think
> its cleaner for the to be accessed via the scoper. I will move the manager
> class as a private implementation detail of the scoper and have
> DllCanUnloadNow make a scoper on the stack and call its Decommit method.
> With that in mind, feel free to either hold off on you review, or do it
> knowing that I plan to make this change. Thanks.
>

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

ptal. i plan to add tests in a followup patchset before trying to commit.

https://codereview.chromium.org/12521002/diff/1004/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (left):

https://codereview.chromium.org/12521002/diff/1004/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:832: InitializeCrashReporting();
On 2013/03/06 13:50:32, robertshield wrote:
> 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?

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.

https://codereview.chromium.org/12521002/diff/1004/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

https://codereview.chromium.org/12521002/diff/1004/chrome_frame/chrome_tab.cc...
chrome_frame/chrome_tab.cc:889: return true;
On 2013/03/06 13:50:32, robertshield wrote:
> Why shutdown here rather than in DllMain? Does shutting down crash reporting
> under the loader lock also cause a hang?

Yes. The hang observed happens during shutdown, in fact.

> 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 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.

> 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.

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?

[grt (UTC plus 2), 2013-03-11 19:42:49]

Okay, new approach before I resolve the linking issue: reporting is now shut
down when the last object handed out by DllGetClassObject is destroyed rather
than when DllCanUnloadNow returns true since the latter may never happen.

[robertshield, 2013-03-12 03:16:49]

lg, coupla nits

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/chrome_tab.c...
chrome_frame/chrome_tab.cc:195: {
nit: should the { not be on the previous line?

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/scoped_initi...
File chrome_frame/scoped_initialization_manager.h (right):

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/scoped_initi...
chrome_frame/scoped_initialization_manager.h:22: // (i.e., Shutdown will be
invoked once all instances are destroyed).
Consider mentioning that multiple Commit() calls are idempotent.

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/scoped_initi...
chrome_frame/scoped_initialization_manager.h:49: // reference is released if
Commit has not been invoked.
Nit: You could write this without the inner class, moving the implementation
directly into ScopedInitializationManager, having |ref_count| and |committed| be
static members. The code would be a bit shorter, but there might be something
lost that I am missing. What say you?

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/scoped_initi...
chrome_frame/scoped_initialization_manager.h:61: if (++ref_count_ == 1 &&
!committed_)
nit: I mildly prefer to have the increment operation on a separate line as I
feel it is more readable. If you violently disagree, then feel free to ignore
me.

[grt (UTC plus 2), 2013-03-12 14:45:51]

thanks. i'm not sure this is worth it given that the underlying issue is a
dependency/linkage thing. although, if this solution here seems good to you, we
can get rid of the breakpad_handler_dll target altogether.

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/chrome_tab.c...
chrome_frame/chrome_tab.cc:195: {
On 2013/03/12 03:16:49, robertshield wrote:
> nit: should the { not be on the previous line?

Done.

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/scoped_initi...
File chrome_frame/scoped_initialization_manager.h (right):

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/scoped_initi...
chrome_frame/scoped_initialization_manager.h:22: // (i.e., Shutdown will be
invoked once all instances are destroyed).
On 2013/03/12 03:16:49, robertshield wrote:
> Consider mentioning that multiple Commit() calls are idempotent.

Done.

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/scoped_initi...
chrome_frame/scoped_initialization_manager.h:49: // reference is released if
Commit has not been invoked.
On 2013/03/12 03:16:49, robertshield wrote:
> Nit: You could write this without the inner class, moving the implementation
> directly into ScopedInitializationManager, having |ref_count| and |committed|
be
> static members. The code would be a bit shorter, but there might be something
> lost that I am missing. What say you?

Hmm. The base::Lock would need to be lazy-initialized, so I don't see that
there's a low-hanging fruit gain. I find the separation in the current code very
easy to understand and reason about. Do you find it overly complicated?

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/scoped_initi...
chrome_frame/scoped_initialization_manager.h:61: if (++ref_count_ == 1 &&
!committed_)
On 2013/03/12 03:16:49, robertshield wrote:
> nit: I mildly prefer to have the increment operation on a separate line as I
> feel it is more readable. If you violently disagree, then feel free to ignore
> me.

I disagree just enough to want to leave it this way. :-)

[robertshield, 2013-03-12 16:06:12]

Code LGTM.

Also, I believe this is more correct than what CF does now re. doing stuff under
the loader lock. Let's chat about whether it's a good idea to commit this given
other considerations.

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/scoped_initi...
File chrome_frame/scoped_initialization_manager.h (right):

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/scoped_initi...
chrome_frame/scoped_initialization_manager.h:49: // reference is released if
Commit has not been invoked.
On 2013/03/12 14:45:51, grt wrote:
> On 2013/03/12 03:16:49, robertshield wrote:
> > Nit: You could write this without the inner class, moving the implementation
> > directly into ScopedInitializationManager, having |ref_count| and
|committed|
> be
> > static members. The code would be a bit shorter, but there might be
something
> > lost that I am missing. What say you?
> 
> Hmm. The base::Lock would need to be lazy-initialized, so I don't see that
> there's a low-hanging fruit gain. I find the separation in the current code
very
> easy to understand and reason about. Do you find it overly complicated?

Not overly complicated, just felt a little longer than it had to be, but it's
just a nit. I do not object to this implementation.

https://codereview.chromium.org/12521002/diff/25001/chrome_frame/scoped_initi...
chrome_frame/scoped_initialization_manager.h:61: if (++ref_count_ == 1 &&
!committed_)
On 2013/03/12 14:45:51, grt wrote:
> On 2013/03/12 03:16:49, robertshield wrote:
> > nit: I mildly prefer to have the increment operation on a separate line as I
> > feel it is more readable. If you violently disagree, then feel free to
ignore
> > me.
> 
> I disagree just enough to want to leave it this way. :-)

I suspected you might :-)

[grt (UTC plus 2), 2013-03-13 15:00:18]

PTAL. I got rid of the breakpad_handler_dll, and now pinning the module also
pins crash reporting. Things left to do:

- see if other initialization tasks in DllMain could/should be moved out.
- fix whatever you find broken with this approach.
- unit test for the initialization manager?

[robertshield, 2013-03-13 17:22:27]

lgtm

https://codereview.chromium.org/12521002/diff/38001/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

https://codereview.chromium.org/12521002/diff/38001/chrome_frame/chrome_tab.c...
chrome_frame/chrome_tab.cc:227: chrome_frame::ScopedCrashReporting*
crash_reporting_;
Could make this a scoped_ptr?

https://codereview.chromium.org/12521002/diff/38001/chrome_frame/utils.h
File chrome_frame/utils.h (right):

https://codereview.chromium.org/12521002/diff/38001/chrome_frame/utils.h#newc...
chrome_frame/utils.h:551: typedef void (*PinModuleCallbackFn)(void);
i-feel-bad-for-asking-nit: it would be nice if this stuff lived in pin_module.h
instead of utils.h.

[grt (UTC plus 2), 2013-03-14 14:40:32]

PTAL. This is now complete. I've added a unit test in the latest patchset. I
haven't been able to convince myself that the other things in DllMain should be
moved out. Please let me know if you reason otherwise.

Mark: Please review the change to breakpad/breakpad_handler.gypi, which is a
pure revert of http://crrev.com/70898. This CL under review here makes that
previous change unneeded. Thanks.

https://codereview.chromium.org/12521002/diff/38001/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

https://codereview.chromium.org/12521002/diff/38001/chrome_frame/chrome_tab.c...
chrome_frame/chrome_tab.cc:227: chrome_frame::ScopedCrashReporting*
crash_reporting_;
On 2013/03/13 17:22:27, robertshield wrote:
> Could make this a scoped_ptr?

No. I explicitly don't want it to be deleted when this instance is destroyed (at
exit or unload, probably under the loader lock). Comment added.

https://codereview.chromium.org/12521002/diff/38001/chrome_frame/utils.h
File chrome_frame/utils.h (right):

https://codereview.chromium.org/12521002/diff/38001/chrome_frame/utils.h#newc...
chrome_frame/utils.h:551: typedef void (*PinModuleCallbackFn)(void);
On 2013/03/13 17:22:27, robertshield wrote:
> i-feel-bad-for-asking-nit: it would be nice if this stuff lived in
pin_module.h
> instead of utils.h.

Done.

[Mark Mentovai, 2013-03-14 14:43:48]

LGTM in breakpad

[robertshield, 2013-03-14 15:35:13]

LGTM the third

[commit-bot: I haz the power, 2013-03-14 15:40:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/12521002/63001

[commit-bot: I haz the power, 2013-03-14 21:52:39]

Change committed as 188207