Forbid ImageDownloaderImpl from accessing RenderThread instance after message loop shutdown.

Forbid ImageDownloaderImpl from accessing RenderThread instance after message loop shutdown.

This CL fixes the crash caused by following possible scenario:
- the message loop is shutdown.
- during its destructor it calls connection error on ImageDownloaderImpl.
- the strong binding of ImageDownloaderImpl causes it to be destructed.
- the destructor calls RenderThread::Get()->RemoveObserver(this),
  which is not valid any more.

BUG=640999
TBR=jochen@chromium.org

Committed: https://crrev.com/d2ac649bc631233f4355860a011d3d30f985bb67
Cr-Commit-Position: refs/heads/master@{#414985}

[leonhsl(Using Gerrit), 2016-08-26 06:48:27]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 06:48:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 07:53:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 07:53:36]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-08-26 07:56:18]

leon.han@intel.com changed reviewers:
+ jochen@chromium.org, yzshen@chromium.org

[leonhsl(Using Gerrit), 2016-08-26 07:56:19]

Hi, would you PTAL at this? Thanks.

[jochen (gone - plz use gerrit), 2016-08-26 15:06:40]

jochen@chromium.org changed reviewers:
+ haraken@chromium.org

[jochen (gone - plz use gerrit), 2016-08-26 15:06:40]

+haraken

can this still happen now that all the shutdown stuff was ripped out?

[haraken, 2016-08-26 15:24:54]

On 2016/08/26 15:06:40, jochen wrote:
> +haraken
> 
> can this still happen now that all the shutdown stuff was ripped out?

Maybe this could still happen because I've just removed blink::shutdown... We
should remove more code from the shutdown sequence so that this kind of crash is
gone.

[yzshen1, 2016-08-26 19:20:31]

LGTM

https://codereview.chromium.org/2286603002/diff/1/content/renderer/image_down...
File content/renderer/image_downloader/image_downloader_impl.cc (right):

https://codereview.chromium.org/2286603002/diff/1/content/renderer/image_down...
content/renderer/image_downloader/image_downloader_impl.cc:134: if (thread)
Please comment on why this null check is necessary

[leonhsl(Using Gerrit), 2016-08-29 02:17:30]

Uploaded ps#2 to address Yuzhu's comments, Thanks~

Hi, jochen@, haraken@, do we need to land this CL for now to fix the bug
http://crbug.com/640999 ? As this bug is marked as high security severity now..
Thanks.

https://codereview.chromium.org/2286603002/diff/1/content/renderer/image_down...
File content/renderer/image_downloader/image_downloader_impl.cc (right):

https://codereview.chromium.org/2286603002/diff/1/content/renderer/image_down...
content/renderer/image_downloader/image_downloader_impl.cc:134: if (thread)
On 2016/08/26 19:20:31, yzshen1 wrote:
> Please comment on why this null check is necessary

Done and thanks.

[leonhsl(Using Gerrit), 2016-08-29 02:17:36]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 02:17:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2016-08-29 02:34:17]

On 2016/08/29 02:17:30, leonhsl wrote:
> Uploaded ps#2 to address Yuzhu's comments, Thanks~
> 
> Hi, jochen@, haraken@, do we need to land this CL for now to fix the bug
> http://crbug.com/640999 ? As this bug is marked as high security severity
now..
> Thanks.
> 
>
https://codereview.chromium.org/2286603002/diff/1/content/renderer/image_down...
> File content/renderer/image_downloader/image_downloader_impl.cc (right):
> 
>
https://codereview.chromium.org/2286603002/diff/1/content/renderer/image_down...
> content/renderer/image_downloader/image_downloader_impl.cc:134: if (thread)
> On 2016/08/26 19:20:31, yzshen1 wrote:
> > Please comment on why this null check is necessary
> 
> Done and thanks.

Yes, please land. LGTM.

[commit-bot: I haz the power, 2016-08-29 03:22:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 03:22:15]

Dry run: Exceeded global retry quota

[leonhsl(Using Gerrit), 2016-08-29 05:04:56]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 05:05:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 06:35:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 06:35:04]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-08-29 06:36:48]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 06:37:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 07:46:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 07:46:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-08-29 08:26:41]

Description was changed from

==========
Forbid ImageDownloaderImpl from accessing RenderThread instance after message
loop shutdown.

This CL fixes the crash caused by following possible scenario:
- the message loop is shutdown.
- during its destructor it calls connection error on ImageDownloaderImpl.
- the strong binding of ImageDownloaderImpl causes it to be destructed.
- the destructor calls RenderThread::Get()->RemoveObserver(this),
  which is not valid any more.

BUG=640999
==========

to

==========
Forbid ImageDownloaderImpl from accessing RenderThread instance after message
loop shutdown.

This CL fixes the crash caused by following possible scenario:
- the message loop is shutdown.
- during its destructor it calls connection error on ImageDownloaderImpl.
- the strong binding of ImageDownloaderImpl causes it to be destructed.
- the destructor calls RenderThread::Get()->RemoveObserver(this),
  which is not valid any more.

BUG=640999
TBR=jochen@chromium.org
==========

[leonhsl(Using Gerrit), 2016-08-29 08:29:19]

On 2016/08/29 02:34:17, haraken wrote:
> On 2016/08/29 02:17:30, leonhsl wrote:
> > Uploaded ps#2 to address Yuzhu's comments, Thanks~
> > 
> > Hi, jochen@, haraken@, do we need to land this CL for now to fix the bug
> > http://crbug.com/640999 ? As this bug is marked as high security severity
> now..
> > Thanks.
> > 
> >
>
https://codereview.chromium.org/2286603002/diff/1/content/renderer/image_down...
> > File content/renderer/image_downloader/image_downloader_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2286603002/diff/1/content/renderer/image_down...
> > content/renderer/image_downloader/image_downloader_impl.cc:134: if (thread)
> > On 2016/08/26 19:20:31, yzshen1 wrote:
> > > Please comment on why this null check is necessary
> > 
> > Done and thanks.
> 
> Yes, please land. LGTM.

Thanks haraken@ a lot for kindly help. Assuming jochen@ are OK with the approval
from haraken@ and Yuzhu, let me TBR and land this now.

[leonhsl(Using Gerrit), 2016-08-29 08:29:35]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2016-08-29 08:29:35]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2286603002/#ps20001
(title: "Add comments to explain why we do the null check")

[commit-bot: I haz the power, 2016-08-29 08:29:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 09:06:33]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-29 09:08:15]

Description was changed from

==========
Forbid ImageDownloaderImpl from accessing RenderThread instance after message
loop shutdown.

This CL fixes the crash caused by following possible scenario:
- the message loop is shutdown.
- during its destructor it calls connection error on ImageDownloaderImpl.
- the strong binding of ImageDownloaderImpl causes it to be destructed.
- the destructor calls RenderThread::Get()->RemoveObserver(this),
  which is not valid any more.

BUG=640999
TBR=jochen@chromium.org
==========

to

==========
Forbid ImageDownloaderImpl from accessing RenderThread instance after message
loop shutdown.

This CL fixes the crash caused by following possible scenario:
- the message loop is shutdown.
- during its destructor it calls connection error on ImageDownloaderImpl.
- the strong binding of ImageDownloaderImpl causes it to be destructed.
- the destructor calls RenderThread::Get()->RemoveObserver(this),
  which is not valid any more.

BUG=640999
TBR=jochen@chromium.org

Committed: https://crrev.com/d2ac649bc631233f4355860a011d3d30f985bb67
Cr-Commit-Position: refs/heads/master@{#414985}
==========

[commit-bot: I haz the power, 2016-08-29 09:08:16]

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