Handle automation server crashes.

Handle automation server crashes.  When Chrome crashes, we now handle the case and support document refresh or reload.
When chrome crashes, we draw a poor man's sad tab (":-("), so that can clearly be improved.
Another thing is that if the chrome instance that crashed held several navigational entries, then that history is lost.

TEST=There are a couple of tests included, so run those (*TabCrash*) and also verify that when the chrome automation server is killed that we do the right thing.  Also check info in bug report.
BUG=25839
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55565

[amit, 2010-08-03 22:25:08]

[+stoyan, for double checking proxy factory threading issues]
Looks excellent and nice tests!

http://codereview.chromium.org/3061036/diff/40003/53006
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/3061036/diff/40003/53006#newcode311
chrome_frame/chrome_frame_activex_base.h:311: if (draw_sad_tab_) {
Since the renderer window covers this area, we could always draw sad tab and
instructions to refresh. However, we can consider two conditions:
- this could produce a flash initially and in that case we could do this only if
the state is greater than initialized/connected etc.
- do nothing if the draw_info.prcBounds are an empty rect (due to other  window
covering us). In theory we shouldn't even get a paint in that case.

http://codereview.chromium.org/3061036/diff/40003/53006#newcode572
chrome_frame/chrome_frame_activex_base.h:572: if (reason ==
AUTOMATION_SERVER_CRASHED)
Do we need to make a distinction why the launch failed? A user would always want
to know when things are not OK and have a clue that she needs to refresh.

http://codereview.chromium.org/3061036/diff/40003/53006#newcode1255
chrome_frame/chrome_frame_activex_base.h:1255:
scoped_ptr<UrlmonUrlRequestManager> url_fetcher_;
It's not obvious to me why url_fetcher_ needs to be dynamically allocated?

http://codereview.chromium.org/3061036/diff/40003/53019
File chrome_frame/test/ui_test.cc (right):

http://codereview.chromium.org/3061036/diff/40003/53019#newcode248
chrome_frame/test/ui_test.cc:248: hr = browser->Navigate(bstr, &empty, &empty,
&empty, &empty);
nit: wasn't this a noop in some version of IE for the currently loaded URL?

http://codereview.chromium.org/3061036/diff/40003/53019#newcode270
chrome_frame/test/ui_test.cc:270: ConnectDocPropNotifySink(&ie_mock_,
&prop_listener),
This is nice, is it possible to do this in OnLoadInternal and always check for
the correct ready state?

[stoyan, 2010-08-03 23:00:59]

http://codereview.chromium.org/3061036/diff/40003/53007
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/3061036/diff/40003/53007#newcode237
chrome_frame/chrome_frame_automation.cc:237: DCHECK(entry->launch_delegate ==
delegate);
Multiple AutomationClients (== multiple tabs) can use same AutomationProxyImpl
(== channel), so this DCHECK is strange. (Unfortunatelly
ProxyFactoryTest.CreateSameProfile test uses same mock object as delegate and
hence will not catch the problem)
Seems that OnChannelError will notify only the first launch_delegate.

[tommi, 2010-08-04 01:06:49]

Thanks.  I left that DCHECK in there as a reminder to myself to look into
whether multiple launch delegates should be supported.
Thanks for catching that.  I'll make a change that supports more delegates
and send out an update.

On Tue, Aug 3, 2010 at 7:00 PM, <stoyan@chromium.org> wrote:

>
> http://codereview.chromium.org/3061036/diff/40003/53007
> File chrome_frame/chrome_frame_automation.cc (right):
>
> http://codereview.chromium.org/3061036/diff/40003/53007#newcode237
> chrome_frame/chrome_frame_automation.cc:237:
> DCHECK(entry->launch_delegate == delegate);
> Multiple AutomationClients (== multiple tabs) can use same
> AutomationProxyImpl (== channel), so this DCHECK is strange.
> (Unfortunatelly ProxyFactoryTest.CreateSameProfile test uses same mock
> object as delegate and hence will not catch the problem)
> Seems that OnChannelError will notify only the first launch_delegate.
>
>
> http://codereview.chromium.org/3061036/show
>

[tommi, 2010-08-04 20:29:01]

Please take another look

On Tue, Aug 3, 2010 at 9:06 PM, Tommi <tommi@chromium.org> wrote:

> Thanks.  I left that DCHECK in there as a reminder to myself to look into
> whether multiple launch delegates should be supported.
> Thanks for catching that.  I'll make a change that supports more delegates
> and send out an update.
>
>
> On Tue, Aug 3, 2010 at 7:00 PM, <stoyan@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/3061036/diff/40003/53007
>> File chrome_frame/chrome_frame_automation.cc (right):
>>
>> http://codereview.chromium.org/3061036/diff/40003/53007#newcode237
>> chrome_frame/chrome_frame_automation.cc:237:
>> DCHECK(entry->launch_delegate == delegate);
>> Multiple AutomationClients (== multiple tabs) can use same
>> AutomationProxyImpl (== channel), so this DCHECK is strange.
>> (Unfortunatelly ProxyFactoryTest.CreateSameProfile test uses same mock
>> object as delegate and hence will not catch the problem)
>> Seems that OnChannelError will notify only the first launch_delegate.
>>
>>
>> http://codereview.chromium.org/3061036/show
>>
>
>

[tommi, 2010-08-04 20:45:06]

sorry - too early, I broke some tests! grr... I'll let you know.

[tommi, 2010-08-05 14:58:16]

Please take another look.  The reference counting stuff for proxy entries is
getting out of hand so I'll have to refactor that at some later point.

[stoyan, 2010-08-05 18:03:23]

OK

http://codereview.chromium.org/3061036/diff/54005/62010
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/3061036/diff/54005/62010#newcode312
chrome_frame/chrome_frame_automation.cc:312: bool
ProxyFactory::FreeLaunchDelegate(LaunchDelegate* delegate) {
I guess the things became pretty messy. LaunchDelegate* was not intended to be
stored anywhere. And adding on top of it refcounting makes everything looks even
worse.
Seems you need to add automation_server_id as parameter here, otherwise code
like:

LaunchDelegate x;
factory->GetAutomationServer(&x, "iexplore_x", &id1);
factory->GetAutomationServer(&x, "iexplore_y", &id2);
factory->FreeLaunchDelegate(&x);

will remove all instances of x.

http://codereview.chromium.org/3061036/diff/54005/62012
File chrome_frame/chrome_frame_npapi_unittest.cc (right):

http://codereview.chromium.org/3061036/diff/54005/62012#newcode100
chrome_frame/chrome_frame_npapi_unittest.cc:100: arg->incognito() == incognito,
, => &&

Personally I find the previous version (with explicit testing::Field) more
readable.

http://codereview.chromium.org/3061036/diff/54005/62014
File chrome_frame/test/automation_client_mock.cc (right):

http://codereview.chromium.org/3061036/diff/54005/62014#newcode41
chrome_frame/test/automation_client_mock.cc:41:
LaunchParamProfileEq(profile_path_.BaseName().value()),
You can use testing::Property() instead of creating dedicated matcher.

[tommi, 2010-08-05 18:28:32]

http://codereview.chromium.org/3061036/diff/54005/62010
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/3061036/diff/54005/62010#newcode312
chrome_frame/chrome_frame_automation.cc:312: bool
ProxyFactory::FreeLaunchDelegate(LaunchDelegate* delegate) {
On 2010/08/05 18:03:23, stoyan wrote:
> I guess the things became pretty messy. LaunchDelegate* was not intended to be
> stored anywhere. And adding on top of it refcounting makes everything looks
even
> worse.
> Seems you need to add automation_server_id as parameter here, otherwise code
> like:
> 
> LaunchDelegate x;
> factory->GetAutomationServer(&x, "iexplore_x", &id1);
> factory->GetAutomationServer(&x, "iexplore_y", &id2);
> factory->FreeLaunchDelegate(&x);
> 
> will remove all instances of x.

There's a newer version of this file uploaded, please take a look

http://codereview.chromium.org/3061036/diff/54005/62012
File chrome_frame/chrome_frame_npapi_unittest.cc (right):

http://codereview.chromium.org/3061036/diff/54005/62012#newcode100
chrome_frame/chrome_frame_npapi_unittest.cc:100: arg->incognito() == incognito,
On 2010/08/05 18:03:23, stoyan wrote:
> , => &&
> 
> Personally I find the previous version (with explicit testing::Field) more
> readable.

Now the fields are protected, so that's not possible.  I'm open to suggestions
though.

[tommi, 2010-08-06 21:34:39]

latest changes uploaded.  Please take a look at the UMA code saneness and the
new AutomationProxyCacheEntry class.

[amit, 2010-08-09 22:34:53]

lgtm!

[tommi, 2010-08-09 23:44:02]

w007!  stoyan?

On Mon, Aug 9, 2010 at 6:34 PM, <amit@chromium.org> wrote:

> lgtm!
>
>
> http://codereview.chromium.org/3061036/show
>

[stoyan, 2010-08-10 00:15:52]

lgtm