Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2425673006:
Make URLFetcherFileWriter::Finish() skip closing file when there is an error (Closed)
DescriptionMake URLFetcherFileWriter::Finish() skip closing file when there is an error
This CL makes URLFetcherFileWriter::Finish() skip closing file when there is an
error. If there is an error, Finish() will delete the file after any pending
operation completes.
TBR=hashimoto@chromium.org
TBR=mkwst@chromium.org
BUG=487732, 656692
Committed: https://crrev.com/ee63375d9c88e55c832cdf7f6181eb3ffc776695
Cr-Commit-Position: refs/heads/master@{#426540}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments and added tests #
Total comments: 6
Patch Set 3 : Address comments to move |owns_file = true| to Initialize() #Patch Set 4 : Fix another subclass #Patch Set 5 : Fix another subclass #
Total comments: 4
Patch Set 6 : Cancel pending callback when Finish(net_error<0) is called #
Total comments: 10
Patch Set 7 : Address comments to invalid weak ptrs #Patch Set 8 : Fix test #Messages
Total messages: 58 (35 generated)
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Seems there are some other classes which subclass URLFetcherResponseWriter. Is it okay to add an overloaded method Finish(net_error, callback) instead of changing the original one?
https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:480: bytes_read > 0 ? OK : bytes_read, Hrm...I'd normally ask for an integration test that runs into the problem, but I'd actually really like to fix URLRequest so that this can't happen. Fixing the API would break the test. (And I'd like to fix the API sooner rather than later) https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... File net/url_request/url_fetcher_response_writer_unittest.cc (right): https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_response_writer_unittest.cc:135: rv = writer_->Initialize(callback.callback()); Maybe a test where Initialize is pending, too? Don't want to test everything, but initializing is magic. https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_response_writer_unittest.cc:144: EXPECT_FALSE(base::PathExists(file_path_)); Is it worth having a test where we Initialize() the writer again before spinning the message loop, just out of paranoia?
On 2016/10/18 20:17:00, xunjieli wrote: > Seems there are some other classes which subclass URLFetcherResponseWriter. Is > it okay to add an overloaded method Finish(net_error, callback) instead of > changing the original one? Since there are only 5 subclasses, think it's easy enough to just update them all (And those changes are TBR-able, so just make sure you add owners is reviewers when you TBR them, and no need to wait for their signoff.
Thanks! a couple of questions before I go ahead and change the subclasses. https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:480: bytes_read > 0 ? OK : bytes_read, On 2016/10/18 20:20:01, mmenke wrote: > Hrm...I'd normally ask for an integration test that runs into the problem, but > I'd actually really like to fix URLRequest so that this can't happen. Fixing > the API would break the test. (And I'd like to fix the API sooner rather than > later) I see. So in the future URLRequest::Delegate::OnReadCompleted() won't be called with an net error (bytes_read < 0)? https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... File net/url_request/url_fetcher_response_writer_unittest.cc (right): https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_response_writer_unittest.cc:135: rv = writer_->Initialize(callback.callback()); On 2016/10/18 20:20:02, mmenke wrote: > Maybe a test where Initialize is pending, too? Don't want to test everything, > but initializing is magic. Done. Ah I should have thought about that. The new test actually caught a bug where CloseAndDelete() checks |owns_file_| synchronously before Open() completes, so we won't delete the file in this case (see comment below) https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_response_writer_unittest.cc:144: EXPECT_FALSE(base::PathExists(file_path_)); On 2016/10/18 20:20:01, mmenke wrote: > Is it worth having a test where we Initialize() the writer again before spinning > the message loop, just out of paranoia? Done. https://codereview.chromium.org/2425673006/diff/20001/net/url_request/url_fet... File net/url_request/url_fetcher_response_writer_unittest.cc (right): https://codereview.chromium.org/2425673006/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_response_writer_unittest.cc:160: EXPECT_FALSE(base::PathExists(file_path_)); Here's the line that fails. CloseAndDelete() is an no-op if |owns_file_| is false. |owns_file_| is true only when Initialize completes asynchronously :( Maybe we should change read access of |owns_file_| on the task runner thread? https://codereview.chromium.org/2425673006/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_response_writer_unittest.cc:174: EXPECT_THAT(callback.GetResult(rv), IsOk()); Will using the same TestCompletionCallback be a problem? I see other tests doing so. I don't suppose TestCompletionCallback resets its callback?
https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:480: bytes_read > 0 ? OK : bytes_read, On 2016/10/18 21:18:07, xunjieli wrote: > On 2016/10/18 20:20:01, mmenke wrote: > > Hrm...I'd normally ask for an integration test that runs into the problem, but > > I'd actually really like to fix URLRequest so that this can't happen. Fixing > > the API would break the test. (And I'd like to fix the API sooner rather than > > later) > > I see. So in the future URLRequest::Delegate::OnReadCompleted() won't be called > with an net error (bytes_read < 0)? No, in the future, I want it not to call into the Delegate with an error unless there's a pending read/start operation, which I think this the only case this happens in? Cancellation through the URLFetcher interface goes through a different path, and doesn't run into this problem, I believe. And this code doesn't currently open/write to the file while trying to read remote data. https://codereview.chromium.org/2425673006/diff/20001/net/url_request/url_fet... File net/url_request/url_fetcher_response_writer_unittest.cc (right): https://codereview.chromium.org/2425673006/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_response_writer_unittest.cc:160: EXPECT_FALSE(base::PathExists(file_path_)); On 2016/10/18 21:18:08, xunjieli wrote: > Here's the line that fails. > > CloseAndDelete() is an no-op if |owns_file_| is false. > |owns_file_| is true only when Initialize completes asynchronously :( > > Maybe we should change read access of |owns_file_| on the task runner thread? Should we just set owns file before each post task to URLFetcherFileWriter::DidOpenFile? If it fails with an error, we'll try to delete the file, anyways, and aborting seems like kind of the same thing. https://codereview.chromium.org/2425673006/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_response_writer_unittest.cc:174: EXPECT_THAT(callback.GetResult(rv), IsOk()); On 2016/10/18 21:18:07, xunjieli wrote: > Will using the same TestCompletionCallback be a problem? I see other tests doing > so. I don't suppose TestCompletionCallback resets its callback? We should use a different callback here - This should work, but seems a bad idea. If it were to be called as a result of the Write() call, we could incorrectly pass the test (Admittedly, if Initialize() called it twice or something, would have the same issue, but we generally don't worry about that). Using a different callback also lets us check that the one passed to Write is never invoked as a result of the Write call.
Patchset #3 (id:40001) has been deleted
Description was changed from ========== Make URLFetcherFileWriter::Finish() skip closing file when there is an error This CL makes URLFetcherFileWriter::Finish() skip closing file when there is an error. If there is an error, Finish() will delete the file after any pending operation completes. BUG=487732, 656692 ========== to ========== Make URLFetcherFileWriter::Finish() skip closing file when there is an error This CL makes URLFetcherFileWriter::Finish() skip closing file when there is an error. If there is an error, Finish() will delete the file after any pending operation completes. BUG=487732, 656692 ==========
xunjieli@chromium.org changed reviewers: + dgozman@chromium.org, hashimoto@chromium.org, rouslan@chromium.org
hashimoto@: PTAL google_apis/drive/base_requests.h rouslan@: PTAL third_party/libaddressinput/chromium/chrome_metadata_source.cc dgozman@: PTAL chrome/browser/devtools/devtools_ui_bindings.cc Since the change in these three files are really small. I will be TBR-ing once I get a l-g-t-m from mmenke@. Thanks! https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/2425673006/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:480: bytes_read > 0 ? OK : bytes_read, On 2016/10/18 21:40:46, mmenke wrote: > On 2016/10/18 21:18:07, xunjieli wrote: > > On 2016/10/18 20:20:01, mmenke wrote: > > > Hrm...I'd normally ask for an integration test that runs into the problem, > but > > > I'd actually really like to fix URLRequest so that this can't happen. > Fixing > > > the API would break the test. (And I'd like to fix the API sooner rather > than > > > later) > > > > I see. So in the future URLRequest::Delegate::OnReadCompleted() won't be > called > > with an net error (bytes_read < 0)? > > No, in the future, I want it not to call into the Delegate with an error unless > there's a pending read/start operation, which I think this the only case this > happens in? > > Cancellation through the URLFetcher interface goes through a different path, and > doesn't run into this problem, I believe. And this code doesn't currently > open/write to the file while trying to read remote data. Acknowledged. Thanks for explaining! https://codereview.chromium.org/2425673006/diff/20001/net/url_request/url_fet... File net/url_request/url_fetcher_response_writer_unittest.cc (right): https://codereview.chromium.org/2425673006/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_response_writer_unittest.cc:160: EXPECT_FALSE(base::PathExists(file_path_)); On 2016/10/18 21:40:46, mmenke wrote: > On 2016/10/18 21:18:08, xunjieli wrote: > > Here's the line that fails. > > > > CloseAndDelete() is an no-op if |owns_file_| is false. > > |owns_file_| is true only when Initialize completes asynchronously :( > > > > Maybe we should change read access of |owns_file_| on the task runner thread? > > Should we just set owns file before each post task to > URLFetcherFileWriter::DidOpenFile? If it fails with an error, we'll try to > delete the file, anyways, and aborting seems like kind of the same thing. Done. Right I think that's equivalent and is better! https://codereview.chromium.org/2425673006/diff/20001/net/url_request/url_fet... net/url_request/url_fetcher_response_writer_unittest.cc:174: EXPECT_THAT(callback.GetResult(rv), IsOk()); On 2016/10/18 21:40:46, mmenke wrote: > On 2016/10/18 21:18:07, xunjieli wrote: > > Will using the same TestCompletionCallback be a problem? I see other tests > doing > > so. I don't suppose TestCompletionCallback resets its callback? > > We should use a different callback here - This should work, but seems a bad > idea. If it were to be called as a result of the Write() call, we could > incorrectly pass the test (Admittedly, if Initialize() called it twice or > something, would have the same issue, but we generally don't worry about that). > Using a different callback also lets us check that the one passed to Write is > never invoked as a result of the Write call. Done.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xunjieli@chromium.org changed reviewers: + bengr@chromium.org
bengr@chromium.org: Please review changes in components/precache/core/precache_fetcher.cc Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://chromiumcodereview.appspot.com/2425673006/diff/100001/net/url_request... File net/url_request/url_fetcher_response_writer_unittest.cc (right): https://chromiumcodereview.appspot.com/2425673006/diff/100001/net/url_request... net/url_request/url_fetcher_response_writer_unittest.cc:145: EXPECT_THAT(callback2.GetResult(rv), IsOk()); I don't think we want the callback to be invoked here (i.e., Finish() with an error should abort any other pending callback)
libaddressinput lgtm
https://chromiumcodereview.appspot.com/2425673006/diff/100001/net/url_request... File net/url_request/url_fetcher_response_writer.h (right): https://chromiumcodereview.appspot.com/2425673006/diff/100001/net/url_request... net/url_request/url_fetcher_response_writer.h:49: // graceful shutdown. If ERR_IO_PENDING is returned, |callback| will be run Also, think we should be more explicit that if net_error is not OK, this may be called in the middle of another operation (We could actually wait for the write to complete instead, but that gets complicated)
devtools lgtm
Patchset #6 (id:120001) has been deleted
xunjieli@chromium.org changed reviewers: + mkwst@chromium.org
Thanks! +mkwst@ for content/shell/browser/shell_devtools_frontend.cc https://codereview.chromium.org/2425673006/diff/100001/net/url_request/url_fe... File net/url_request/url_fetcher_response_writer.h (right): https://codereview.chromium.org/2425673006/diff/100001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer.h:49: // graceful shutdown. If ERR_IO_PENDING is returned, |callback| will be run On 2016/10/19 14:32:29, mmenke wrote: > Also, think we should be more explicit that if net_error is not OK, this may be > called in the middle of another operation (We could actually wait for the write > to complete instead, but that gets complicated) Done. https://codereview.chromium.org/2425673006/diff/100001/net/url_request/url_fe... File net/url_request/url_fetcher_response_writer_unittest.cc (right): https://codereview.chromium.org/2425673006/diff/100001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer_unittest.cc:145: EXPECT_THAT(callback2.GetResult(rv), IsOk()); On 2016/10/19 14:18:40, mmenke wrote: > I don't think we want the callback to be invoked here (i.e., Finish() with an > error should abort any other pending callback) Done. Changed implementation to cancel pending callback.
components/precache/core/precache_fetcher.cc lgtm
On 2016/10/19 16:07:52, bengr wrote: > components/precache/core/precache_fetcher.cc lgtm It suddenly occurs to me that some of the other subclasses may have trouble with this case, want to audit all of them before signing off on this (Though fixing them may be beyond the scope of this CL, if there are any such issues)
On 2016/10/19 16:07:52, bengr wrote: > components/precache/core/precache_fetcher.cc lgtm It suddenly occurs to me that some of the other subclasses may have trouble with this case, want to audit all of them before signing off on this (Though fixing them may be beyond the scope of this CL, if there are any such issues)
Other subclasses all look fine, noticed a problem, though. https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... File net/url_request/url_fetcher_response_writer.cc (right): https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer.cc:69: file_stream_.reset(new FileStream(file_task_runner_)); DCHECK(!callback_);? Same for all of these, except Finish() https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer.cc:108: base::Bind(&URLFetcherFileWriter::OnIOCompleted, nit: Line too long, run git cl format. https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer.cc:128: CloseAndDeleteFile(); BUG: If Finish() is called with an error when there's a pending operation, and then Init is called before that operation completes, the completion of that pending operation will cause the Callback() passed to Initialize to be invoked before Initialize has been completed. Not sure if we can reasonable write a test for this. Regardless, should fix this issue. I think the simplest fix is to just release all weak ptrs the factory has? I don't think that will get us into trouble. Does mean we don't need callback_ anymore, I think, though I'm fine with keeping it (Or getting rid of it, no strong opinion). https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer.cc:130: } DCHECK(!callback_); https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... File net/url_request/url_fetcher_response_writer.h (right): https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer.h:50: // |net_error| is not OK, graceful shutdown might be skipped. If nit: Maybe reword the 2nd and third sentences? "On errors (|net_error| not net::OK), this method may be called before the previous operation completed. In this case, URLFetcherResponseWriter may skip graceful shutdown and completion of the pending operation. After such a failure, the ResponseWriter may be reused.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review! PTAL https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... File net/url_request/url_fetcher_response_writer.cc (right): https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer.cc:69: file_stream_.reset(new FileStream(file_task_runner_)); On 2016/10/19 20:49:34, mmenke wrote: > DCHECK(!callback_);? Same for all of these, except Finish() Done. https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer.cc:108: base::Bind(&URLFetcherFileWriter::OnIOCompleted, On 2016/10/19 20:49:34, mmenke wrote: > nit: Line too long, run git cl format. Done. https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer.cc:128: CloseAndDeleteFile(); On 2016/10/19 20:49:34, mmenke wrote: > BUG: If Finish() is called with an error when there's a pending operation, and > then Init is called before that operation completes, the completion of that > pending operation will cause the Callback() passed to Initialize to be invoked > before Initialize has been completed. > > Not sure if we can reasonable write a test for this. > > Regardless, should fix this issue. I think the simplest fix is to just release > all weak ptrs the factory has? I don't think that will get us into trouble. > Does mean we don't need callback_ anymore, I think, though I'm fine with keeping > it (Or getting rid of it, no strong opinion). Done. I haven't thought of that. That indeed can happen with the current setup. Thanks for catching that! https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer.cc:130: } On 2016/10/19 20:49:34, mmenke wrote: > DCHECK(!callback_); Done. https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... File net/url_request/url_fetcher_response_writer.h (right): https://codereview.chromium.org/2425673006/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_response_writer.h:50: // |net_error| is not OK, graceful shutdown might be skipped. If On 2016/10/19 20:49:34, mmenke wrote: > nit: Maybe reword the 2nd and third sentences? > > "On errors (|net_error| not net::OK), this method may be called before the > previous operation completed. In this case, URLFetcherResponseWriter may skip > graceful shutdown and completion of the pending operation. After such a failure, > the ResponseWriter may be reused. Done.
LGTM, thanks for investigating and fixing this!
On 2016/10/20 16:21:39, mmenke wrote: > LGTM, thanks for investigating and fixing this! Thank you. +mkwst, hashimoto@: I am TBR-ing you since the change in files other than net/ are mechanical.
Description was changed from ========== Make URLFetcherFileWriter::Finish() skip closing file when there is an error This CL makes URLFetcherFileWriter::Finish() skip closing file when there is an error. If there is an error, Finish() will delete the file after any pending operation completes. BUG=487732, 656692 ========== to ========== Make URLFetcherFileWriter::Finish() skip closing file when there is an error This CL makes URLFetcherFileWriter::Finish() skip closing file when there is an error. If there is an error, Finish() will delete the file after any pending operation completes. TBR=hashimoto@chromium.org TBR=mkwst@chromium.org BUG=487732, 656692 ==========
The CQ bit was unchecked by xunjieli@chromium.org
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, dgozman@chromium.org, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2425673006/#ps180001 (title: "Fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make URLFetcherFileWriter::Finish() skip closing file when there is an error This CL makes URLFetcherFileWriter::Finish() skip closing file when there is an error. If there is an error, Finish() will delete the file after any pending operation completes. TBR=hashimoto@chromium.org TBR=mkwst@chromium.org BUG=487732, 656692 ========== to ========== Make URLFetcherFileWriter::Finish() skip closing file when there is an error This CL makes URLFetcherFileWriter::Finish() skip closing file when there is an error. If there is an error, Finish() will delete the file after any pending operation completes. TBR=hashimoto@chromium.org TBR=mkwst@chromium.org BUG=487732, 656692 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Make URLFetcherFileWriter::Finish() skip closing file when there is an error This CL makes URLFetcherFileWriter::Finish() skip closing file when there is an error. If there is an error, Finish() will delete the file after any pending operation completes. TBR=hashimoto@chromium.org TBR=mkwst@chromium.org BUG=487732, 656692 ========== to ========== Make URLFetcherFileWriter::Finish() skip closing file when there is an error This CL makes URLFetcherFileWriter::Finish() skip closing file when there is an error. If there is an error, Finish() will delete the file after any pending operation completes. TBR=hashimoto@chromium.org TBR=mkwst@chromium.org BUG=487732, 656692 Committed: https://crrev.com/ee63375d9c88e55c832cdf7f6181eb3ffc776695 Cr-Commit-Position: refs/heads/master@{#426540} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ee63375d9c88e55c832cdf7f6181eb3ffc776695 Cr-Commit-Position: refs/heads/master@{#426540} |
