Resubmit of 149313003: Significantly cleans up the ImageWriter Operation class and subclasses.

Fixes memory leak in 149313003.

The leak was caused by the modular testing not forcing the run-and-clear of
clean-up callbacks.  This left a callback owned by the object which increment
the refcount of the object.  These callbacks should be removed and a different
method of ensuring cleanup should be employed.

Ideally we can find a way to get rid of the ref-counting on the object to avoid
this sort of issue in the future, but currently the operation manager lives on
the IO thread and the Operation must work on the FILE thread.  A sequenced task
queue or a worker may be useful.

Previous CL: https://codereview.chromium.org/149313003/

BUG=292956
BUG=337883
BUG=335404
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251984

[Drew Haven, 2014-02-18 21:47:36]

Toni,

Apparently I introduced a memory leak in that CL from last week.

What happened is that the class has a list of callbacks to run when it's done. 
However, when I was only running one phase of the operation it wasn't running
those callbacks because it didn't error and it didn't finish.  The callback has
a reference back to the object and since it is owned by the object it creates a
circular reference.

The quick fix is to add a call to Cancel() in the tests to force those callbacks
to run before exiting.  In the long run I'd like to remove the use of
ref-counting entirely, but I'm not sure if that's possible since I need to do IO
operations from the file thread and communicate with the API on the IO thread. 
Any such solution is out of scope for this CL.  More changes are necessary to
reduce the probability of bugs in the future.

[tbarzic, 2014-02-18 22:13:14]

btw. uploading the reverted cl (with no fixes) first would make it easier to see
what actually changed :)

Also, reusing the original codereview issue should be fine too.

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/image_writer_private/operation_unittest.cc
(right):

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/image_writer_private/operation_unittest.cc:146: //
Chrome OS tests don't actually write to the disk because that's handled by
nit: update the comment

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc
(right):

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc:130:
operation->Cancel();
this is the only change, right (here and in the rest of the tests)?
if so, LGTM

(but consider adding this to test in other files, or even better (if possible),
add it to TearDown method)

[Drew Haven, 2014-02-19 00:10:19]

On 2014/02/18 22:13:14, tbarzic wrote:
> btw. uploading the reverted cl (with no fixes) first would make it easier to
see
> what actually changed :)

Oh, that's a good point about the two CLs.  Also, I didn't realize you could
reuse a CL that had been reverted.  Something gave me the impression it couldn't
be used, but I guess it can be reopened?  Does it need approval again?
 
> Also, reusing the original codereview issue should be fine too.
> 
>
https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
> File chrome/browser/extensions/api/image_writer_private/operation_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/image_writer_private/operation_unittest.cc:146:
//
> Chrome OS tests don't actually write to the disk because that's handled by
> nit: update the comment
> 
>
https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
> File
>
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
>
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc:130:
> operation->Cancel();
> this is the only change, right (here and in the rest of the tests)?
> if so, LGTM
> 
> (but consider adding this to test in other files, or even better (if
possible),
> add it to TearDown method)

[Drew Haven, 2014-02-19 00:49:19]

I think I addressed these two things.  For the moment I'd like to get this in,
but I know it needs to be cleaned up and I've assigned myself a bug to get it
done.  With the windows-writing done I should be able to prioritize that stuff.

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/image_writer_private/operation_unittest.cc
(right):

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/image_writer_private/operation_unittest.cc:146: //
Chrome OS tests don't actually write to the disk because that's handled by
On 2014/02/18 22:13:14, tbarzic wrote:
> nit: update the comment

I just moved this comment to a compiler condition around the writing check
itself.  ChromeOS code still works fine for the rest of the test.

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc
(right):

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc:130:
operation->Cancel();
On 2014/02/18 22:13:14, tbarzic wrote:
> this is the only change, right (here and in the rest of the tests)?
> if so, LGTM
> 
> (but consider adding this to test in other files, or even better (if
possible),
> add it to TearDown method)

I need to find a better way to do it in general. This is the only place it's
actually necessary.  My next CL already moves creation of the operation up into
the test fixture setup and so the cancel can be put there, so for this CL I'll
leave it and I think I'll add a general TODO to operation.h to fix this and warn
any other readers.

I need to mentally figure out what the "right" way to approach these classes
that touch multiple threads is.  For example, am I sure that everywhere I
dereference the manager_ WeakPtr that I'm on the UI thread?  I think so, but I
may have made a mistake which would crash shutdown if the system
deallocated/accessed that pointer on the UI thread after it had been bound to
the FILE thread.

[tbarzic, 2014-02-19 01:20:15]

for reusing cls:
I remember a PSA on chromium-dev some time ago about banning this (by adding
presubmit check), but there was a push back on that.

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/image_writer_private/operation_unittest.cc
(right):

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/image_writer_private/operation_unittest.cc:146: //
Chrome OS tests don't actually write to the disk because that's handled by
On 2014/02/19 00:49:19, Drew Haven wrote:
> On 2014/02/18 22:13:14, tbarzic wrote:
> > nit: update the comment
> 
> I just moved this comment to a compiler condition around the writing check
> itself.  ChromeOS code still works fine for the rest of the test.

yeah, we only have linux and cros implementation... do'h

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc
(right):

https://codereview.chromium.org/170713007/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc:130:
operation->Cancel();
On 2014/02/19 00:49:19, Drew Haven wrote:
> On 2014/02/18 22:13:14, tbarzic wrote:
> > this is the only change, right (here and in the rest of the tests)?
> > if so, LGTM
> > 
> > (but consider adding this to test in other files, or even better (if
> possible),
> > add it to TearDown method)
> 
> I need to find a better way to do it in general. This is the only place it's
> actually necessary.  My next CL already moves creation of the operation up
into
> the test fixture setup and so the cancel can be put there, so for this CL I'll
> leave it and I think I'll add a general TODO to operation.h to fix this and
warn
> any other readers.
> 

sounds ok

> I need to mentally figure out what the "right" way to approach these classes
> that touch multiple threads is.  For example, am I sure that everywhere I
> dereference the manager_ WeakPtr that I'm on the UI thread?  I think so, but I
> may have made a mistake which would crash shutdown if the system
> deallocated/accessed that pointer on the UI thread after it had been bound to
> the FILE thread.

yeah, that's why ref counting is evil.
Though, it's better to have it crash than to have it fail silently (crashes get
fixed) :)

[Drew Haven, 2014-02-19 02:02:51]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-02-19 02:03:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/170713007/80001

[commit-bot: I haz the power, 2014-02-19 07:55:23]

Change committed as 251984