Destroy InputHandlerManager after compositor thread

Destroy InputHandlerManager after compositor thread

InputHandlerManager uses Unretained(this) to post tasks to compositor
thread which means it needs to outlive the compositor thread.

Added a test that is flaky without this fix.

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

[boliu, 2013-04-04 18:54:28]

Hey James, could you take a look at this fix for windows crash.

[jamesr, 2013-04-04 19:12:13]

lgtm. Good catch!

[boliu, 2013-04-04 19:13:58]

On 2013/04/04 19:12:13, jamesr wrote:
> lgtm. Good catch!

Not by me. Is top crasher in m27 :(

[commit-bot: I haz the power, 2013-04-04 19:14:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/13650004/1

[commit-bot: I haz the power, 2013-04-04 19:16:26]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[MAD, 2013-04-04 19:41:58]

On 2013/04/04 19:16:26, I haz the power (commit-bot) wrote:
> Step "update" is always a major failure.
> Look at the try server FAQ for more details.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

I don't understand how this solves it... The handler is explicitly reset at a
specific point in the destructor... What am I missing?

[boliu, 2013-04-04 19:51:58]

On 2013/04/04 19:41:58, MAD wrote:
> I don't understand how this solves it... The handler is explicitly reset at a
> specific point in the destructor... What am I missing?

You are not missing anything. It was a bad fix due to lack of repro and testing.

Added explicit compositor_thread_.reset in PS2 (maybe better to remove explicit
input_handler_manager_.reset? I'm not sure..)

James: PTAL

[jamesr, 2013-04-04 19:57:59]

Shutting this down before shutting down WebKit seems like a bad idea.  I'm now a
little less confident about checking something in without testing it.

Revert the header change - that isn't needed any more.

[boliu, 2013-04-04 20:11:19]

On 2013/04/04 19:57:59, jamesr wrote:
> Shutting this down before shutting down WebKit seems like a bad idea.  I'm now
a
> little less confident about checking something in without testing it.
> 
> Revert the header change - that isn't needed any more.

Reverted header change.

The original code before I broke this was:

  if (compositor_thread_.get()) {
    RemoveFilter(compositor_thread_->GetMessageFilter());
    compositor_thread_.reset();
  }

  if (webkit_platform_support_.get())
    WebKit::shutdown();

And now this should match the original code. I can't repro the crash on linux.
So.. should I land this?

[jamesr, 2013-04-04 20:12:29]

On 2013/04/04 20:11:19, boliu wrote:
> On 2013/04/04 19:57:59, jamesr wrote:
> > Shutting this down before shutting down WebKit seems like a bad idea.  I'm
now
> a
> > little less confident about checking something in without testing it.
> > 
> > Revert the header change - that isn't needed any more.
> 
> Reverted header change.
> 
> The original code before I broke this was:
> 
>   if (compositor_thread_.get()) {
>     RemoveFilter(compositor_thread_->GetMessageFilter());
>     compositor_thread_.reset();
>   }
> 
>   if (webkit_platform_support_.get())
>     WebKit::shutdown();
> 
> And now this should match the original code. I can't repro the crash on linux.
> So.. should I land this?

Are you trying with the flags "--force-compositing-mode
--enable-threaded-compositing"?  Are you sure you're hitting the clean renderer
shutdown path?

[MAD, 2013-04-04 20:18:19]

Maybe you should write a test for this?
One that fails like the crash and doesn't with the fix...

[boliu, 2013-04-04 21:21:33]

On 2013/04/04 20:12:29, jamesr wrote:
> Are you trying with the flags "--force-compositing-mode
> --enable-threaded-compositing"?

Yes

> Are you sure you're hitting the clean renderer
> shutdown path?

Nope. Double checked and I can't get RenderThreadImpl destructor to run. Seems
the whole renderer process just gets killed by the OS on linux? Is there a way
to force clean shutdown?

On 2013/04/04 20:18:19, MAD wrote:
> Maybe you should write a test for this?
> One that fails like the crash and doesn't with the fix...

Worthwhile in long term but I don't think for this crasher fix. (Also I don't
really want to volunteer myself since I'm not really familiar with renderer
init/shutdown, or windows.)

[boliu, 2013-04-06 19:47:37]

My best effort at adding a test to catch this case. The test is only flaky
without the fix (tested on linux); I can't make it fail consistently without
adding hooks into production code.

PTAL

[jamesr, 2013-04-06 19:50:19]

lgtm

[boliu, 2013-04-07 03:08:02]

+avi for content_tests.gypi stamp

[Avi (use Gerrit), 2013-04-07 03:19:43]

lgtm

stampity stamp

[commit-bot: I haz the power, 2013-04-07 03:35:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/13650004/18001

[commit-bot: I haz the power, 2013-04-07 03:59:16]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 17:39:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/13650004/18001

[commit-bot: I haz the power, 2013-04-08 19:09:03]

Change committed as 192857