Don't call WebContents::DownloadImage() callback if the WebContents were deleted

Don't call WebContents::DownloadImage() callback if the WebContents were deleted

BUG=583718
Committed: https://crrev.com/75ca8ffd7bd7c58ace1144df05e1307d8d707662
Cr-Commit-Position: refs/heads/master@{#375700}

[pkotwicz, 2016-02-11 04:34:32]

pkotwicz@chromium.org changed reviewers:
+ rockot@chromium.org

[pkotwicz, 2016-02-11 04:34:32]

rockot@ can you please take a look?

I cannot reproduce the use-after-free locally. However, this is the only
explanation I can think of for the clusterfuzz stack trace.

The liftetime of ContentFaviconDriver is tied to the lifetime of the
WebContents. (ContentFaviconDriver inherits from content::WebContentsUserData)

I think that the behavior without this CL is counter intuitive. The
WebContents::DownloadImage() callback should not be called if the WebContents
was destroyed.

[Ken Rockot(use gerrit already), 2016-02-12 19:42:25]

lgtm

[pkotwicz, 2016-02-12 19:45:34]

pkotwicz@chromium.org changed reviewers:
+ jam@chromium.org

[pkotwicz, 2016-02-12 19:45:34]

jam@ can you please take a look?

[jam, 2016-02-12 23:31:42]

lgtm with comments

https://codereview.chromium.org/1685343004/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1685343004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4620: DCHECK(result);
this is unnecessary. in release builds, it does nothing. in debug builds, the
crash a few lines down is just as helpful of a signal

https://codereview.chromium.org/1685343004/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1685343004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.h:1316:
base::WeakPtrFactory<WebContentsImpl> download_image_weak_factory_;
since this is never reset, just make this be weak_factory_ so that if other code
needs it they can use it.

[pkotwicz, 2016-02-16 21:46:00]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-02-16 21:46:01]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1685343004/#ps20001
(title: " ")

[commit-bot: I haz the power, 2016-02-16 21:49:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685343004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685343004/20001

[commit-bot: I haz the power, 2016-02-16 23:10:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-16 23:12:04]

Description was changed from

==========
Don't call WebContents::DownloadImage() callback if the WebContents were deleted

BUG=583718
==========

to

==========
Don't call WebContents::DownloadImage() callback if the WebContents were deleted

BUG=583718

Committed: https://crrev.com/75ca8ffd7bd7c58ace1144df05e1307d8d707662
Cr-Commit-Position: refs/heads/master@{#375700}
==========

[commit-bot: I haz the power, 2016-02-16 23:12:05]

Patchset 2 (id:??) landed as
https://crrev.com/75ca8ffd7bd7c58ace1144df05e1307d8d707662
Cr-Commit-Position: refs/heads/master@{#375700}