New issue
Advanced search Search tips

Issue 778839 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 3
Type: Bug



Sign in to add a comment

AsyncFileUtil should clean up base::File objects properly if the callback goes away

Project Member Reported by jrumm...@chromium.org, Oct 26 2017

Issue description

I'm working on creating a mojo file service for use by Content Decryption Modules running in the utility process. This service calls AsyncFileUtil::CreateOrOpen() when opening the file. Normally the file returned in the CreateOrOpenCallback is simply passed to mojo to send to the client, as the service doesn't do any I/O. However, if mojo destroys the service (due to a connection error, for example), then the callback doesn't run due to a weak_ptr, and the base::File is cleaned up on the services thread, which doesn't allow for blocking calls.

Example crash (Linux):
[7095:7095:1025/162344.748076:FATAL:thread_restrictions.cc(29)] Check failed: !g_blocking_disallowed.Get().Get(). Function marked as blocking was called from a scope that disallows blocking! If this task is running inside the TaskScheduler, it needs to have MayBlock() in its TaskTraits. Otherwise, consider making this blocking work asynchronous or, as a last resort, you may use ScopedAllowBlocking in a narrow scope.
#0 0x0000056297a7 base::debug::StackTrace::StackTrace()
#1 0x000005647e41 logging::LogMessage::~LogMessage()
#2 0x0000056b6b60 base::AssertBlockingAllowed()
#3 0x000005638837 base::File::Close()
#4 0x00000562ef09 base::File::~File()
#5 0x0000078b2c2d storage::(anonymous namespace)::RunCreateOrOpenCallback()
#6 0x0000078b510c _ZN4base8internal7InvokerINS0_9BindStateIPFvPN7storage26FileSystemOperationContextERKNS_17RepeatingCallbackIFvNS_4FileERKNS6_IFvvEEEEEES7_EJNS0_12OwnedWrapperIS4_EESD_EEEFvS7_EE3RunEPNS0_13BindStateBaseEOS7_
#7 0x0000041c32b9 base::internal::ReplyAdapter<>()
#8 0x000003b2423b _ZN4base8internal7InvokerINS0_9BindStateIPFvNS_12OnceCallbackIFvN10extensions12_GLOBAL__N_112UnpackResultEEEEPS6_EJS8_NS0_12OwnedWrapperIS6_EEEEEFvvEE7RunOnceEPNS0_13BindStateBaseE
#9 0x0000056add3e base::(anonymous namespace)::PostTaskAndReplyRelay::RunReplyAndSelfDestruct()

 

Comment 1 by jsb...@chromium.org, Oct 27 2017

Do you happen to know if the callback would report IsCancelled() within the RunCreateOrOpenCallback?

https://cs.chromium.org/chromium/src/storage/browser/fileapi/async_file_util_adapter.cc?type=cs&sq=package:chromium&l=139

If so, RunCreateOrOpenCallback could check and if the callback will be a no-op then pass the base::File on to the FileSystemOperationContext's task runner for destruction.

I haven't fully dug into where the call against a weakptr bottoms out in a no-op. If the RunCreateOrOpenCallback doesn't have enough context to know that the callback will be a no-op I don't see how AsyncFileUtil can help here - you may need to pass in a wrapper callback that has the weak ptr as an explicit arg and can check it. 
I ended up providing CreateOrOpen() a callback to a static method that takes a weak ptr and the "real" callback. If the weak ptr is null, post a task to free the base::File object.

I just tried checking IsCancelled() in my code. (On Windows) It is true if the weak ptr is NULL, so it looks like it will work. I'll try modifying AsyncFileUtil to check, remove the static method method in my code, and try. If it works I'll submit a CL to fix AsyncFileUtil.

Comment 3 by jsb...@chromium.org, Oct 27 2017

Cc: jsb...@chromium.org
Owner: jrumm...@chromium.org
Status: Assigned (was: Untriaged)
Thanks! If that doesn't pan out we should update the docs for CreateOrOpen to clarify that the passed callback must dispose of the File on an appropriate sequence.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b5dbaf4b2c13bb6da70486401429fa6ad2bc77e

commit 4b5dbaf4b2c13bb6da70486401429fa6ad2bc77e
Author: John Rummell <jrummell@chromium.org>
Date: Fri Nov 03 23:54:46 2017

Clean up base::File objects properly if the callback goes away

If the callback has been cancelled (e.g due to weak ptr and that object has
been destroyed), run base::File::Close() on the thread that allows I/O.

BUG= 778839 
TEST=tests using CdmFile still pass

Change-Id: I116b1d6068b55b0bd3c429a6dbdcfa9d662c0927
Reviewed-on: https://chromium-review.googlesource.com/747878
Commit-Queue: John Rummell <jrummell@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513971}
[modify] https://crrev.com/4b5dbaf4b2c13bb6da70486401429fa6ad2bc77e/content/browser/media/cdm_file_impl.cc
[modify] https://crrev.com/4b5dbaf4b2c13bb6da70486401429fa6ad2bc77e/storage/browser/fileapi/async_file_util_adapter.cc

Status: Fixed (was: Assigned)

Sign in to add a comment