Fix for 165634

There are no reproduction steps for 165634 but I think I know what's the cause
of the crashes. Here are the steps:

1. Go to web store
2. Click any extension to install
3. Close tab with web store before extension download is started

If tab is closed fast enough there will be a crash. It is very hard to be so
quick so I added delay in posting task from ui thread to file thread
(in WebstoreInstaller::DownloadCrx). Then it's very easy to get the crash.

This change fixes the problem as right now WebStoreInstaller will not try
to install extension when web contents was destroyed. I also removed
null pointer checks in WebstoreInstaller::StartDownload as I assume they
were added as an attempt of fixing the crash and they should not be needed
any longer.

BUG=165634

[ckulakowski, 2014-02-11 16:52:27]

@tmdiep1: I wrote to you yesterday about the crashes in chromium related to
extension installation from the web store. I've investigated the problem and I
think I have a fix.

[tmdiep, 2014-02-11 23:27:34]

https://codereview.chromium.org/145873003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/145873003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:575:
GetAssociatedWebContents(),
Thanks for investigating this issue and submitting the patch! I submitted
essentially the same patch yesterday and it has just been lgtm'ed by an owner:
https://codereview.chromium.org/138803012

Before I commit, just wanted to check with you - where our patches differ is
here. Is calling GetAssociatedWebContents() essential to the fix? Or can
dispatcher()->delegate()->GetAssociatedWebContents() be used?

[ckulakowski, 2014-02-12 08:31:31]

On 2014/02/11 23:27:34, tmdiep wrote:
>
https://codereview.chromium.org/145873003/diff/1/chrome/browser/extensions/ap...
> File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
> (right):
> 
>
https://codereview.chromium.org/145873003/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:575:
> GetAssociatedWebContents(),
> Thanks for investigating this issue and submitting the patch! I submitted
> essentially the same patch yesterday and it has just been lgtm'ed by an owner:
> https://codereview.chromium.org/138803012
> 
> Before I commit, just wanted to check with you - where our patches differ is
> here. Is calling GetAssociatedWebContents() essential to the fix? Or can
> dispatcher()->delegate()->GetAssociatedWebContents() be used?

No, it's not essential. I don't know why I did it this way ;).

I just wonder: if somebody will change the page (rather than closing tab)
web_contents will not be destroyed but render view host will probably change.
Won't it brake anything? We use render_process_host_id and
render_view_host_routing_id to set download url parameters and they won't be
correct.
I don't know if it can break anything (I just checked that it won't crash and
download will finish successfully). Maybe instead of download manager
you can use url fetcher which do not use web_contents?

[tmdiep, 2014-02-12 09:27:49]

On 2014/02/12 08:31:31, ckulakowski wrote:
> On 2014/02/11 23:27:34, tmdiep wrote:
> > Before I commit, just wanted to check with you - where our patches differ is
> > here. Is calling GetAssociatedWebContents() essential to the fix? Or can
> > dispatcher()->delegate()->GetAssociatedWebContents() be used?
> 
> No, it's not essential. I don't know why I did it this way ;).

Ok great, I will commit the patch. The fact that we arrived at the same fix
gives me confidence that we have finally solved this crash. Thanks for your help
:)

> I just wonder: if somebody will change the page (rather than closing tab)
> web_contents will not be destroyed but render view host will probably change.
> Won't it brake anything? We use render_process_host_id and
> render_view_host_routing_id to set download url parameters and they won't be
> correct.
> I don't know if it can break anything (I just checked that it won't crash and
> download will finish successfully). Maybe instead of download manager
> you can use url fetcher which do not use web_contents?

Good question. I don't know this code well enough to give you an accurate
answer. My guess would be that using the download manager will expose the
download in the UI and gives the user the ability to cancel a large CRX file.
You can ask asargent@, the reviewer of https://codereview.chromium.org/138803012
if you have concerns about the host IDs.

[asargent_no_longer_on_chrome, 2014-02-12 22:56:00]

I think the reason we use the download manager instead of the url fetcher
here is because we want the download progress UI "shelf".



On Wed, Feb 12, 2014 at 1:27 AM, <tmdiep@chromium.org> wrote:

> On 2014/02/12 08:31:31, ckulakowski wrote:
>
>> On 2014/02/11 23:27:34, tmdiep wrote:
>> > Before I commit, just wanted to check with you - where our patches
>> differ is
>> > here. Is calling GetAssociatedWebContents() essential to the fix? Or can
>> > dispatcher()->delegate()->GetAssociatedWebContents() be used?
>>
>
>  No, it's not essential. I don't know why I did it this way ;).
>>
>
> Ok great, I will commit the patch. The fact that we arrived at the same fix
> gives me confidence that we have finally solved this crash. Thanks for
> your help
> :)
>
>
>  I just wonder: if somebody will change the page (rather than closing tab)
>> web_contents will not be destroyed but render view host will probably
>> change.
>> Won't it brake anything? We use render_process_host_id and
>> render_view_host_routing_id to set download url parameters and they won't
>> be
>> correct.
>> I don't know if it can break anything (I just checked that it won't crash
>> and
>> download will finish successfully). Maybe instead of download manager
>> you can use url fetcher which do not use web_contents?
>>
>
> Good question. I don't know this code well enough to give you an accurate
> answer. My guess would be that using the download manager will expose the
> download in the UI and gives the user the ability to cancel a large CRX
> file.
> You can ask asargent@, the reviewer of https://codereview.chromium.
> org/138803012
> if you have concerns about the host IDs.
>
> https://codereview.chromium.org/145873003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.