This CL fixes the following issues:-...

This CL fixes the following issues:-
1. http://code.google.com/p/chromium/issues/detail?id=27200
   This was a crash which would occur in the Stop function in the UrlmonUrlRequest object
   which would get invoked when the active document instance was being destroyed. The crash
   occured if the active document instance was reused in which case we end up reusing the
   automation client instance. The fix is to ensure that we clean up existing requests from
   the map before reusing the automation client instance.

2. http://code.google.com/p/chromium/issues/detail?id=27202
   This was a DCHECK which would occur when adding a new request to the request map, which
   indicated that an existing request with the same request id existed in the map. This would
   occur during a redirect where the request id is reused by Chrome. Fix is to remove the 
   request from the map when we handle the AutomationMsg_RequestEnd message in the UI thread
   itself. The UrlmonUrlRequest functions which attempt to remove the request from the map
   now check if the request is valid before doing this.

Bug=27200, 27202
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31681

[ananta, 2009-11-10 18:10:48]

Reviewers: amit,

Description:
This CL fixes the following issues:-
1. http://code.google.com/p/chromium/issues/detail?id=27200
    This was a crash which would occur in the Stop function in the
UrlmonUrlRequest object
    which would get invoked when the active document instance was being
destroyed. The crash
    occured if the active document instance was reused in which case we end  
up
reusing the
    automation client instance. The fix is to ensure that we clean up  
existing
requests from
    the map before reusing the automation client instance.

2. http://code.google.com/p/chromium/issues/detail?id=27202
    This was a DCHECK which would occur when adding a new request to the  
request
map, which
    indicated that an existing request with the same request id existed in  
the
map. This would
    occur during a redirect where the request id is reused by Chrome. Fix is  
to
remove the
    request from the map when we handle the AutomationMsg_RequestEnd message  
in
the UI thread
    itself. The UrlmonUrlRequest functions which attempt to remove the  
request
from the map
    now check if the request is valid before doing this.

Bug=27200,27202


Please review this at http://codereview.chromium.org/388008

SVN Base: svn://chrome-svn/chrome/trunk/src/

Affected files:
   M     chrome_frame/chrome_active_document.cc
   M     chrome_frame/chrome_frame_activex_base.h
   M     chrome_frame/chrome_frame_automation.cc
   M     chrome_frame/plugin_url_request.h
   M     chrome_frame/urlmon_url_request.cc

[ananta, 2009-11-10 18:17:12]

Reviewers: amit,

Description:
This CL fixes the following issues:-
1. http://code.google.com/p/chromium/issues/detail?id=27200
    This was a crash which would occur in the Stop function in the
UrlmonUrlRequest object
    which would get invoked when the active document instance was being
destroyed. The crash
    occured if the active document instance was reused in which case we end  
up
reusing the
    automation client instance. The fix is to ensure that we clean up  
existing
requests from
    the map before reusing the automation client instance.

2. http://code.google.com/p/chromium/issues/detail?id=27202
    This was a DCHECK which would occur when adding a new request to the  
request
map, which
    indicated that an existing request with the same request id existed in  
the
map. This would
    occur during a redirect where the request id is reused by Chrome. Fix is  
to
remove the
    request from the map when we handle the AutomationMsg_RequestEnd message  
in
the UI thread
    itself. The UrlmonUrlRequest functions which attempt to remove the  
request
from the map
    now check if the request is valid before doing this.

Bug=27200,27202


Please review this at http://codereview.chromium.org/388008

SVN Base: svn://chrome-svn/chrome/trunk/src/

Affected files:
   M     chrome_frame/chrome_active_document.cc
   M     chrome_frame/chrome_frame_activex_base.h
   M     chrome_frame/chrome_frame_automation.cc
   M     chrome_frame/plugin_url_request.h
   M     chrome_frame/urlmon_url_request.cc

[ananta, 2009-11-10 18:26:52]

Reviewers: amit,

Description:
This CL fixes the following issues:-
1. http://code.google.com/p/chromium/issues/detail?id=27200
    This was a crash which would occur in the Stop function in the
UrlmonUrlRequest object
    which would get invoked when the active document instance was being
destroyed. The crash
    occured if the active document instance was reused in which case we end  
up
reusing the
    automation client instance. The fix is to ensure that we clean up  
existing
requests from
    the map before reusing the automation client instance.

2. http://code.google.com/p/chromium/issues/detail?id=27202
    This was a DCHECK which would occur when adding a new request to the  
request
map, which
    indicated that an existing request with the same request id existed in  
the
map. This would
    occur during a redirect where the request id is reused by Chrome. Fix is  
to
remove the
    request from the map when we handle the AutomationMsg_RequestEnd message  
in
the UI thread
    itself. The UrlmonUrlRequest functions which attempt to remove the  
request
from the map
    now check if the request is valid before doing this.

Bug=27200,27202


Please review this at http://codereview.chromium.org/388008

SVN Base: svn://chrome-svn/chrome/trunk/src/

Affected files:
   M     chrome_frame/chrome_active_document.cc
   M     chrome_frame/chrome_frame_activex_base.h
   M     chrome_frame/chrome_frame_automation.cc
   M     chrome_frame/plugin_url_request.h
   M     chrome_frame/urlmon_url_request.cc

[amit, 2009-11-10 18:51:21]

lgtm

http://codereview.chromium.org/388008/diff/4001/4003
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/388008/diff/4001/4003#newcode65
Line 65: automation_client_->CleanupAsyncRequests();
This should be part of 'Reinitialize'

http://codereview.chromium.org/388008/diff/4001/4002
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/388008/diff/4001/4002#newcode1085
Line 1085: std::vector<scoped_refptr<PluginUrlRequest>> request_list;
A space between >>? Not having it might bother some compilers on other platforms
:)

http://codereview.chromium.org/388008/diff/4001/4002#newcode1094
Line 1094: request_list.push_back(request);
request_list.insert(request_list.begin(), request_map_.begin(),
request_map_.end())

http://codereview.chromium.org/388008/diff/4001/4004
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/388008/diff/4001/4004#newcode72
Line 72: request_handler()->RemoveRequest(this);
I think it's better if RemoveRequest checked the validity of the request
internally.

[amit, 2009-11-10 18:51:39]

lgtm


http://codereview.chromium.org/388008/diff/4001/4003
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/388008/diff/4001/4003#newcode65
Line 65: automation_client_->CleanupAsyncRequests();
This should be part of 'Reinitialize'

http://codereview.chromium.org/388008/diff/4001/4002
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/388008/diff/4001/4002#newcode1085
Line 1085: std::vector<scoped_refptr<PluginUrlRequest>> request_list;
A space between >>? Not having it might bother some compilers on other
platforms :)

http://codereview.chromium.org/388008/diff/4001/4002#newcode1094
Line 1094: request_list.push_back(request);
request_list.insert(request_list.begin(), request_map_.begin(),
request_map_.end())

http://codereview.chromium.org/388008/diff/4001/4004
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/388008/diff/4001/4004#newcode72
Line 72: request_handler()->RemoveRequest(this);
I think it's better if RemoveRequest checked the validity of the request
internally.

http://codereview.chromium.org/388008

[ananta, 2009-11-10 21:58:17]

http://codereview.chromium.org/388008/diff/4001/4003
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/388008/diff/4001/4003#newcode65
Line 65: automation_client_->CleanupAsyncRequests();
On 2009/11/10 18:51:21, amit wrote:
> This should be part of 'Reinitialize'

Done.

http://codereview.chromium.org/388008/diff/4001/4002
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/388008/diff/4001/4002#newcode1085
Line 1085: std::vector<scoped_refptr<PluginUrlRequest>> request_list;
On 2009/11/10 18:51:21, amit wrote:
> A space between >>? Not having it might bother some compilers on other
platforms
> :)

Done.

http://codereview.chromium.org/388008/diff/4001/4002#newcode1094
Line 1094: request_list.push_back(request);
On 2009/11/10 18:51:21, amit wrote:
> request_list.insert(request_list.begin(), request_map_.begin(),
> request_map_.end())

We would probably need to use a transform as the iterator types are
incompatible. I left the code as is.

http://codereview.chromium.org/388008/diff/4001/4004
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/388008/diff/4001/4004#newcode72
Line 72: request_handler()->RemoveRequest(this);
On 2009/11/10 18:51:21, amit wrote:
> I think it's better if RemoveRequest checked the validity of the request
> internally.

Done.

[ananta, 2009-11-10 21:58:35]

http://codereview.chromium.org/388008/diff/4001/4003
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/388008/diff/4001/4003#newcode65
Line 65: automation_client_->CleanupAsyncRequests();
On 2009/11/10 18:51:21, amit wrote:
> This should be part of 'Reinitialize'

Done.

http://codereview.chromium.org/388008/diff/4001/4002
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/388008/diff/4001/4002#newcode1085
Line 1085: std::vector<scoped_refptr<PluginUrlRequest>> request_list;
On 2009/11/10 18:51:21, amit wrote:
> A space between >>? Not having it might bother some compilers on other
platforms
> :)

Done.

http://codereview.chromium.org/388008/diff/4001/4002#newcode1094
Line 1094: request_list.push_back(request);
On 2009/11/10 18:51:21, amit wrote:
> request_list.insert(request_list.begin(), request_map_.begin(),
> request_map_.end())

We would probably need to use a transform as the iterator types are
incompatible. I left the code as is.

http://codereview.chromium.org/388008/diff/4001/4004
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/388008/diff/4001/4004#newcode72
Line 72: request_handler()->RemoveRequest(this);
On 2009/11/10 18:51:21, amit wrote:
> I think it's better if RemoveRequest checked the validity of the
request
> internally.

Done.

http://codereview.chromium.org/388008