Remove some PlatformFile instances from ChromeOS Drive.

Remove some PlatformFile instances from ChromeOS Drive.

BUG=322664
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260326

[rvargas (doing something else), 2014-03-26 22:10:09]

PTAL

[kinaba, 2014-03-27 01:25:56]

On 2014/03/26 22:34:24, satorux1 wrote:

LGTM with one request.

The LocalFileReader class was introduced (http://crbug.com/235385) to avoid
memory
leak in tests from net::FileStream that, at that time, didn't take TaskRunner as
an
argument. I think the original issue has gone since now net::FileStream takes
TaskRunner in constructor, but could you double check that the tests won't fail
in
asan bots?

[rvargas (doing something else), 2014-03-27 01:44:24]

On 2014/03/27 01:25:56, kinaba wrote:
> On 2014/03/26 22:34:24, satorux1 wrote:
> 
> LGTM with one request.
> 
> The LocalFileReader class was introduced (http://crbug.com/235385) to avoid
> memory
> leak in tests from net::FileStream that, at that time, didn't take TaskRunner
as
> an
> argument. I think the original issue has gone since now net::FileStream takes
> TaskRunner in constructor, but could you double check that the tests won't
fail
> in
> asan bots?

Thanks. Running now on all the CrOS memory try bots.

[rvargas (doing something else), 2014-03-28 21:17:47]

+willchan

It turns out that asan detects a problem with the unit tests:
Direct leak of 72 byte(s) in 1 object(s) allocated from:
    #0 operator new(unsigned long)
    #1 base::internal::PostTaskAndReplyImpl::PostTaskAndReply()
    #2 base::TaskRunner::PostTaskAndReply()
    #3 net::FileStream::Context::CloseAndDelete()
net/base/file_stream_context.cc:277
    #4 net::FileStream::~FileStream() net/base/file_stream.cc:70
    #5 operator() base/memory/scoped_ptr.h:137
    #6 ~scoped_ptr_impl base/memory/scoped_ptr.h:220
    #7 ~scoped_ptr base/memory/scoped_ptr.h:432
    #8 ~LocalFileReaderTest
chrome/browser/chromeos/drive/local_file_reader_unittest.cc:45
    #9
drive::util::LocalFileReaderTest_FullRead_Test::~LocalFileReaderTest_FullRead_Test()

The issue is that the text fixture has
  scoped_ptr<base::Thread> worker_thread_;
  scoped_ptr<LocalFileReader> file_reader_;

so at destruction it just kills the worker thread right after LocalFileReader.

I think this is an issue with the test, and the reason the old code doesn't fail
is because there is no memory involved on the cleanup task.... but the task to
close the file is not executed either (so the handle is leaked).

The new code requires a task to be posted back to the main thread to release the
memory, and that triggers the leak.

So I'm changing net::FileStream anyway to remove that extra trip back, which
makes the bot happy.

[rvargas (doing something else), 2014-03-28 21:20:47]

On 2014/03/28 21:17:47, rvargas wrote:
> +willchan
> 
> It turns out that asan detects a problem with the unit tests:
> Direct leak of 72 byte(s) in 1 object(s) allocated from:
>     #0 operator new(unsigned long)
>     #1 base::internal::PostTaskAndReplyImpl::PostTaskAndReply()
>     #2 base::TaskRunner::PostTaskAndReply()
>     #3 net::FileStream::Context::CloseAndDelete()
> net/base/file_stream_context.cc:277
>     #4 net::FileStream::~FileStream() net/base/file_stream.cc:70
>     #5 operator() base/memory/scoped_ptr.h:137
>     #6 ~scoped_ptr_impl base/memory/scoped_ptr.h:220
>     #7 ~scoped_ptr base/memory/scoped_ptr.h:432
>     #8 ~LocalFileReaderTest
> chrome/browser/chromeos/drive/local_file_reader_unittest.cc:45
>     #9
>
drive::util::LocalFileReaderTest_FullRead_Test::~LocalFileReaderTest_FullRead_Test()
> 
> The issue is that the text fixture has
>   scoped_ptr<base::Thread> worker_thread_;
>   scoped_ptr<LocalFileReader> file_reader_;
> 
> so at destruction it just kills the worker thread right after LocalFileReader.
> 
> I think this is an issue with the test, and the reason the old code doesn't
fail
> is because there is no memory involved on the cleanup task.... but the task to
> close the file is not executed either (so the handle is leaked).
> 
> The new code requires a task to be posted back to the main thread to release
the
> memory, and that triggers the leak.
> 
> So I'm changing net::FileStream anyway to remove that extra trip back, which
> makes the bot happy.

Small correction. The cleanup task may be executed anyway, but even if it is,
the reponse task never runs... hence the leak.

[willchan no longer on Chromium, 2014-03-28 21:58:35]

lgtm

[rvargas (doing something else), 2014-03-28 22:27:31]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-03-28 22:49:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rvargas@chromium.org/211483004/120001

[commit-bot: I haz the power, 2014-03-29 00:57:30]

Change committed as 260326