ChromeFrame HTTP requests would randomly fail if we navigated to multiple HTT...

ChromeFrame HTTP requests would randomly fail if we navigated to multiple HTTP sites. This was because
the automation resource message filter tracked HTTP requests based on the request ids which are generated
by the renderer process. As a result a new request would get created say with id 0, while an existing request
would end in ChromeFrame causing the new request to incorrectly shutdown.

Fix is to revert back to the original way of tracking requests with an auto incrementing id. The automation url
job maintains both ids now, i.e. the automation request id and the chrome request id. The download notification
receives the automation id and basically looks up the associated automation request id and sends the notification
back to ChromeFrame.

This fixes bug http://code.google.com/p/chromium/issues/detail?id=27401

Other fixes in this CL include the following:-
1. The active document instance would never get destroyed. This was because we call ShowUI on the doc host 
   which maintains a reference. We need to call HideUI in Setsite of NULL, which releases the reference.

2. When the active x instance is shutting down we try to shutdown all running requests in the OnDestroy handler.
   To ensure that the request is deleted from the request map and released in the same thread which created it
   we post a task back to the ui thread which never runs as the window is being destroyed. Fix is to create
   a message only window with every urlmonrequest instance which supports task marshaling.

Tests in a future CL.

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

[Paweł Hajdan Jr., 2009-11-11 21:03:27]

Drive-by.

http://codereview.chromium.org/386008/diff/1/9
File chrome/browser/automation/automation_resource_message_filter.cc (right):

http://codereview.chromium.org/386008/diff/1/9#newcode223
Line 223: Send(new AutomationMsg_DownloadRequestInHost(0, tab_handle,
If Send returns bool (iirc it does), it would be better to make
SendDownloadRequestToHost return bool as well.

http://codereview.chromium.org/386008/diff/1/7
File chrome/browser/automation/automation_resource_message_filter.h (right):

http://codereview.chromium.org/386008/diff/1/7#newcode42
Line 42: int NewRequestId() {
Please add a comment (i.e. what is the scope of uniqueness of the request id).
Also, describe if it's chrome request id or automation request id (comments
below suggest that there is a difference).

http://codereview.chromium.org/386008/diff/1/7#newcode75
Line 75: // Retreives the automation request id for the passed in chrome request
nit: typo - Retrieves.

http://codereview.chromium.org/386008/diff/1/10
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/386008/diff/1/10#newcode63
Line 63: DCHECK(id_);
DCHECK_NE(id_, 0)?

http://codereview.chromium.org/386008/diff/1/10#newcode65
Line 65: NOTREACHED();
How about just DCHECK(message_filter_) at the beginning?

http://codereview.chromium.org/386008/diff/1/6
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/386008/diff/1/6#newcode407
Line 407: g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
ChromeThread::PostTask?

[tommi (sloooow) - chröme, 2009-11-11 21:12:58]

thanks for tracking this down Ananta

http://codereview.chromium.org/386008/diff/5001/5009
File chrome/browser/automation/automation_resource_message_filter.cc (right):

http://codereview.chromium.org/386008/diff/5001/5009#newcode200
Line 200: RequestMap::iterator it = request_map_.begin();
DCHECK that you're on the correct thread

http://codereview.chromium.org/386008/diff/5001/5010
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/386008/diff/5001/5010#newcode226
Line 226: message.ReadInt(&iter, request_id)) {
don't you want id_ here?  I think that's how it used to be.
if so, you need to also update the parameter name

http://codereview.chromium.org/386008/diff/5001/5002
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/386008/diff/5001/5002#newcode115
Line 115: Release();
where's the matching AddRef?

[amit, 2009-11-11 21:14:34]

LGTM! Awesome finds and fixes.

http://codereview.chromium.org/386008/diff/5001/5004
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/386008/diff/5001/5004#newcode143
Line 143: // Release the pointers we have no need for now.
nit: no need to override this method anymore

http://codereview.chromium.org/386008/diff/5001/5002
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/386008/diff/5001/5002#newcode630
Line 630: DestroyWindow();
Object could get destroyed in the context of DestroyWindow? In that case add a
comment saying we shouldn't be doing anything below that.

[ananta, 2009-11-11 21:21:00]

http://codereview.chromium.org/386008/diff/1/9
File chrome/browser/automation/automation_resource_message_filter.cc (right):

http://codereview.chromium.org/386008/diff/1/9#newcode223
Line 223: Send(new AutomationMsg_DownloadRequestInHost(0, tab_handle,
On 2009/11/11 21:03:27, Paweł Hajdan Jr. wrote:
> If Send returns bool (iirc it does), it would be better to make
> SendDownloadRequestToHost return bool as well.

Done.

http://codereview.chromium.org/386008/diff/1/7
File chrome/browser/automation/automation_resource_message_filter.h (right):

http://codereview.chromium.org/386008/diff/1/7#newcode42
Line 42: int NewRequestId() {
On 2009/11/11 21:03:27, Paweł Hajdan Jr. wrote:
> Please add a comment (i.e. what is the scope of uniqueness of the request id).
> Also, describe if it's chrome request id or automation request id (comments
> below suggest that there is a difference).

Done.

http://codereview.chromium.org/386008/diff/1/7#newcode75
Line 75: // Retreives the automation request id for the passed in chrome request
On 2009/11/11 21:03:27, Paweł Hajdan Jr. wrote:
> nit: typo - Retrieves.

Done.

http://codereview.chromium.org/386008/diff/1/10
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/386008/diff/1/10#newcode63
Line 63: DCHECK(id_);
On 2009/11/11 21:03:27, Paweł Hajdan Jr. wrote:
> DCHECK_NE(id_, 0)?

Done.

http://codereview.chromium.org/386008/diff/1/10#newcode65
Line 65: NOTREACHED();
On 2009/11/11 21:03:27, Paweł Hajdan Jr. wrote:
> How about just DCHECK(message_filter_) at the beginning?

Done.

http://codereview.chromium.org/386008/diff/1/6
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/386008/diff/1/6#newcode407
Line 407: g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
On 2009/11/11 21:03:27, Paweł Hajdan Jr. wrote:
> ChromeThread::PostTask?

Done.

http://codereview.chromium.org/386008/diff/5001/5009
File chrome/browser/automation/automation_resource_message_filter.cc (right):

http://codereview.chromium.org/386008/diff/5001/5009#newcode200
Line 200: RequestMap::iterator it = request_map_.begin();
On 2009/11/11 21:12:58, tommi wrote:
> DCHECK that you're on the correct thread

Done.

http://codereview.chromium.org/386008/diff/5001/5010
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/386008/diff/5001/5010#newcode226
Line 226: message.ReadInt(&iter, request_id)) {
On 2009/11/11 21:12:58, tommi wrote:
> don't you want id_ here?  I think that's how it used to be.
> if so, you need to also update the parameter name
I just checked the old code and it basically read the id from the message. I
think this should be fine.

http://codereview.chromium.org/386008/diff/5001/5004
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/386008/diff/5001/5004#newcode143
Line 143: // Release the pointers we have no need for now.
On 2009/11/11 21:14:34, amit wrote:
> nit: no need to override this method anymore

Done.

http://codereview.chromium.org/386008/diff/5001/5002
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/386008/diff/5001/5002#newcode115
Line 115: Release();
On 2009/11/11 21:12:58, tommi wrote:
> where's the matching AddRef?

In UrlmonUrlRequest::Start

http://codereview.chromium.org/386008/diff/5001/5002#newcode630
Line 630: DestroyWindow();
On 2009/11/11 21:14:34, amit wrote:
> Object could get destroyed in the context of DestroyWindow? In that case add a
> comment saying we shouldn't be doing anything below that.

Done.

[Paweł Hajdan Jr., 2009-11-11 21:25:49]

LGTM

[tommi (sloooow) - chröme, 2009-11-11 22:09:50]

http://codereview.chromium.org/386008/diff/5001/5002
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/386008/diff/5001/5002#newcode115
Line 115: Release();
On 2009/11/11 21:21:01, ananta wrote:
> On 2009/11/11 21:12:58, tommi wrote:
> > where's the matching AddRef?
> 
> In UrlmonUrlRequest::Start

I only see one AddRef in UrlmonUrlRequest::Start and that was there before this
change with the note "This will be released in EndRequest"

Is it possible that we were leaking before and not now?
It feels like there's a mismatch somewhere.

[ananta, 2009-11-11 22:23:06]

On 2009/11/11 22:09:50, tommi wrote:
> http://codereview.chromium.org/386008/diff/5001/5002
> File chrome_frame/urlmon_url_request.cc (right):
> 
> http://codereview.chromium.org/386008/diff/5001/5002#newcode115
> Line 115: Release();
> On 2009/11/11 21:21:01, ananta wrote:
> > On 2009/11/11 21:12:58, tommi wrote:
> > > where's the matching AddRef?
> > 
> > In UrlmonUrlRequest::Start
> 
> I only see one AddRef in UrlmonUrlRequest::Start and that was there before
this
> change with the note "This will be released in EndRequest"
> 
> Is it possible that we were leaking before and not now?
> It feels like there's a mismatch somewhere.

We were not leaking in theory before as well. Release was called in
EndRequestInternal which is the queued task. However there were instances where
this task would not run like when the activex was destroyed. This CL fixes that
by having every urlmonurlrequest instance have its own message window.

[ananta, 2009-11-11 22:23:52]

On 2009/11/11 22:23:06, ananta wrote:
> On 2009/11/11 22:09:50, tommi wrote:
> > http://codereview.chromium.org/386008/diff/5001/5002
> > File chrome_frame/urlmon_url_request.cc (right):
> > 
> > http://codereview.chromium.org/386008/diff/5001/5002#newcode115
> > Line 115: Release();
> > On 2009/11/11 21:21:01, ananta wrote:
> > > On 2009/11/11 21:12:58, tommi wrote:
> > > > where's the matching AddRef?
> > > 
> > > In UrlmonUrlRequest::Start
> > 
> > I only see one AddRef in UrlmonUrlRequest::Start and that was there before
> this
> > change with the note "This will be released in EndRequest"
> > 
> > Is it possible that we were leaking before and not now?
> > It feels like there's a mismatch somewhere.
> 
> We were not leaking in theory before as well. Release was called in
> EndRequestInternal which is the queued task. However there were instances
where
> this task would not run like when the activex was destroyed. This CL fixes
that
> by having every urlmonurlrequest instance have its own message window.

I also updated the comment in the urlmonurlrequest::start function indicating
that the additional refcount is released in OnFinalMessage.

[tommi (sloooow) - chröme, 2009-11-11 22:43:47]

cool. thanks.  lgtm

On Wed, Nov 11, 2009 at 5:23 PM, <ananta@chromium.org> wrote:

> On 2009/11/11 22:23:06, ananta wrote:
>
>> On 2009/11/11 22:09:50, tommi wrote:
>> > http://codereview.chromium.org/386008/diff/5001/5002
>> > File chrome_frame/urlmon_url_request.cc (right):
>> >
>> > http://codereview.chromium.org/386008/diff/5001/5002#newcode115
>> > Line 115: Release();
>> > On 2009/11/11 21:21:01, ananta wrote:
>> > > On 2009/11/11 21:12:58, tommi wrote:
>> > > > where's the matching AddRef?
>> > >
>> > > In UrlmonUrlRequest::Start
>> >
>> > I only see one AddRef in UrlmonUrlRequest::Start and that was there
>> before
>> this
>> > change with the note "This will be released in EndRequest"
>> >
>> > Is it possible that we were leaking before and not now?
>> > It feels like there's a mismatch somewhere.
>>
>
>  We were not leaking in theory before as well. Release was called in
>> EndRequestInternal which is the queued task. However there were instances
>>
> where
>
>> this task would not run like when the activex was destroyed. This CL fixes
>>
> that
>
>> by having every urlmonurlrequest instance have its own message window.
>>
>
> I also updated the comment in the urlmonurlrequest::start function
> indicating
> that the additional refcount is released in OnFinalMessage.
>
>
>
> http://codereview.chromium.org/386008
>