Chrome side of the fix for http://b/issue?id=1694574, which is a bug caused w...

Chrome side of the fix for http://b/issue?id=1694574, which is a bug caused when a new automation client instance is launched and attempts to attach to an existing external tab. An example of where this could happen is javascript on a page attempting a window.open with target _blank. In this case the Chrome browser creates a TabContents instance which is attached to an ExternalTabContainer instance. The automation client then attaches to this ExternalTabContainer. This all works if the automation client is in the same client process. If a new process is launched a separate automation channel is created between the client and the chrome browser which causes this to not work as expected.

Fix is have a floating ExternalTabContainer instance which is eventually connected to by the client. When we receive a notification from the client that it is about to connect to the ExternalTabContainer instance we setup the automation channel and other info in the underlying automation profile.

The new TabContents is created with the same profile instance as the current TabContents. This does not
work correctly if the underlying profile is an automation profile as its lifetime is tied to the
ExternalTabContainer. To fix this I added a setter for the new policy to the NavigationController. Not doing
this causes the browser to crash if the original ExternalTabContainer instance dies.

There is a bigger issue here which is that all this profile sharing would cause session cookies to not work correctly if multiple automation clients are connected to the same Chrome browser instance over the same profile. I will file a separate bug to track this issue.

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

[amit, 2009-09-03 17:34:28]

The changes overall look pretty good, a few suggestions below

http://codereview.chromium.org/200003/diff/8001/8003
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/200003/diff/8001/8003#newcode258
Line 258: Profile* profile = new_contents->profile();
Another way to get the original profile is to call:
new_contents->profile()->GetOffTheRecordProfile()->GetOriginalProfile(); This
feels much less ugly than reinterpret_cast and checking for
load_request_via_automation_

http://codereview.chromium.org/200003/diff/8001/8008
File chrome/browser/external_tab_container.h (right):

http://codereview.chromium.org/200003/diff/8001/8008#newcode141
Line 141: static ExternalTabContainer* GetExternalTabForCookie(long cookie);
Just have one function and call it RemovePendingTab?

[ananta, 2009-09-04 05:49:09]

I made a change to call RegisterRenderView when the floating
ExternalTabContainer is Reinitialized. This ensures that the RenderViewHost
instance and the routing id get connected to the correct
AutomationResourceMsgFilter instance.

The AutomationResourceMsgFilter class when it receives a notification that the
channel is closing cleans up the map of RenderView's. This is not correct as we
could have multiple automation clients connected to a single chrome browser
instance.

http://codereview.chromium.org/200003/diff/8001/8008
File chrome/browser/external_tab_container.h (right):

http://codereview.chromium.org/200003/diff/8001/8008#newcode141
Line 141: static ExternalTabContainer* GetExternalTabForCookie(long cookie);
On 2009/09/03 17:34:28, amit wrote:
> Just have one function and call it RemovePendingTab?

Done.

[amit, 2009-09-04 13:59:29]

lgtm with a following nits.

http://codereview.chromium.org/200003/diff/2003/2009
File chrome/browser/automation/automation_profile_impl.h (right):

http://codereview.chromium.org/200003/diff/2003/2009#newcode203
Line 203: Profile* original_profile() const {
I don't think this is necessary since
AutomationProfileImpl::GetOriginalProfile() will give exactly what you want.

http://codereview.chromium.org/200003/diff/2003/2012
File chrome/browser/automation/automation_resource_message_filter.cc (right):

http://codereview.chromium.org/200003/diff/2003/2012#newcode50
Line 50: // Only erase RenderViews which are associated with this
This is a good catch!

http://codereview.chromium.org/200003/diff/2003/2005
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/200003/diff/2003/2005#newcode167
Line 167:
tab_contents_->profile()->GetOffTheRecordProfile()->GetOriginalProfile();
Just tab_contents_->profile()->GetOriginalProfile() should work.

http://codereview.chromium.org/200003/diff/2003/2005#newcode275
Line 275: off_the_record_profile->GetOriginalProfile(),
new_contents->profile()->GetOriginalProfile()

http://codereview.chromium.org/200003/diff/2003/2010
File chrome/browser/external_tab_container.h (right):

http://codereview.chromium.org/200003/diff/2003/2010#newcode38
Line 38: typedef std::map<long, ExternalTabContainer*> PendingTabs;
I think this map should have a scoped_refptr so that if there are any orphan
tabs, they get cleaned up at the end.

http://codereview.chromium.org/200003/diff/2003/2010#newcode166
Line 166: Profile* GetUnderlyingProfileFromAutomationProfile();
not needed anymore?

http://codereview.chromium.org/200003/diff/2003/2011
File chrome/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/200003/diff/2003/2011#newcode142
Line 142: void set_profile(Profile* profile) {
Just a note that the very first navigation has already began with the initial
profile and then we switch it. I think it's OK because the new automation
profile has the same underlying profile, but there could be subtle issues due to
this.

[ananta, 2009-09-04 16:27:04]

http://codereview.chromium.org/200003/diff/2003/2009
File chrome/browser/automation/automation_profile_impl.h (right):

http://codereview.chromium.org/200003/diff/2003/2009#newcode203
Line 203: Profile* original_profile() const {
On 2009/09/04 13:59:30, amit wrote:
> I don't think this is necessary since
> AutomationProfileImpl::GetOriginalProfile() will give exactly what you want.

Done.

http://codereview.chromium.org/200003/diff/2003/2005
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/200003/diff/2003/2005#newcode167
Line 167:
tab_contents_->profile()->GetOffTheRecordProfile()->GetOriginalProfile();
On 2009/09/04 13:59:30, amit wrote:
> Just tab_contents_->profile()->GetOriginalProfile() should work.

Done.

http://codereview.chromium.org/200003/diff/2003/2005#newcode275
Line 275: off_the_record_profile->GetOriginalProfile(),
On 2009/09/04 13:59:30, amit wrote:
> new_contents->profile()->GetOriginalProfile()

Done.

http://codereview.chromium.org/200003/diff/2003/2010
File chrome/browser/external_tab_container.h (right):

http://codereview.chromium.org/200003/diff/2003/2010#newcode38
Line 38: typedef std::map<long, ExternalTabContainer*> PendingTabs;
On 2009/09/04 13:59:30, amit wrote:
> I think this map should have a scoped_refptr so that if there are any orphan
> tabs, they get cleaned up at the end.

Done.

http://codereview.chromium.org/200003/diff/2003/2010#newcode166
Line 166: Profile* GetUnderlyingProfileFromAutomationProfile();
On 2009/09/04 13:59:30, amit wrote:
> not needed anymore?

Done.

[amit, 2009-09-04 16:48:28]

lgtm++

http://codereview.chromium.org/200003/diff/8015/9009
File chrome/browser/automation/automation_provider_win.cc (right):

http://codereview.chromium.org/200003/diff/8015/9009#newcode472
Line 472: if (AddExternalTab(external_tab_container)) {
feels like AddExternalTab should hold a reference and release it when it is
removed from the map. Will you pl see it makes sense?

http://codereview.chromium.org/200003/diff/8015/9006
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/200003/diff/8015/9006#newcode148
Line 148: AutomationResourceMessageFilter* filter) {
80, Also pl try to fix other lint errors in the CL

http://codereview.chromium.org/200003/diff/8015/9006#newcode167
Line 167: tab_contents_->profile()->GetOriginalProfile();
can fit above

http://codereview.chromium.org/200003/diff/8015/9006#newcode502
Line 502: Release();
Please comment why this is here.

http://codereview.chromium.org/200003/diff/8015/9006#newcode578
Line 578: ExternalTabContainer* ExternalTabContainer::RemovePendingTab(
this could return scoped_refptr<>

[ananta, 2009-09-04 17:31:42]

http://codereview.chromium.org/200003/diff/8015/9006
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/200003/diff/8015/9006#newcode167
Line 167: tab_contents_->profile()->GetOriginalProfile();
On 2009/09/04 16:48:28, amit wrote:
> can fit above

Done.

[amit, 2009-09-04 17:40:43]

lgtm++
looks much better, thanks for patiently slogging through all the iterations.

http://codereview.chromium.org/200003/diff/7047/6040
File chrome/browser/automation/automation_provider_win.cc (right):

http://codereview.chromium.org/200003/diff/7047/6040#newcode481
Line 481: external_tab_container.release();
Call Uninitialize instead?

http://codereview.chromium.org/200003/diff/7047/6037
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/200003/diff/7047/6037#newcode509
Line 509: return views::WidgetWin::OnCreate(create_struct);
we should grab a ref only if the base OnCreate is successful

http://codereview.chromium.org/200003/diff/7047/6037#newcode585
Line 585: int cookie) {
fits above?

[amit, 2009-09-04 17:49:14]

lgtm and a few more nits

http://codereview.chromium.org/200003/diff/7047/6040
File chrome/browser/automation/automation_provider_win.cc (right):

http://codereview.chromium.org/200003/diff/7047/6040#newcode344
Line 344: external_tab_container->Init(profile, settings.parent,
settings.dimensions,
A comment explaining that init will grab a self ref.

http://codereview.chromium.org/200003/diff/7047/6040#newcode481
Line 481: external_tab_container.release();
On 2009/09/04 17:40:43, amit wrote:
> Call Uninitialize instead?
I think we should just remove this call. There's one additional ref in the map
that's no longer necessary. 

Also, maybe we should Uninitialize if the AddExternalTab fails.

http://codereview.chromium.org/200003/diff/7047/6037
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/200003/diff/7047/6037#newcode296
Line 296: pending_tabs_[cookie] = new_container;
no point adding to the map if init fails

[ananta, 2009-09-04 17:54:47]

http://codereview.chromium.org/200003/diff/7047/6040
File chrome/browser/automation/automation_provider_win.cc (right):

http://codereview.chromium.org/200003/diff/7047/6040#newcode344
Line 344: external_tab_container->Init(profile, settings.parent,
settings.dimensions,
On 2009/09/04 17:49:14, amit wrote:
> A comment explaining that init will grab a self ref.

Done.

http://codereview.chromium.org/200003/diff/7047/6040#newcode481
Line 481: external_tab_container.release();
On 2009/09/04 17:40:43, amit wrote:
> Call Uninitialize instead?

Done.

http://codereview.chromium.org/200003/diff/7047/6037
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/200003/diff/7047/6037#newcode509
Line 509: return views::WidgetWin::OnCreate(create_struct);
On 2009/09/04 17:40:43, amit wrote:
> we should grab a ref only if the base OnCreate is successful

Done.

http://codereview.chromium.org/200003/diff/7047/6037#newcode585
Line 585: int cookie) {
On 2009/09/04 17:40:43, amit wrote:
> fits above?

Done.