Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
82273002:
Fix various issues in RedirectToFileResourceHandler. (Closed)
|
Created:
7 years, 1 month ago by davidben Modified:
6 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix various issues in RedirectToFileResourceHandler.
- Handle errors in creating temporary files.
- Cancel on write failure instead of resuming. This used to work, but got
lost in some refactoring in r142108.
- Fix the OnResponseCompleted resume logic to account for partial writes.
- Don't lose write errors which occur after OnResponseCompleted is received.
- WeakPtrFactory goes after other members.
- OnWillStart was never forwarded to downstream handlers.
- Make sure the file is closed before deleting it.
Also add a lot of machinery so we can better unit test this stack.
BUG=316634, 347663
TEST=ResourceDispatcherHostTest.DownloadToFile
ResourceDispatcherHostTest.RegisterDownloadedTempFile
ResourceLoaderTest.RedirectToFile*
TemporaryFileStreamTest.Basic
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256688
Patch Set 1 #Patch Set 2 : Check for errors in creating the temporary file. #Patch Set 3 : Add a RDH-level test. Also fix two more nuisances. #
Total comments: 1
Patch Set 4 : UUUUNIT TESTS #
Total comments: 1
Patch Set 5 : Appease clang #Patch Set 6 : Further appease clang #Patch Set 7 : Fix content_browsertests. #Patch Set 8 : Close the file before deleting it. #Patch Set 9 : Fix ResourceDispatcherHostTest.DownloadToFile #Patch Set 10 : Comments #
Total comments: 41
Patch Set 11 : Various #
Total comments: 45
Patch Set 12 : Various comments #
Total comments: 2
Patch Set 13 : Comment #
Total comments: 10
Patch Set 14 : Rebase, move a lot of code around. #Patch Set 15 : Fix test on Windows. #Patch Set 16 : Rebase #
Total comments: 17
Patch Set 17 : Rebase, address mmenke comments. #
Total comments: 16
Patch Set 18 : Rebase #Patch Set 19 : mmenke comments #
Total comments: 45
Patch Set 20 : mmenke comments #
Total comments: 2
Patch Set 21 : Revise comment #Patch Set 22 : Rebase #Patch Set 23 : Code duplication from rebase. #
Total comments: 6
Patch Set 24 : Re-use ResourceLoaderTest machinery. #
Total comments: 2
Patch Set 25 : Rename test fixture #Messages
Total messages: 51 (0 generated)
This is to fix some of the local issues in RedirectToFileResourceHandler. The more general cancel/resume issues in the bug probably want to be fixed in the LayeredResourceHandler base class or something, and I haven't tackled them here.
On 2013/11/22 00:24:52, David Benjamin wrote: > This is to fix some of the local issues in RedirectToFileResourceHandler. The > more general cancel/resume issues in the bug probably want to be fixed in the > LayeredResourceHandler base class or something, and I haven't tackled them here. I don't suppose we can unit test any of this? I suspect that without heavy mocking out of file IO, we can't, but it would be great if we could make a file openable but not writeable or something.
Added another bit of missing error-handling. Yeah, I wouldn't mind some tests here. (And on resource handlers in general.) Maybe if we could mock out the whole CreateTemporary -> FileStream dance and get a MockFileStream in. Except there's a call to RegisterDownloadedTempFile on the RDH.
On 2013/11/22 17:28:37, David Benjamin wrote: > Added another bit of missing error-handling. > > Yeah, I wouldn't mind some tests here. (And on resource handlers in general.) > Maybe if we could mock out the whole CreateTemporary -> FileStream dance and get > a MockFileStream in. Except there's a call to RegisterDownloadedTempFile on the > RDH. Sorry, forgot about this CL. I'll try to get to it later today. If not, tomorrow.
On 2013/11/25 19:08:51, mmenke wrote: > On 2013/11/22 17:28:37, David Benjamin wrote: > > Added another bit of missing error-handling. > > > > Yeah, I wouldn't mind some tests here. (And on resource handlers in general.) > > Maybe if we could mock out the whole CreateTemporary -> FileStream dance and > get > > a MockFileStream in. Except there's a call to RegisterDownloadedTempFile on > the > > RDH. > > Sorry, forgot about this CL. I'll try to get to it later today. If not, > tomorrow. Eh, I'm working on mocking out the CreateTemporary and RDH dependencies into a separate interface for testing anyway. Seems a reasonable plan, although it does add a bit of machinery just for this one resource handler.
On 2013/11/25 20:23:24, David Benjamin wrote: > On 2013/11/25 19:08:51, mmenke wrote: > > On 2013/11/22 17:28:37, David Benjamin wrote: > > > Added another bit of missing error-handling. > > > > > > Yeah, I wouldn't mind some tests here. (And on resource handlers in > general.) > > > Maybe if we could mock out the whole CreateTemporary -> FileStream dance and > > get > > > a MockFileStream in. Except there's a call to RegisterDownloadedTempFile on > > the > > > RDH. > > > > Sorry, forgot about this CL. I'll try to get to it later today. If not, > > tomorrow. > > Eh, I'm working on mocking out the CreateTemporary and RDH dependencies into a > separate interface for testing anyway. Seems a reasonable plan, although it does > add a bit of machinery just for this one resource handler. Right, you were working on tests... I knew that! I was...umm...just making sure you did?
On 2013/11/25 20:26:20, mmenke wrote: > On 2013/11/25 20:23:24, David Benjamin wrote: > > On 2013/11/25 19:08:51, mmenke wrote: > > > On 2013/11/22 17:28:37, David Benjamin wrote: > > > > Added another bit of missing error-handling. > > > > > > > > Yeah, I wouldn't mind some tests here. (And on resource handlers in > > general.) > > > > Maybe if we could mock out the whole CreateTemporary -> FileStream dance > and > > > get > > > > a MockFileStream in. Except there's a call to RegisterDownloadedTempFile > on > > > the > > > > RDH. > > > > > > Sorry, forgot about this CL. I'll try to get to it later today. If not, > > > tomorrow. > > > > Eh, I'm working on mocking out the CreateTemporary and RDH dependencies into a > > separate interface for testing anyway. Seems a reasonable plan, although it > does > > add a bit of machinery just for this one resource handler. > > Right, you were working on tests... I knew that! I was...umm...just making > sure you did? Added the first test. I've got a lot of unuploaded machinery for lower level tests (the mocking stuff out above) that I'll update the CL with later, but this is a start. The test gives us (a) some actual test coverage for RedirectToFileResourceHandler and (b) a (fail-flaky) test for the OnResponseCompleted resumption fix.
Alright, here is a LOT of machinery for unit tests. I split out the RDH dependency into a separate interface which, in unit tests, lets us inject errors at various points. MockFileStream also got a bit of work. It no longer calls callbacks after forcing an error and can both synchronously and asynchronously error, where relevant. It also can throttle the callback. https://codereview.chromium.org/82273002/diff/200001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/200001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:266: weak_factory_.GetWeakPtr())); Hrm. Realized this one's actually fine because the callback is cancelled when the file_stream_ is destroyed. https://codereview.chromium.org/82273002/diff/220001/content/browser/loader/r... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/220001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:130: return false; These weren't called by the existing tests and aren't called when behind a RedirectToFileResourceHandler either. Returning true didn't work anyway since we have to supply a buffer. Leaving this with a NOTREACHED() until a future test has need of it.
This will take me a while to work through. May not get back to you until after the break.
On 2013/11/27 15:39:16, mmenke wrote: > This will take me a while to work through. May not get back to you until after > the break. And a failure on win_rel. The file's not getting deleted when it should. Current theory is that this has to do with Windows not letting you delete files that are open. I think RedirectToFileResourceHandler's destructor or so needs to detach the net::FileStream and webkit_blob::ShareableFileReference into an object that waits for the asynchronous FileStream::Close to return, otherwise you can't actually be sure it's gone. I'll do that on Monday.
Bah, this CL just keeps getting bigger. Hopefully this resolves the close + delete issue on win_rel. I've split off a separate Writer object from the resource handler so it can outlive the handler just long enough to close the file before releasing the ShareableFileReference. It's a little silly that FileStream itself already is split like this, but taking advantage of that split means we don't get to know when the file closes.
On 2013/12/02 19:39:25, David Benjamin wrote: > Bah, this CL just keeps getting bigger. Hopefully this resolves the close + > delete issue on win_rel. I've split off a separate Writer object from the > resource handler so it can outlive the handler just long enough to close the > file before releasing the ShareableFileReference. It's a little silly that > FileStream itself already is split like this, but taking advantage of that split > means we don't get to know when the file closes. I'll try to get you some comments today, but could well end up being tomorrow before I understand the CL well enough to comment.
On 2013/12/03 16:49:04, mmenke wrote: > On 2013/12/02 19:39:25, David Benjamin wrote: > > Bah, this CL just keeps getting bigger. Hopefully this resolves the close + > > delete issue on win_rel. I've split off a separate Writer object from the > > resource handler so it can outlive the handler just long enough to close the > > file before releasing the ShareableFileReference. It's a little silly that > > FileStream itself already is split like this, but taking advantage of that > split > > means we don't get to know when the file closes. > > I'll try to get you some comments today, but could well end up being tomorrow > before I understand the CL well enough to comment. Can we add a closed form description of what these classes actually do? Neither the old classes nor the new classes really describe what's going on, and figuring out just how the "DELETE_ON_FINAL_RELEASE" behavior fits into the scheme of things, or even just what the heck this is all used for, seems to require reading a rather large chunk of code, some of which does not even run in the browser process.
Added some block comments at the top of each class. Is this clearer?
Still reviewing (Yes, I'm slow). https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:56: // |file_stream_->Write()| completes. Could we just make net::FileStream take a SequencedTaskRunner instead of a TaskRunner? Seems like a more general solution to close/delete funkiness. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:244: } else if (!defer) { Should probably have a test where defer is true. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:31: class TemporaryFileStreamFactory { This should be an inner class for RedirectToFileResourceHandler https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:67: TemporaryFileStreamFactory* file_stream_factory); Should mention lifetime requirements here. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:764: scoped_ptr<base::RunLoop> wait_for_request_complete_; This name makes me expect a bool. Maybe just wait_for_complete_run_loop_? https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:82: ResourceHandlerStub(net::URLRequest* request) explicit https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:98: uint64 size) OVERRIDE { NOTREACHED()? https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:105: bool* defer) OVERRIDE { NOTREACHED()? https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:112: DCHECK(!response_); If we want these to trigger in release builds (Which is probably a good idea, since trybots don't run debug tests), should make these CHECKs or EXPECTs. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:244: ASSERT_TRUE(file_util::CreateTemporaryFile(&file_path)); Believe this is now in the base namespace. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/t... File content/browser/loader/temporary_file_manager.cc (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/t... content/browser/loader/temporary_file_manager.cc:31: bool cancelled; nit: Methods go before member variables. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/t... File content/browser/loader/temporary_file_manager.h (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/t... content/browser/loader/temporary_file_manager.h:69: // CreateTemporary is in flight. optional: May be worth noting that if we have yet to call CreateTemporary for a RedirectRoFileResourceHandler, the request will be safely cancelled before it has the chance to call into CreateTemporary. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... File net/base/mock_file_stream.cc (right): https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.cc:93: weak_factory_.GetWeakPtr(), Does this weak factory do any good? FileStream::Context seems to take care of the orphaned case. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.cc:96: return FileStream::Write(buf, buf_len, wrapped_callback); Maybe: int rv = FileStream::Write(buf, buf_len, wrapped_callback); EXPECT_EQ(ERR_IO_PENDING, rv); return rv; Otherwise, throttling won't work, unless we're returning async errors. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.cc:122: DCHECK(!throttled_); These should probably be EXPECTs or CHECKs, so tryjobs will fail. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.cc:164: } DCHECK(!throttled_)? https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.cc:177: } DCHECK(!throttled_)? https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_stream.h File net/base/mock_file_stream.h (right): https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.h:25: MockFileStream(net::NetLog* net_log); While you're here, this should be explicit. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.h:54: void set_forced_error(int error) { forced_error_ = error; } This should probably set async_error_ to false. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.h:63: void ReleaseCallbacks(); These really need comments. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.h:90: int64 ErrorCallback64(const Int64CompletionCallback& callback); Think these could use some comments.
Also got rid of pending_temp_files_ in TemporaryFileManager. Made the interface more explicitly a Delegate that calls a particular method on a base::WeakPtr<RedirectToFileResourceHandler>. As long as the handler goes away before UnregisterFilesForChild, we don't have this nuisance. (I.e. this is now back to how it was before the split.) https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:56: // |file_stream_->Write()| completes. On 2013/12/04 20:55:48, mmenke wrote: > Could we just make net::FileStream take a SequencedTaskRunner instead of a > TaskRunner? Seems like a more general solution to close/delete funkiness. I thought about that, but I don't think it quite works? That assumes that, when you destroy the net::FileStream, all tasks needed to Close the PlatformFile have been posted already, so that you can then enqueue a DeleteFile after it. If the RedirectToFileResourceHandler is destroyed with a Write pending, it first waits for the Write to finish before it posts the Close. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:244: } else if (!defer) { On 2013/12/04 20:55:48, mmenke wrote: > Should probably have a test where defer is true. Done. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:31: class TemporaryFileStreamFactory { On 2013/12/04 20:55:48, mmenke wrote: > This should be an inner class for RedirectToFileResourceHandler Done. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:67: TemporaryFileStreamFactory* file_stream_factory); On 2013/12/04 20:55:48, mmenke wrote: > Should mention lifetime requirements here. Done. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:764: scoped_ptr<base::RunLoop> wait_for_request_complete_; On 2013/12/04 20:55:48, mmenke wrote: > This name makes me expect a bool. Maybe just wait_for_complete_run_loop_? Done. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:82: ResourceHandlerStub(net::URLRequest* request) On 2013/12/04 20:55:48, mmenke wrote: > explicit Done. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:98: uint64 size) OVERRIDE { On 2013/12/04 20:55:48, mmenke wrote: > NOTREACHED()? Done. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:105: bool* defer) OVERRIDE { On 2013/12/04 20:55:48, mmenke wrote: > NOTREACHED()? Done. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:112: DCHECK(!response_); On 2013/12/04 20:55:48, mmenke wrote: > If we want these to trigger in release builds (Which is probably a good idea, > since trybots don't run debug tests), should make these CHECKs or EXPECTs. Done. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:244: ASSERT_TRUE(file_util::CreateTemporaryFile(&file_path)); On 2013/12/04 20:55:48, mmenke wrote: > Believe this is now in the base namespace. Done. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/t... File content/browser/loader/temporary_file_manager.cc (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/t... content/browser/loader/temporary_file_manager.cc:31: bool cancelled; On 2013/12/04 20:55:48, mmenke wrote: > nit: Methods go before member variables. Done. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/t... File content/browser/loader/temporary_file_manager.h (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/t... content/browser/loader/temporary_file_manager.h:69: // CreateTemporary is in flight. On 2013/12/04 20:55:48, mmenke wrote: > optional: May be worth noting that if we have yet to call CreateTemporary for a > RedirectRoFileResourceHandler, the request will be safely cancelled before it > has the chance to call into CreateTemporary. Done. Also reordered DetachableResourceHandler and RedirectToFileResourceHandler so that's actually true. :-) Not that you'll ever have download_to_file with a resource type of PING or PREFETCH. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... File net/base/mock_file_stream.cc (right): https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.cc:93: weak_factory_.GetWeakPtr(), On 2013/12/04 20:55:48, mmenke wrote: > Does this weak factory do any good? FileStream::Context seems to take care of > the orphaned case. If the wrapped callback gets called in the ErrorCallback case, the callback needs to get invalidated. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.cc:96: return FileStream::Write(buf, buf_len, wrapped_callback); On 2013/12/04 20:55:48, mmenke wrote: > Maybe: > > int rv = FileStream::Write(buf, buf_len, wrapped_callback); > EXPECT_EQ(ERR_IO_PENDING, rv); > return rv; > > Otherwise, throttling won't work, unless we're returning async errors. Hrm, I'd been thinking that it wouldn't do anything on calls that happened to return synchronously, but that's probably never actually what you want. I dunno, piles of boilerplate. It certainly wont' do anything on the calls that actually are synchronous. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.cc:122: DCHECK(!throttled_); On 2013/12/04 20:55:48, mmenke wrote: > These should probably be EXPECTs or CHECKs, so tryjobs will fail. DCHECKs really don't run on try bots? That seems kinda silly. Done. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_stream.h File net/base/mock_file_stream.h (right): https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.h:25: MockFileStream(net::NetLog* net_log); On 2013/12/04 20:55:48, mmenke wrote: > While you're here, this should be explicit. Done. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.h:54: void set_forced_error(int error) { forced_error_ = error; } On 2013/12/04 20:55:48, mmenke wrote: > This should probably set async_error_ to false. Done. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.h:63: void ReleaseCallbacks(); On 2013/12/04 20:55:48, mmenke wrote: > These really need comments. Done. https://codereview.chromium.org/82273002/diff/340001/net/base/mock_file_strea... net/base/mock_file_stream.h:90: int64 ErrorCallback64(const Int64CompletionCallback& callback); On 2013/12/04 20:55:48, mmenke wrote: > Think these could use some comments. Done.
On 2013/12/04 22:44:03, David Benjamin wrote: > Also got rid of pending_temp_files_ in TemporaryFileManager. Made the interface > more explicitly a Delegate that calls a particular method on a > base::WeakPtr<RedirectToFileResourceHandler>. As long as the handler goes away > before UnregisterFilesForChild, we don't have this nuisance. (I.e. this is now > back to how it was before the split.) I'm not following your last sentence.
On 2013/12/05 17:05:30, mmenke wrote: > On 2013/12/04 22:44:03, David Benjamin wrote: > > Also got rid of pending_temp_files_ in TemporaryFileManager. Made the > interface > > more explicitly a Delegate that calls a particular method on a > > base::WeakPtr<RedirectToFileResourceHandler>. As long as the handler goes away > > before UnregisterFilesForChild, we don't have this nuisance. (I.e. this is now > > back to how it was before the split.) > > I'm not following your last sentence. Moving part of the callbacks to TemporaryFileManager instead of RedirectToFileResourceHandler meant that they didn't get cancelled when the resource handler went away, so there was the possibility of a child dying while it had a CreateTemporary in flight, leaking an entry in registered_temp_files_. Now it can check the WeakPtr, which is admittedly a little weird, but avoids that race in the same way the old code does. (The resource handlers are destroyed before UnregisterFilesForChild.)
On 2013/12/05 17:34:31, David Benjamin wrote: > On 2013/12/05 17:05:30, mmenke wrote: > > On 2013/12/04 22:44:03, David Benjamin wrote: > > > Also got rid of pending_temp_files_ in TemporaryFileManager. Made the > > interface > > > more explicitly a Delegate that calls a particular method on a > > > base::WeakPtr<RedirectToFileResourceHandler>. As long as the handler goes > away > > > before UnregisterFilesForChild, we don't have this nuisance. (I.e. this is > now > > > back to how it was before the split.) > > > > I'm not following your last sentence. > > Moving part of the callbacks to TemporaryFileManager instead of > RedirectToFileResourceHandler meant that they didn't get cancelled when the > resource handler went away, so there was the possibility of a child dying while > it had a CreateTemporary in flight, leaking an entry in registered_temp_files_. > Now it can check the WeakPtr, which is admittedly a little weird, but avoids > that race in the same way the old code does. (The resource handlers are > destroyed before UnregisterFilesForChild.) Thanks for the explanation! I'll get you more comments (Hopefully my last set) later this afternoon.
Not quite finished, just want to look through the unit tests. https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/t... File content/browser/loader/temporary_file_manager.h (right): https://codereview.chromium.org/82273002/diff/340001/content/browser/loader/t... content/browser/loader/temporary_file_manager.h:69: // CreateTemporary is in flight. On 2013/12/04 22:44:04, David Benjamin wrote: > On 2013/12/04 20:55:48, mmenke wrote: > > optional: May be worth noting that if we have yet to call CreateTemporary for > a > > RedirectRoFileResourceHandler, the request will be safely cancelled before it > > has the chance to call into CreateTemporary. > > Done. Also reordered DetachableResourceHandler and RedirectToFileResourceHandler > so that's actually true. :-) Not that you'll ever have download_to_file with a > resource type of PING or PREFETCH. I was actually thinking about the destroyed before it even starts to create a file case... I like your interpretation much better, though. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:56: // |file_stream_->Write()| completes. Suggest modifying the second sentence to make it clear that the Writer does the closing (And the writing, though that's kinda expected of the Writer), and the ShareableFileReference does the deleting. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:71: int Write(net::IOBuffer* buf, int buf_len) { DCHECK(!write_callback_pending_)? DCHECK(handler_)? https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:87: void DidWriteToFile(int result) { DCHECK(write_callback_pending_)? https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:96: void CloseAndDelete() { DCHECK(!write_callback_pending_)? https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:146: DCHECK(writer_ && !writer_->path().empty()); Suggest splitting this into two DCHECKs, while you're here. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:36: class CONTENT_EXPORT RedirectToFileResourceHandler Should include content_export.h https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:39: typedef base::Callback< Should include callback.h or callback_forward.h. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:43: CreateTemporaryCallback; I'm unable to find anything that's quite this ugly to model this off of, but I suggest: typedef base::Callback< void(base::PlatformFileError, scoped_ptr<net::FileStream>, scoped_refptr<webkit_blob::ShareableFileReference>)> CreateTemporaryCallback; As being slightly easier to read. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:45: nit: Remove extra blank line. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:94: void DidWriteToFile(int result); This can be private, since inner classes are friends of their parent classes (But not the other way around). https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:142: DISALLOW_COPY_AND_ASSIGN(RedirectToFileResourceHandler); While you're fixing this file, should add includes for compiler_specific.h and basictypes.h https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:547: DISALLOW_COPY_AND_ASSIGN(ShareableFileReferenceWaiter); include basictypes https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:315: test_url_request_context_.set_job_factory(&job_factory_); Optional: Can't we just do this in the constructor? https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:319: } Don't think we need this. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:500: EXPECT_FALSE(base::PathExists(temp_path)); Still a bit concerned about testing cleanup, and we've mocked out too much in these tests to really test that. This CL is big enough that I don't want to go more aggressive https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:539: // Setup the request. nit: "setup" is a noun, "set up" is a verb. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... File content/browser/loader/temporary_file_manager.cc (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... content/browser/loader/temporary_file_manager.cc:27: struct TemporaryFileManager::PendingTemporary { optional: Do we even need this class? Seems like you can just pass them all directly now. Doesn't really save us a whole lot, of course. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... content/browser/loader/temporary_file_manager.cc:54: } Can't all of this just be replaced with: registered_temp_files_[child_id].erase(request_id)? https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... content/browser/loader/temporary_file_manager.cc:127: // loaded with to keep the file (and permissions) alive. nit: Don't use "we" in comments. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... File content/browser/loader/temporary_file_manager.h (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... content/browser/loader/temporary_file_manager.h:29: } This function isn't needed, is it? It's just called TemporaryFileManager::CreateTemporary, so can be inlined. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... content/browser/loader/temporary_file_manager.h:45: base::WeakPtr<RedirectToFileResourceHandler> handler) OVERRIDE; include compiler_specific for override.
https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:56: // |file_stream_->Write()| completes. On 2013/12/05 21:34:03, mmenke wrote: > Suggest modifying the second sentence to make it clear that the Writer does the > closing (And the writing, though that's kinda expected of the Writer), and the > ShareableFileReference does the deleting. Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:71: int Write(net::IOBuffer* buf, int buf_len) { On 2013/12/05 21:34:03, mmenke wrote: > DCHECK(!write_callback_pending_)? > DCHECK(handler_)? Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:87: void DidWriteToFile(int result) { On 2013/12/05 21:34:03, mmenke wrote: > DCHECK(write_callback_pending_)? Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:96: void CloseAndDelete() { On 2013/12/05 21:34:03, mmenke wrote: > DCHECK(!write_callback_pending_)? Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:146: DCHECK(writer_ && !writer_->path().empty()); On 2013/12/05 21:34:03, mmenke wrote: > Suggest splitting this into two DCHECKs, while you're here. Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:36: class CONTENT_EXPORT RedirectToFileResourceHandler On 2013/12/05 21:34:03, mmenke wrote: > Should include content_export.h Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:39: typedef base::Callback< On 2013/12/05 21:34:03, mmenke wrote: > Should include callback.h or callback_forward.h. It's already there, but... https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:43: CreateTemporaryCallback; On 2013/12/05 21:34:03, mmenke wrote: > I'm unable to find anything that's quite this ugly to model this off of, but I > suggest: > > typedef base::Callback< > void(base::PlatformFileError, > scoped_ptr<net::FileStream>, > scoped_refptr<webkit_blob::ShareableFileReference>)> > CreateTemporaryCallback; > > As being slightly easier to read. Actually deleting it altogether. It's gone now and I forgot to remove the typedef. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:45: On 2013/12/05 21:34:03, mmenke wrote: > nit: Remove extra blank line. Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:94: void DidWriteToFile(int result); On 2013/12/05 21:34:03, mmenke wrote: > This can be private, since inner classes are friends of their parent classes > (But not the other way around). Done. Huh, I didn't actually know that. Handy. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:142: DISALLOW_COPY_AND_ASSIGN(RedirectToFileResourceHandler); On 2013/12/05 21:34:03, mmenke wrote: > While you're fixing this file, should add includes for compiler_specific.h and > basictypes.h Done. I kinda wonder if we should just have a single header included everywhere which pulls in compiler_specific.h and basictypes.h. They seem pretty common. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:547: DISALLOW_COPY_AND_ASSIGN(ShareableFileReferenceWaiter); On 2013/12/05 21:34:03, mmenke wrote: > include basictypes Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:315: test_url_request_context_.set_job_factory(&job_factory_); On 2013/12/05 21:34:03, mmenke wrote: > Optional: Can't we just do this in the constructor? Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:319: } On 2013/12/05 21:34:03, mmenke wrote: > Don't think we need this. Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:500: EXPECT_FALSE(base::PathExists(temp_path)); On 2013/12/05 21:34:03, mmenke wrote: > Still a bit concerned about testing cleanup, and we've mocked out too much in > these tests to really test that. This CL is big enough that I don't want to go > more aggressive Hrm? Do you mean you'd rather not assert on base::PathExists? https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:539: // Setup the request. On 2013/12/05 21:34:03, mmenke wrote: > nit: "setup" is a noun, "set up" is a verb. Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... File content/browser/loader/temporary_file_manager.cc (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... content/browser/loader/temporary_file_manager.cc:27: struct TemporaryFileManager::PendingTemporary { On 2013/12/05 21:34:03, mmenke wrote: > optional: Do we even need this class? Seems like you can just pass them all > directly now. Doesn't really save us a whole lot, of course. Done. I guess DidCreateTemporaryFile is now kinda huge, but oh well. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... content/browser/loader/temporary_file_manager.cc:54: } On 2013/12/05 21:34:03, mmenke wrote: > Can't all of this just be replaced with: > > registered_temp_files_[child_id].erase(request_id)? Hrm, yeah, it can... this just came from the original version. Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... content/browser/loader/temporary_file_manager.cc:127: // loaded with to keep the file (and permissions) alive. On 2013/12/05 21:34:03, mmenke wrote: > nit: Don't use "we" in comments. Also copied from the old file, but done. :-P https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... File content/browser/loader/temporary_file_manager.h (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... content/browser/loader/temporary_file_manager.h:29: } On 2013/12/05 21:34:03, mmenke wrote: > This function isn't needed, is it? It's just called > TemporaryFileManager::CreateTemporary, so can be inlined. Done. https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/t... content/browser/loader/temporary_file_manager.h:45: base::WeakPtr<RedirectToFileResourceHandler> handler) OVERRIDE; On 2013/12/05 21:34:03, mmenke wrote: > include compiler_specific for override. Done.
https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:500: EXPECT_FALSE(base::PathExists(temp_path)); On 2013/12/05 22:43:20, David Benjamin wrote: > On 2013/12/05 21:34:03, mmenke wrote: > > Still a bit concerned about testing cleanup, and we've mocked out too much in > > these tests to really test that. This CL is big enough that I don't want to > go > > more aggressive > > Hrm? Do you mean you'd rather not assert on base::PathExists? No, I'm fine with that. I mean I'm not sure asserting on base::PathExists serves as a good test that cleanup works in production code. Could add the assert to a couple of the error tests, but that doesn't really assuage my concerns about testing cleanup.
https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:500: EXPECT_FALSE(base::PathExists(temp_path)); On 2013/12/05 23:00:13, mmenke wrote: > On 2013/12/05 22:43:20, David Benjamin wrote: > > On 2013/12/05 21:34:03, mmenke wrote: > > > Still a bit concerned about testing cleanup, and we've mocked out too much > in > > > these tests to really test that. This CL is big enough that I don't want to > > go > > > more aggressive > > > > Hrm? Do you mean you'd rather not assert on base::PathExists? > > No, I'm fine with that. I mean I'm not sure asserting on base::PathExists > serves as a good test that cleanup works in production code. Could add the > assert to a couple of the error tests, but that doesn't really assuage my > concerns about testing cleanup. Ah. Yeah, it's not a terribly good test there. It does test that, in isolation, the handler will delete the file. ResourceDispatcherHostTest.DownloadToFile also has a similar assertion which is somewhat less badly mocked up.
LGTM, though perhaps I should just another 5 tests, just because this CL hasn't gone through enough revisions or grown enough yet. :) https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/360001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:142: DISALLOW_COPY_AND_ASSIGN(RedirectToFileResourceHandler); On 2013/12/05 22:43:20, David Benjamin wrote: > On 2013/12/05 21:34:03, mmenke wrote: > > While you're fixing this file, should add includes for compiler_specific.h and > > basictypes.h > > Done. I kinda wonder if we should just have a single header included everywhere > which pulls in compiler_specific.h and basictypes.h. They seem pretty common. Indeed. Even just having one include the other would make life simpler. https://codereview.chromium.org/82273002/diff/380001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/380001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:56: // stream is closed is the ShareableFileReference is released, to ensure the nit: "is the ShareableFileReference is released" -> "is the ShareableFileReference released"
+darin for content/ OWNERS and for content/browser/loader code. mmenke's already done the bulk of the review, but you may also want to take a look. This fixes issues within RedirectToFileResourceHandler. It doesn't address the more general Cancel/Resume races of bug #316634. I'll play with those after this one. (Original plan was to tidy up local issues within individual resource handlers first, and then we got a little carried away with this one, so the CL got kinda big.) https://codereview.chromium.org/82273002/diff/380001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/380001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:56: // stream is closed is the ShareableFileReference is released, to ensure the On 2013/12/06 16:15:39, mmenke wrote: > nit: "is the ShareableFileReference is released" -> "is the > ShareableFileReference released" Done.
https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:243: writer_.reset(new Writer(this, file_stream.Pass(), deletable_file)); It makes me a bit nervous seeing a pointer to Writer stored in a scoped_ptr when Writer::DidClose calls |delete this|. Perhaps it would be cleaner to make ~Writer private, just use a raw pointer for writer_, and then require that the only way to clean up a Writer instance is to call Orphan(). You might also give that a name like Close instead. https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:55: virtual void CreateTemporary( Perhaps this would be cleaner as a base::Callback specialization? Ditto for DidCreateTemporaryFile. At the very least it seems the relationship between TemporaryFileManager and RedirectToFileResourceHandler is inverted in this CL. TemporaryFileManager is the utility that RedirectToFileResourceHandler could consume. Other things like RedirectToFileResourceHandler might like to consume TemporaryFileManager too, but TemporaryFileManager is only useful to RedirectToFileResourceHandler. As constructed in this CL, TemporaryFileManager is an implementation detail of RedirectToFileResourceHandler. So, a variant on passing base::Callback<...> to RedirectToFileResourceHandler would be to make RedirectToFileResourceHandler depend directly on TemporaryFileManager and be passed an instance of it, but I would be fine with this being abstracted as well. https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:1163: // Prefetches and <a ping> requests outlive their child process. Is this part of a different CL? https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/t... File content/browser/loader/temporary_file_manager.cc (right): https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/t... content/browser/loader/temporary_file_manager.cc:48: base::WeakPtr<RedirectToFileResourceHandler> handler) { It would be nice from a unit-testing point of view to break this dependency from TemporaryFileManager on RedirectToFileResourceHandler. https://codereview.chromium.org/82273002/diff/400001/content/content_browser.... File content/content_browser.gypi (right): https://codereview.chromium.org/82273002/diff/400001/content/content_browser.... content/content_browser.gypi:700: 'browser/loader/temporary_file_manager.h', can you write a unit test for this? (should be possible after you break the dependency on RedirectToFileResourceHandler)
Sorry, I sat on this for a bit. Rebased the CL and shuffled some of the code around. The RDH is changed less now; instead the child process registration logic is moved to a downstream handler. The takes the child process logic out of TemporaryFileManager which now a single CreateTemporaryFileStream function with a unit test. RedirectToFileResourceHandler takes it as a base::Callback for unit testing purposes. https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:243: writer_.reset(new Writer(this, file_stream.Pass(), deletable_file)); On 2013/12/06 17:56:07, darin wrote: > It makes me a bit nervous seeing a pointer to Writer stored in a scoped_ptr when > Writer::DidClose calls |delete this|. > > Perhaps it would be cleaner to make ~Writer private, just use a raw pointer for > writer_, and then require that the only way to clean up a Writer instance is to > call Orphan(). You might also give that a name like Close instead. Done. https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:55: virtual void CreateTemporary( On 2013/12/06 17:56:07, darin wrote: > Perhaps this would be cleaner as a base::Callback specialization? Ditto for > DidCreateTemporaryFile. > > At the very least it seems the relationship between TemporaryFileManager and > RedirectToFileResourceHandler is inverted in this CL. TemporaryFileManager is > the utility that RedirectToFileResourceHandler could consume. Other things like > RedirectToFileResourceHandler might like to consume TemporaryFileManager too, > but TemporaryFileManager is only useful to RedirectToFileResourceHandler. As > constructed in this CL, TemporaryFileManager is an implementation detail of > RedirectToFileResourceHandler. > > So, a variant on passing base::Callback<...> to RedirectToFileResourceHandler > would be to make RedirectToFileResourceHandler depend directly on > TemporaryFileManager and be passed an instance of it, but I would be fine with > this being abstracted as well. Hrm. TemporaryFileManager's job is pretty unclear, yeah. It exists partially to remove the handler's dependency on the RDH and its child process logic and partially just to mock out temporary file and net::FileStream creation. I originally split it out of RDH just so the CreateTemporary call has something to bind the callback's lifetime to. I've reorganized it a bit so that the child process logic is back in RDH and AsyncResourceHandler and SyncResourceHandler call RegisterDownloadedTemp file instead of RedirectToFileResourceHandler. It's a little annoying with requiring the code in two places, but I think it makes more sense for it to be at the entry point to the child. RedirectToFileResourceHandler just downloads to a file, and it's the downstream handler's business what happens to the file. Granted I'm now just pushing RDH dependency from one handler to another, but less code shuffle. TemporaryFileManager is now just a single function and the RedirectToFileResourceHandler gets it as a callback so the unit tests can still inject errors and a MockFileStream. https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:1163: // Prefetches and <a ping> requests outlive their child process. On 2013/12/06 17:56:07, darin wrote: > Is this part of a different CL? I moved it out from the code above where AsyncResourceHandler is created. Noticed that it results in RedirectToFileResourceHandler not being next in the chain, so I swapped the order here. (Shouldn't ever happen, but if the renderer decides to create a download_to_file prefetch for some incomprehensible reason...) This way DetachableResourceHandler never gets its OnWillRead/OnReadCompleted preempted in favor of OnDataDownloaded and it preserves the behavior that RedirectToFileResourceHandler gets destroyed when the request is cancelled via CancelRequestsForRoute. https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/t... File content/browser/loader/temporary_file_manager.cc (right): https://codereview.chromium.org/82273002/diff/400001/content/browser/loader/t... content/browser/loader/temporary_file_manager.cc:48: base::WeakPtr<RedirectToFileResourceHandler> handler) { On 2013/12/06 17:56:07, darin wrote: > It would be nice from a unit-testing point of view to break this dependency from > TemporaryFileManager on RedirectToFileResourceHandler. Done. https://codereview.chromium.org/82273002/diff/400001/content/content_browser.... File content/content_browser.gypi (right): https://codereview.chromium.org/82273002/diff/400001/content/content_browser.... content/content_browser.gypi:700: 'browser/loader/temporary_file_manager.h', On 2013/12/06 17:56:07, darin wrote: > can you write a unit test for this? (should be possible after you break the > dependency on RedirectToFileResourceHandler) Done.
Friendly ping for revised patchset.
On 2014/01/10 21:31:37, David Benjamin wrote: > Friendly ping for revised patchset. I'll get back to you this week, but since it's a 1,000 line CL, and I've lost all context in the past month, may be a couple days.
Still need to get to the unit tests. https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/a... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/a... content/browser/loader/async_resource_handler.cc:206: } Sure you don't want to keep this in the RedirectToFileResourceHandler? I'm a little concerned about changes/fixes here not making it into the SyncResourceHandler as well. https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:230: if (writer_ && writer_->write_callback_pending()) { How can writer be NULL here? Cancellation while we're still trying to open the file? If that happens, will the temporary file be cleanup up properly? Doesn't look like it would. Instead, one option would be to have it always create the writer, and then check if the object still exists via the weak pointer. https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:275: if (failed) { Maybe throw in a "DCHECK(!writer_->write_callback_pending())" just to make it clearer what happens with the next handler in this case? https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:311: write_cursor_ = 0; Random comment: Is it just me, or does this seem weird/dangerous? We're relying on ResumeIfDeferred resulting in calling OnWillRead asynchronously. I could walk through the code to figure out the details of this dependency, but it's certainly not immediately obvious to me why we can rely on this. https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:44: CreateTemporaryCB; Google style guide strongly discourages uncommon abbreviations. Seems like "callback" is almost always written out. Also, I think this name is very confusing. A "CreateTemporaryFileStreamCallback" is what's called after the FileStream is created, but a "CreateTemporaryCB" is a callback that creates a FileStream. I think naming this just "CreateTemporaryFileStream" would be much clearer. https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:44: CreateTemporaryCB; I also think this should have a comment... Maybe something like "Creates a file stream and a ShareableFileReference, and then asynchronously passes them to CreateTemporaryFileStreamCallback." https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:85: CreateTemporaryCB create_temporary_cb_; const? https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/t... File content/browser/loader/temporary_file_stream.h (right): https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/t... content/browser/loader/temporary_file_stream.h:29: // net::FileStream and webkit_blob::ShareableFileReference The file is deleted nit: period after webkit_blob::ShareableFileReference https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/t... content/browser/loader/temporary_file_stream.h:33: // |callback| is called with an error in the first parameter. I don't think it's obvious that this method is not threadsafe (ShareableFileReference::GetOrCreate isn't threadsafe). Suggest a comment and DCHECKing that we're called on the IO thread.
https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/a... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/a... content/browser/loader/async_resource_handler.cc:206: } On 2014/01/16 16:43:26, mmenke wrote: > Sure you don't want to keep this in the RedirectToFileResourceHandler? I'm a > little concerned about changes/fixes here not making it into the > SyncResourceHandler as well. Well, keeping it in RedirectToFileResourceHandler keeps the RTFRH -> RDH dependency, which is what made it difficult to unit test in the first place. The previous version put it in the CreateTemporary implementation, but it seemed doing it right before sending the file path to the renderer was more appropriate. (Also this means the mocked out function doesn't need to tie back to the RDH and care about that thing's lifetime.) I've added a comment to the two copies. We may also be able to just remove SyncResourceHandler's download_to_file support altogether. I couldn't find code in Blink that cared about it. And I think the loadSynchronously codepaths never read from the blob handle renderer-side, so if it did happen, it wouldn't work. Added a TODO about getting rid of it. https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:230: if (writer_ && writer_->write_callback_pending()) { On 2014/01/16 16:43:26, mmenke wrote: > How can writer be NULL here? Cancellation while we're still trying to open the > file? If that happens, will the temporary file be cleanup up properly? Doesn't > look like it would. Instead, one option would be to have it always create the > writer, and then check if the object still exists via the weak pointer. Yeah, that's the NULL case. The temporary in this version will actually be deleted fine because the callback will be nullified, no one will take a reference to the ShareableFileReference, and the file will be deleted. (Actually, this was a problem with the old code; if DidCreateTemporaryFile failed to run because of the WeakPtr, the temporary file just got leaked.) https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:275: if (failed) { On 2014/01/16 16:43:26, mmenke wrote: > Maybe throw in a "DCHECK(!writer_->write_callback_pending())" just to make it > clearer what happens with the next handler in this case? Done. https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:311: write_cursor_ = 0; On 2014/01/16 16:43:26, mmenke wrote: > Random comment: Is it just me, or does this seem weird/dangerous? We're > relying on ResumeIfDeferred resulting in calling OnWillRead asynchronously. I > could walk through the code to figure out the details of this dependency, but > it's certainly not immediately obvious to me why we can rely on this. Hrm. Well, the assumption is true right now. I suppose anyone in the chain could override Resume to make it re-entrant, though that does seem a little rude. I think we can just reorder those lines. I'm going to do that separately because this CL is too big. https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:44: CreateTemporaryCB; On 2014/01/16 16:43:26, mmenke wrote: > I also think this should have a comment... Maybe something like "Creates a file > stream and a ShareableFileReference, and then asynchronously passes them to > CreateTemporaryFileStreamCallback." How's this version? https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:85: CreateTemporaryCB create_temporary_cb_; On 2014/01/16 16:43:26, mmenke wrote: > const? Pulled it out into a Set...ForTesting function. https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/t... File content/browser/loader/temporary_file_stream.h (right): https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/t... content/browser/loader/temporary_file_stream.h:29: // net::FileStream and webkit_blob::ShareableFileReference The file is deleted On 2014/01/16 16:43:26, mmenke wrote: > nit: period after webkit_blob::ShareableFileReference Done. https://codereview.chromium.org/82273002/diff/550001/content/browser/loader/t... content/browser/loader/temporary_file_stream.h:33: // |callback| is called with an error in the first parameter. On 2014/01/16 16:43:26, mmenke wrote: > I don't think it's obvious that this method is not threadsafe > (ShareableFileReference::GetOrCreate isn't threadsafe). Suggest a comment and > DCHECKing that we're called on the IO thread. Done.
Still need to carefully go over the tests. https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:164: DCHECK(!writer_->path().empty()); Random comment: Fine to leave as-is, but it's kinda weird to have this DCHECK here and not, say, where we actually create the writer [either also or instead]. https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:270: } Erm...Shouldn't did_defer_ also be set to false if defer is true? The request is still deferred, but it's no longer our responsibility to resume it, since they just call the controller directly. I suppose this also technically goes for the call in DidWriteToFile, though that one doesn't really matter. Hmm... Can this be reasonably unit tested? https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:296: controller()->CancelWithError(net::ERR_FAILED); Hmm... Should we really be cancelling with an error if completed_during_write_ is true? https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:120: GURL will_start_url_; Need to include the GURL header. https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:1213: return; Generally we shouldn't be handling NOTREACHED / DCHECK cases, so I think this block can just be a DCHECK(reference); https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:191: ChildProcessSecurityPolicyImpl::GetInstance()->Add(child_id()); What's this for? https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:570: class ShareableFileReferenceWaiter { Maybe ShareableFileReferenceReleaseWaiter? Or just add a comment. https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:629: virtual void SetUp() { Add OVERRIDE while you're here? Yea, this is a huge CL, but figure that's pretty trivial addition.
https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:164: DCHECK(!writer_->path().empty()); On 2014/02/14 21:52:49, mmenke wrote: > Random comment: Fine to leave as-is, but it's kinda weird to have this DCHECK > here and not, say, where we actually create the writer [either also or instead]. Done. https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:270: } On 2014/02/14 21:52:49, mmenke wrote: > Erm...Shouldn't did_defer_ also be set to false if defer is true? The request > is still deferred, but it's no longer our responsibility to resume it, since > they just call the controller directly. > > I suppose this also technically goes for the call in DidWriteToFile, though that > one doesn't really matter. > > Hmm... Can this be reasonably unit tested? Yeah, I don't think this ResumeIfDeferred() pattern actually works very well for deferrals outside OnWillStart. Really the whole chain with calling next_handler_->OnWillStart and the like is part of the resume process. I've just reset did_defer_ for now since this CL's already kinda big. Deferral, especially around OnResponseCompleted has lots of broken edge cases and we should probably think about fixing things in LayeredResourceHandler. (Maybe provide ResumeOnWillStart() functions and the like.) https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.cc:296: controller()->CancelWithError(net::ERR_FAILED); On 2014/02/14 21:52:49, mmenke wrote: > Hmm... Should we really be cancelling with an error if completed_during_write_ > is true? I don't think it especially matters? I've gone and guarded it with condition, but ResourceLoader will need to support canceling after the URLRequest is done anyway because ResponseCompleted gets called asynchronously after the cancel. https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/redirect_to_file_resource_handler.h:120: GURL will_start_url_; On 2014/02/14 21:52:49, mmenke wrote: > Need to include the GURL header. Done. https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:1213: return; On 2014/02/14 21:52:49, mmenke wrote: > Generally we shouldn't be handling NOTREACHED / DCHECK cases, so I think this > block can just be a DCHECK(reference); Done. https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:191: ChildProcessSecurityPolicyImpl::GetInstance()->Add(child_id()); On 2014/02/14 21:52:49, mmenke wrote: > What's this for? Making temporaries has to do things with ChildProcessSecurityPolicyImpl to mark files as readable by the child. It doesn't work if it doesn't know a child exists, and all these things are mocked. https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:570: class ShareableFileReferenceWaiter { On 2014/02/14 21:52:49, mmenke wrote: > Maybe ShareableFileReferenceReleaseWaiter? Or just add a comment. Done. https://codereview.chromium.org/82273002/diff/720001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:629: virtual void SetUp() { On 2014/02/14 21:52:49, mmenke wrote: > Add OVERRIDE while you're here? Yea, this is a huge CL, but figure that's > pretty trivial addition. Done.
LGTM https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:58: // temporary is not deleted before it is closed. May be worth mentioning this lives on the same thread as the RedirectToFileResourceHandler https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:71: bool write_callback_pending() const { return write_callback_pending_; } Optional: is_writing maybe be clearer. If you prefer this (Which is more accurate), feel free to keep it as-is. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:148: writer_ = NULL; Optional: Do we get anything out of NULLing this out? Close() shouldn't call into |this|...Suppose you could argue it's safer to NULL it out, just wondering because we're in the destructor, and it's a raw pointer. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:258: controller()->CancelWithError(net::FileErrorToNetError(error_code)); optional: Should we set did_defer_ to false here? It doesn't currently matter, but given the spaghetti nature of the behavior of this class, just seems like one more place that things could go wrong. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:272: did_defer_ = false; This still makes me nervous... Yes, if we call ResumeIfDeferred, OnResponseStarted *shouldn't* be synchronously called, and there's probably even at least one guarantee of it somewhere, possibly more... But I'd like not to depend on that. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:313: did_defer_ = false; Maybe put this in an else? We don't be deferring again, since no one will be calling into us, but... https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.h:56: CreateTemporaryFileStreamFunction; nit: Per Google style guide, typedefs and enums go before everything else. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:592: DISALLOW_COPY_AND_ASSIGN(ShareableFileReleaseWaiter); nit: Blank line before DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2790: base::MessageLoop::current()->RunUntilIdle(); Think it's worth mentioning that this will wait for the deletion, since the file is deleted synchronously on the bogus "FILE" thread. If the underlying file IO were async, that wouldn't be true. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2790: base::MessageLoop::current()->RunUntilIdle(); base::RunLoop().RunUntilIdle(); https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2823: base::MessageLoop::current()->RunUntilIdle(); base::RunLoop().RunUntilIdle(); https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2846: base::MessageLoop::current()->RunUntilIdle(); base::RunLoop().RunUntilIdle(); https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2898: // message loop for the delete itself. May be worth mentioning this depends on DidCreateTemporaryFile using the FILE thread, rather than the worker pool. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2899: base::MessageLoop::current()->RunUntilIdle(); base::RunLoop().RunUntilIdle(); https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:237: base::Bind(callback, error, base::Passed(&null_stream), Doesn't really matter, but can you just use "scoped_ptr<net::FileStream>()" and get rid of null_stream? https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:272: ShareableFileReference* deletable_file() const { As-is, these only work before PostCallback is called. I think that's a non-obvious property. Suggest a comment, as changing that property looks like a bit of work. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:290: DCHECK(deletable_file_); Weird to mix CHECKs (Higher up in this CL) and DCHECKs in tests. Fine with either one (I tend to prefer CHECKs, since they're tests and it makes it clear they run in release, others prefer DCHECKs, since they're still run on the trybots, apparently, and they don't run in production code) https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:292: file_stream_.PassAs<net::FileStream>(); Can you just inline "file_stream_.PassAs<net::FileStream>()" in the call to bind? Or is the call to base::Passed still needed in that case? If so, this is probably prettier. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:301: scoped_refptr<ShareableFileReference> deletable_file_; DISALLOW_COPY_AND_ASSIGN? Not copyable anyways, with the scoped_ptr, but best to be careful. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:543: EXPECT_EQ(0, leaf_handler->total_bytes_downloaded()); Check that the file was deleted? Without a real write failing, suppose it doesn't really add much to the test, but still feels like we should be doing it (Same goes for the next 3 tests as well). https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/temporary_file_stream.cc (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:29: callback.Run(error_code, scoped_ptr<net::FileStream>(), NULL); The old code seems to have handled deleting the file when there was an error... It looks to me like we'll never get an error other than PLATFORM_FILE_OK when we have a file, on either windows or posix, just calling out this fact i ncase I'm missing something. ("PassPlatformFile" nominally sounds like it's supposed to delete the file if not used, but it doesn't actually seem to have any code to actually do this).
https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:58: // temporary is not deleted before it is closed. On 2014/03/03 21:10:11, mmenke wrote: > May be worth mentioning this lives on the same thread as the > RedirectToFileResourceHandler Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:71: bool write_callback_pending() const { return write_callback_pending_; } On 2014/03/03 21:10:11, mmenke wrote: > Optional: is_writing maybe be clearer. If you prefer this (Which is more > accurate), feel free to keep it as-is. Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:148: writer_ = NULL; On 2014/03/03 21:10:11, mmenke wrote: > Optional: Do we get anything out of NULLing this out? Close() shouldn't call > into |this|...Suppose you could argue it's safer to NULL it out, just wondering > because we're in the destructor, and it's a raw pointer. Not especially. Mostly just makes it clear that we're releasing our reference to that object; after Close(), it could be deleted at any time. I think a previous version did a scoped_ptr<>::release() and I was just mirroring that. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:258: controller()->CancelWithError(net::FileErrorToNetError(error_code)); On 2014/03/03 21:10:11, mmenke wrote: > optional: Should we set did_defer_ to false here? It doesn't currently matter, > but given the spaghetti nature of the behavior of this class, just seems like > one more place that things could go wrong. *shrug* I don't think it matters? I think we want cancel and deferral to be orthogonal anyway, so having one not muck with the others' state seems reasonable. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:272: did_defer_ = false; On 2014/03/03 21:10:11, mmenke wrote: > This still makes me nervous... Yes, if we call ResumeIfDeferred, > OnResponseStarted *shouldn't* be synchronously called, and there's probably even > at least one guarantee of it somewhere, possibly more... But I'd like not to > depend on that. "This" being the did_defer_ outside of the else? Moved to an else block. Resumption has enough sharp edges and this CL's large enough that I'd rather leave the serious refactoring to a follow-up. This was originally just to the small local fixes before trying to fix the troubles with the overall framework. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:313: did_defer_ = false; On 2014/03/03 21:10:11, mmenke wrote: > Maybe put this in an else? We don't be deferring again, since no one will be > calling into us, but... Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.h:56: CreateTemporaryFileStreamFunction; On 2014/03/03 21:10:11, mmenke wrote: > nit: Per Google style guide, typedefs and enums go before everything else. Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:592: DISALLOW_COPY_AND_ASSIGN(ShareableFileReleaseWaiter); On 2014/03/03 21:10:11, mmenke wrote: > nit: Blank line before DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2790: base::MessageLoop::current()->RunUntilIdle(); On 2014/03/03 21:10:11, mmenke wrote: > base::RunLoop().RunUntilIdle(); Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2790: base::MessageLoop::current()->RunUntilIdle(); On 2014/03/03 21:10:11, mmenke wrote: > Think it's worth mentioning that this will wait for the deletion, since the file > is deleted synchronously on the bogus "FILE" thread. If the underlying file IO > were async, that wouldn't be true. Done. (Well, it's still async. Just on the same thread.) https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2823: base::MessageLoop::current()->RunUntilIdle(); On 2014/03/03 21:10:11, mmenke wrote: > base::RunLoop().RunUntilIdle(); Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2846: base::MessageLoop::current()->RunUntilIdle(); On 2014/03/03 21:10:11, mmenke wrote: > base::RunLoop().RunUntilIdle(); Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2898: // message loop for the delete itself. On 2014/03/03 21:10:11, mmenke wrote: > May be worth mentioning this depends on DidCreateTemporaryFile using the FILE > thread, rather than the worker pool. Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2899: base::MessageLoop::current()->RunUntilIdle(); On 2014/03/03 21:10:11, mmenke wrote: > base::RunLoop().RunUntilIdle(); Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:237: base::Bind(callback, error, base::Passed(&null_stream), On 2014/03/03 21:10:11, mmenke wrote: > Doesn't really matter, but can you just use "scoped_ptr<net::FileStream>()" and > get rid of null_stream? Done. I thought that didn't work, but apparently you spell that base::Passed(scoped_ptr<...>()) https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:272: ShareableFileReference* deletable_file() const { On 2014/03/03 21:10:11, mmenke wrote: > As-is, these only work before PostCallback is called. I think that's a > non-obvious property. Suggest a comment, as changing that property looks like a > bit of work. Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:290: DCHECK(deletable_file_); On 2014/03/03 21:10:11, mmenke wrote: > Weird to mix CHECKs (Higher up in this CL) and DCHECKs in tests. Fine with > either one (I tend to prefer CHECKs, since they're tests and it makes it clear > they run in release, others prefer DCHECKs, since they're still run on the > trybots, apparently, and they don't run in production code) Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:292: file_stream_.PassAs<net::FileStream>(); On 2014/03/03 21:10:11, mmenke wrote: > Can you just inline "file_stream_.PassAs<net::FileStream>()" in the call to > bind? Or is the call to base::Passed still needed in that case? If so, this is > probably prettier. Done. base::Passed is still needed, but apparently it has a mode that doesn't take an address. (Funny, I could have sworn that didn't work...) https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:301: scoped_refptr<ShareableFileReference> deletable_file_; On 2014/03/03 21:10:11, mmenke wrote: > > DISALLOW_COPY_AND_ASSIGN? Not copyable anyways, with the scoped_ptr, but best > to be careful. Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:543: EXPECT_EQ(0, leaf_handler->total_bytes_downloaded()); On 2014/03/03 21:10:11, mmenke wrote: > Check that the file was deleted? Without a real write failing, suppose it > doesn't really add much to the test, but still feels like we should be doing it > (Same goes for the next 3 tests as well). Done. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/temporary_file_stream.cc (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:29: callback.Run(error_code, scoped_ptr<net::FileStream>(), NULL); On 2014/03/03 21:10:11, mmenke wrote: > The old code seems to have handled deleting the file when there was an error... > It looks to me like we'll never get an error other than PLATFORM_FILE_OK when we > have a file, on either windows or posix, just calling out this fact i ncase I'm > missing something. > > ("PassPlatformFile" nominally sounds like it's supposed to delete the file if > not used, but it doesn't actually seem to have any code to actually do this). The old code handled that? It didn't really check if the file was there in the first place. That should be handled by this guy: https://codereview.chromium.org/183003009/
Still LGTM. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:258: controller()->CancelWithError(net::FileErrorToNetError(error_code)); On 2014/03/03 23:20:18, David Benjamin wrote: > On 2014/03/03 21:10:11, mmenke wrote: > > optional: Should we set did_defer_ to false here? It doesn't currently > matter, > > but given the spaghetti nature of the behavior of this class, just seems like > > one more place that things could go wrong. > > *shrug* I don't think it matters? I think we want cancel and deferral to be > orthogonal anyway, so having one not muck with the others' state seems > reasonable. Only issue here is that you can defer handling of OnResponseCompleted, which cancellation sends... Anyways, fine as-is. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:272: did_defer_ = false; On 2014/03/03 23:20:18, David Benjamin wrote: > On 2014/03/03 21:10:11, mmenke wrote: > > This still makes me nervous... Yes, if we call ResumeIfDeferred, > > OnResponseStarted *shouldn't* be synchronously called, and there's probably > even > > at least one guarantee of it somewhere, possibly more... But I'd like not to > > depend on that. > > "This" being the did_defer_ outside of the else? Moved to an else block. > > Resumption has enough sharp edges and this CL's large enough that I'd rather > leave the serious refactoring to a follow-up. This was originally just to the > small local fixes before trying to fix the troubles with the overall framework. Yea, "this" being setting did_defer_ to false after calling ResumeIfDeferred. I'm happy with your change. https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... File content/browser/loader/temporary_file_stream.cc (right): https://codereview.chromium.org/82273002/diff/1270001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:29: callback.Run(error_code, scoped_ptr<net::FileStream>(), NULL); On 2014/03/03 23:20:18, David Benjamin wrote: > On 2014/03/03 21:10:11, mmenke wrote: > > The old code seems to have handled deleting the file when there was an > error... > > It looks to me like we'll never get an error other than PLATFORM_FILE_OK when > we > > have a file, on either windows or posix, just calling out this fact i ncase > I'm > > missing something. > > > > ("PassPlatformFile" nominally sounds like it's supposed to delete the file if > > not used, but it doesn't actually seem to have any code to actually do this). > > The old code handled that? It didn't really check if the file was there in the > first place. > > That should be handled by this guy: https://codereview.chromium.org/183003009/ The old code actually did handle that, in a roundabout way. FileStream handled that: If we got ok with an invalid file stream, it would return an error on write, and if we got a valid handle but an error of some sort, it would return whatever we got when trying to write to the file. Looking at the underlying code, I don't think either case is possible, but given the complete absence of function documentation, and the rather odd code to determine the return code... https://codereview.chromium.org/82273002/diff/1290001/content/browser/loader/... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/1290001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:54: // A separate IO-thread object to manage the lifetime of the net::FileStream and nit: IO thread or IOThread are more common.
Any plans on landing this? There's a p-1 bug about one of the issues: https://code.google.com/p/chromium/issues/detail?id=347663 tyoshino has a tiny fix for that particular issue, but if we're landing this soon, seems like we should just land this one.
Did a not entirely trivial rebase. The ResourceLoaderTest logic now has a little bit of duplication due to it... probably could tidy that up a bit and have all the RedirectToFile stuff use a subclass. Could do that now or in a follow-up. I'll ping darin to see if he wants another look at it since he had comments before. https://codereview.chromium.org/82273002/diff/1290001/content/browser/loader/... File content/browser/loader/redirect_to_file_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/1290001/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.cc:54: // A separate IO-thread object to manage the lifetime of the net::FileStream and On 2014/03/04 18:06:52, mmenke wrote: > nit: IO thread or IOThread are more common. Done.
It seems the other bugs this change fixes may also affect XHR. So, the best is to get this landed and merge. But as it looks it may take long time to get this landed, I'll prepare a CL to revert XHR back to use memory to create a blob response.
https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.h:41: // The temporary is vended through a delegate interface. nit: "The temporary [file]..." https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:461: TEST_F(ResourceLoaderTest, RedirectToFile) { Hmm, do these new tests really belong in this file? They don't make use of the |loader_| member variable. They seem more like tests of RedirectToFileResourceHandler instead of ResourceLoader. Perhaps if you want to share code with resource_loader_unittest.cc, we should instead do a bit of refactoring to make that possible? Would it make sense for any of these tests to be written as unit tests instead that just operate on RedirectToFileResourceHandler? It seems like we could separately make some assertions about the behavior of ResourceLoader in this file, and then based on those assertions, directly invoke ResourceHandler methods on a RedirectToFileResourceHandler, simulating what ResourceLoader would do. https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... File content/browser/loader/sync_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... content/browser/loader/sync_resource_handler.cc:93: // TODO(davidben): Can we remove support for download_file in sync requests Yes, I believe we should be able to disallow support for downloading to a file in the sync XHR case.
https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... File content/browser/loader/redirect_to_file_resource_handler.h (right): https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... content/browser/loader/redirect_to_file_resource_handler.h:41: // The temporary is vended through a delegate interface. On 2014/03/11 05:15:58, darin wrote: > nit: "The temporary [file]..." Oh whoops, that comment's no longer accurate anyway. Removed it. https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:461: TEST_F(ResourceLoaderTest, RedirectToFile) { On 2014/03/11 05:15:58, darin wrote: > Hmm, do these new tests really belong in this file? They don't make use of the > |loader_| member variable. They seem more like tests of > RedirectToFileResourceHandler instead of ResourceLoader. Perhaps if you want to > share code with resource_loader_unittest.cc, we should instead do a bit of > refactoring to make that possible? Yeah, that was what I alluded to in comment #38. This file grew a lot more machinery recently. I've gone and finished that refactoring. > Would it make sense for any of these tests to be written as unit tests instead > that just operate on RedirectToFileResourceHandler? It seems like we could > separately make some assertions about the behavior of ResourceLoader in this > file, and then based on those assertions, directly invoke ResourceHandler > methods on a RedirectToFileResourceHandler, simulating what ResourceLoader would > do. Hrm, maybe. There's a lot to simulate though; making ResourceResponses and such. These tests did also expose some bugs in ResourceLoader (see the TODO(davidben) in OnResponseCompleted earlier in this file), though really that's indicative of our ResourceLoader test coverage being pretty poor in itself. https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... File content/browser/loader/sync_resource_handler.cc (right): https://codereview.chromium.org/82273002/diff/1330002/content/browser/loader/... content/browser/loader/sync_resource_handler.cc:93: // TODO(davidben): Can we remove support for download_file in sync requests On 2014/03/11 05:15:58, darin wrote: > Yes, I believe we should be able to disallow support for downloading to a file > in the sync XHR case. Alright. Let's do that in a follow-up, unless you prefer it in the same one.
LGTM https://codereview.chromium.org/82273002/diff/1360001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/1360001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:407: class RedirectToFileTest : public ResourceLoaderTest { OK, nit: maybe use a common prefix of ResourceLoader here? ResourceLoaderRedirectToFileTest? I know that is a mouthful, but it helps to be able to identify the likely file name when you just have a test fixture name.
https://codereview.chromium.org/82273002/diff/1360001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/82273002/diff/1360001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:407: class RedirectToFileTest : public ResourceLoaderTest { On 2014/03/11 20:17:33, darin wrote: > OK, nit: maybe use a common prefix of ResourceLoader here? > ResourceLoaderRedirectToFileTest? I know that is a mouthful, but it helps to be > able to identify the likely file name when you just have a test fixture name. Done.
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/82273002/1380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/82273002/1380001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/82273002/1380001
Message was sent while issue was closed.
Change committed as 256688 |
