Fix race conditions in ImageDecoder.

Fix race conditions in ImageDecoder.

BUG=474590
Committed: https://crrev.com/c5eeb9f36639f5ff01f357926d3022e8e2b0a5ce
Cr-Commit-Position: refs/heads/master@{#326350}

[Lei Zhang, 2015-04-20 18:47:46]

thestig@chromium.org changed reviewers:
+ twellington@chromium.org

[Lei Zhang, 2015-04-20 19:03:28]

I'm worried https://codereview.chromium.org/1068743002/ is a bit too
complicated, so I took a stab at fixing this bug. Can you take a look and see if
this works?

[Lei Zhang, 2015-04-20 21:52:59]

And some updates in patch set 2 and 3.

[Theresa, 2015-04-20 22:13:14]

On 2015/04/20 19:03:28, Lei Zhang wrote:
> I'm worried https://codereview.chromium.org/1068743002/ is a bit too
> complicated, so I took a stab at fixing this bug. Can you take a look and see
if
> this works?

Yes, I think this fixes both of the race conditions that CL was trying to
address. I like the simpler approach in this CL (fewer subclasses is better).

#1, Cancel() being called immediately after Start() is fixed in this CL by
moving the logic that inserts requests into the RequestMap from
DecodeImageInSandbox() to StartWithOptionsImpl().

#2, OnImageDecoded()/OnDecodeImageFailed() being called (very shortly) after the
request is cancelled while the ImageRequest object is in the process of being
deleted, is fixed by the addition of RunOnImageDecoded and
RunOnDecodeImageFailed.

[Theresa, 2015-04-20 22:35:26]

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:53: ImageDecoder::Cancel(this);
Do we want a DCHECK explicitly enforcing the destructor being called from
task_runner_? Or is it so unlikely that the destructor will be called from
another thread that it's not worth the check?

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/image_de...
chrome/browser/image_decoder.h:52: ImageRequest();
I like this introduction, definitely makes things easier for the clients.

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/image_de...
chrome/browser/image_decoder.h:100: void DecodeImageInSandbox(int
image_request_counter,
int request_id?

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(left):

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:56:
BrowserThread::IO)),
I'm not familiar enough with this class to know whether or not it's constructed
on the IO thread... or whether or not OnImageDecoded/OnDecodeImageFailed
actually need to be called on the IO thread since they just invoke
CopyOrMoveFileValidator::ResultCallback.Run()?

[Lei Zhang, 2015-04-21 00:44:00]

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/image_de...
File chrome/browser/image_decoder.cc (right):

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/image_de...
chrome/browser/image_decoder.cc:53: ImageDecoder::Cancel(this);
On 2015/04/20 22:35:26, twellington wrote:
> Do we want a DCHECK explicitly enforcing the destructor being called from
> task_runner_? Or is it so unlikely that the destructor will be called from
> another thread that it's not worth the check?

We are checking this via |sequence_checker_|. When the UI thread is being
destroyed in some tests, task_runner_->RunsTasksOnCurrentThread() returns false
even though it's being called on the UI thread.

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/image_de...
File chrome/browser/image_decoder.h (right):

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/image_de...
chrome/browser/image_decoder.h:100: void DecodeImageInSandbox(int
image_request_counter,
On 2015/04/20 22:35:26, twellington wrote:
> int request_id?

Done.

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(left):

https://codereview.chromium.org/1067593005/diff/40001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:56:
BrowserThread::IO)),
On 2015/04/20 22:35:26, twellington wrote:
> I'm not familiar enough with this class to know whether or not it's
constructed
> on the IO thread... or whether or not OnImageDecoded/OnDecodeImageFailed
> actually need to be called on the IO thread since they just invoke
> CopyOrMoveFileValidator::ResultCallback.Run()?

The only place that calls the ImageDecoderDelegateAdapter ctor is in
SupportedImageTypeValidator::OnFileOpen(), which clearly runs on the IO thread.

[Theresa, 2015-04-21 15:04:02]

lgtm

[Lei Zhang, 2015-04-21 19:24:39]

thestig@chromium.org changed reviewers:
+ asargent@chromium.org

[Lei Zhang, 2015-04-21 19:24:39]

+asargent who reviewed https://codereview.chromium.org/931993002/

[asargent_no_longer_on_chrome, 2015-04-21 21:14:57]

one question about test, otherwise lgtm

https://codereview.chromium.org/1067593005/diff/60001/chrome/browser/image_de...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/1067593005/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder_browsertest.cc:68: test_request.reset();
At this point aren't we left with a stale pointer to the TestImageRequest object
inside the ImageDecoder's RequestMap? Did you mean to call ImageDecoder::Cancel
inside of ~TestImageRequest if quit_called_ is false?

[Lei Zhang, 2015-04-21 22:19:05]

https://codereview.chromium.org/1067593005/diff/60001/chrome/browser/image_de...
File chrome/browser/image_decoder_browsertest.cc (right):

https://codereview.chromium.org/1067593005/diff/60001/chrome/browser/image_de...
chrome/browser/image_decoder_browsertest.cc:68: test_request.reset();
On 2015/04/21 21:14:57, Antony Sargent wrote:
> At this point aren't we left with a stale pointer to the TestImageRequest
object
> inside the ImageDecoder's RequestMap? Did you mean to call
ImageDecoder::Cancel
> inside of ~TestImageRequest if quit_called_ is false?

When the code hits TestImageRequest's dtor, it calls ImageRequest's dtor, which
calls ImageDecoder::Cancel(). Without the fix, this test case ends up causing a
use-after-free because ImageDecoder::DecodeImageInSandbox() still had a raw
(freed) pointer. That no longer happens with the fix.

[asargent_no_longer_on_chrome, 2015-04-21 22:55:23]

On 2015/04/21 22:19:05, Lei Zhang wrote:
>
https://codereview.chromium.org/1067593005/diff/60001/chrome/browser/image_de...
> File chrome/browser/image_decoder_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1067593005/diff/60001/chrome/browser/image_de...
> chrome/browser/image_decoder_browsertest.cc:68: test_request.reset();
> On 2015/04/21 21:14:57, Antony Sargent wrote:
> > At this point aren't we left with a stale pointer to the TestImageRequest
> object
> > inside the ImageDecoder's RequestMap? Did you mean to call
> ImageDecoder::Cancel
> > inside of ~TestImageRequest if quit_called_ is false?
> 
> When the code hits TestImageRequest's dtor, it calls ImageRequest's dtor,
which
> calls ImageDecoder::Cancel(). Without the fix, this test case ends up causing
a
> use-after-free because ImageDecoder::DecodeImageInSandbox() still had a raw
> (freed) pointer. That no longer happens with the fix.

Ah, ok, I forgot to look there.

[Lei Zhang, 2015-04-22 17:15:25]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-04-22 17:15:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1067593005/#ps100001
(title: "Use TestBrowserThreadBundle, cleanup tests")

[commit-bot: I haz the power, 2015-04-22 17:15:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1067593005/100001

[commit-bot: I haz the power, 2015-04-22 18:42:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-22 18:43:56]

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