Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10695110:
Avoid disk accesses on the wrong thread in URLRequestFileJob (Closed)
|
Created:
8 years, 5 months ago by pivanof Modified:
8 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAvoid disk accesses on the wrong thread in URLRequestFileJob
while opening and positioning in the file.
Patch from Pavel Ivanov <paivanof@gmail.com>;
BUG=59849, 114783
TEST=net_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168626
Patch Set 1 #Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 14
Patch Set 5 : #Patch Set 6 : #
Total comments: 6
Patch Set 7 : Avoid disk accesses on the wrong thread in URLRequestFileJob #
Total comments: 17
Patch Set 8 : #Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #
Total comments: 2
Messages
Total messages: 32 (0 generated)
This patch is based on changes in https://chromiumcodereview.appspot.com/10701050/.
Hi Pavel, there were no comments on this review yet because were were all on vacation last week, and Will is out this week too :) I'll try to give this a look over later today Cheers.
LGTM. I haven't reviewed the dependent changelist which introduces CloseAndCancelAsync(), but its usage here looks good. https://chromiumcodereview.appspot.com/10695110/diff/4002/net/url_request/url... File net/url_request/url_request_file_job.cc (right): https://chromiumcodereview.appspot.com/10695110/diff/4002/net/url_request/url... net/url_request/url_request_file_job.cc:61: is_directory_(false), Could you initialize file_size_ to something like 0 or -1, for more predictability in case it is accidentally used before being initialized? https://chromiumcodereview.appspot.com/10695110/diff/4002/net/url_request/url... net/url_request/url_request_file_job.cc:239: if (!request_) I wander if this is still true, or if Kill() will now prevent DidResolve from being called altogether. Fine to leave this line as-is, just thinking out loud. https://chromiumcodereview.appspot.com/10695110/diff/4002/net/url_request/url... net/url_request/url_request_file_job.cc:254: DidOpen(ERR_FILE_NOT_FOUND); [optional] Rather than an if/else chain, I like to break things like this up with some return statements, as in: if (!(*exists)) { DidOpen(ERR_FILE_NOT_FOUND); return; } if (is_directory) { DidOpen(OK); return } // !directory code here...
Thanks eroman. I'll adjust the patch but could you please also look at my comments in http://crbug.com/59849 ? I think it would be good to include in this patch removal of ScopedAllowIO from GetMimeType() too. But it turns out I can't call GetMimeTypeFromFile() in WorkerPool. What thread I can call it in? On 2012/07/10 23:50:56, eroman wrote: https://chromiumcodereview.appspot.com/10695110/diff/4002/net/url_request/url... > File net/url_request/url_request_file_job.cc (right): > > https://chromiumcodereview.appspot.com/10695110/diff/4002/net/url_request/url... > net/url_request/url_request_file_job.cc:239: if (!request_) > I wander if this is still true, or if Kill() will now prevent DidResolve from > being called altogether. Fine to leave this line as-is, just thinking out loud. It looks like you are right, I'll remove this check. > https://chromiumcodereview.appspot.com/10695110/diff/4002/net/url_request/url... > net/url_request/url_request_file_job.cc:254: DidOpen(ERR_FILE_NOT_FOUND); > [optional] Rather than an if/else chain, I like to break things like this up > with some return statements, as in: > > if (!(*exists)) { > DidOpen(ERR_FILE_NOT_FOUND); > return; > } > > if (is_directory) { > DidOpen(OK); > return > } > > // !directory code here... I agree and support this style. I'll change that.
> But it turns out I can't call GetMimeTypeFromFile() in > WorkerPool. What thread I can call it in? If you want to call MimeUtil from the worker pool, try changing its global from: From: base::LazyInstance<MimeUtil> To: base::LazyInstance<MimeUtil>::Leaky The "Leaky" variant means that it will not register with the AtExitManager (and will simply leak on exit). Hence this should avoid the assert you are seeing. Converting MimeUtil in this manner should be safe, since it doesn't do anything important in its destructor. Long term I believe that changing the API for mimeutil to be async is the best solution. That way other callsites don't pick up these problems, and the worker thread stuff can be internalized.
I've update the patch and included changes in GetMimeType(). On 2012/07/11 18:42:32, eroman wrote: > Long term I believe that changing the API for mimeutil to be async is the best > solution. That way other callsites don't pick up these problems, and the worker > thread stuff can be internalized. I'm not sure this is doable. There's too many callsites which will have to be changed to cope with asynchronicity. Could you please also tell me, should I add somebody to dependent patch to get it reviewed? I know Will is the right person but I guess he will be too overloaded next week when he returns from vacation.
https://chromiumcodereview.appspot.com/10695110/diff/8003/net/base/mime_util.cc File net/base/mime_util.cc (right): https://chromiumcodereview.appspot.com/10695110/diff/8003/net/base/mime_util.... net/base/mime_util.cc:89: static base::LazyInstance<MimeUtil>::Leaky g_mime_util = Please add a comment explaining that it is Leaky in order to allow usage on worker threads https://chromiumcodereview.appspot.com/10695110/diff/8003/net/url_request/url... File net/url_request/url_request_file_job.cc (right): https://chromiumcodereview.appspot.com/10695110/diff/8003/net/url_request/url... net/url_request/url_request_file_job.cc:101: base::PlatformFileInfo* file_info = new base::PlatformFileInfo; This feels a bit crowded now with the extra parameters. I suggest packaging them into a single struct, and then just doing one allocation. For instance: struct ResolveAndGetMimeTypeResult { bool exists; base::PlatformFileInfo file_info; bool mime_type_result; std::string read_mime_type; }; You could also use this struct to replace the following member variables: bool is_directory_; bool mime_type_result_; std::string mime_type_; int64 file_size_; Since it would be sufficient to have: ResolveAndGetMimeTypeResult resolve_result_; (Note, I'm not sure if PlatformFileInfo supports default assignment. If it doesn't might want to have the struct simply encapsulate {is_directory, file_size} instead).
On 2012/07/12 19:13:15, eroman wrote: > https://chromiumcodereview.appspot.com/10695110/diff/8003/net/url_request/url... > File net/url_request/url_request_file_job.cc (right): > > https://chromiumcodereview.appspot.com/10695110/diff/8003/net/url_request/url... > net/url_request/url_request_file_job.cc:101: base::PlatformFileInfo* file_info = > new base::PlatformFileInfo; > This feels a bit crowded now with the extra parameters. > > I suggest packaging them into a single struct, and then just doing one > allocation. Of course! Why didn't I think of that? :) I've adjusted your suggestion a little bit and updated the patch.
LGTM. I'll give the dependent review a look over: if I feel qualified I can take it.
Review comments on patch set 4: https://chromiumcodereview.appspot.com/10695110/diff/1004/net/base/mime_util.cc File net/base/mime_util.cc (right): https://chromiumcodereview.appspot.com/10695110/diff/1004/net/base/mime_util.... net/base/mime_util.cc:89: // This variable is Leaky because we need to access it from WorkerPool threads Nit: add a period (.) at the end of this sentence. https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... File net/url_request/url_request_file_job.cc (right): https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.cc:55: meta_info_.mime_type_result_ = false; Please define a default constructor for the FileMetaInfo structure. Then you don't need to manually initialize its members here. https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.cc:85: FileMetaInfo* meta_info = new FileMetaInfo; Will meta_info be deleted by the WorkerPool because of the use of base::Owned(meta_info) on line 92? https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.cc:184: *mime_type = meta_info_.mime_type_; Nit: it seems that we only need to set *mime_type if meta_info_.mime_type_result_ is true. https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.cc:287: DidSeek(-1); BUG?: Why do we pass -1 to DidSeek here? It seems that we should pass rv instead. https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.cc:289: DidSeek(byte_range_.first_byte_position()); This DidSeek call is a little hard to understand because we didn't call Seek in this case. (I figured out why it's necessary.) https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... File net/url_request/url_request_file_job.h (right): https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.h:62: bool is_directory_; The style guide recommends that struct member names should not have trailing underscores. Nit: it would be nice to document the members. All the member names are descriptive except mime_type_result_, which I couldn't figure out. (It means the result/return value for the GetMimeType function.) https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.h:71: // Function fetching file info on a WorkerPool thread. Nit: Function fetching => Fetches or This function fetches Also, it may be better to call the thread "a background thread" instead of "a WorkerPool thread" to be consistent with the comments below. https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.h:81: // Callback after seeking to necessary position in the file which is done Nit: change this comment to: // Callback after seeking to the specified position in the file on a background thread.
https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... File net/url_request/url_request_file_job.cc (right): https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.cc:85: FileMetaInfo* meta_info = new FileMetaInfo; On 2012/07/13 22:38:55, wtc wrote: > > Will meta_info be deleted by the WorkerPool because of the > use of base::Owned(meta_info) on line 92? Yes, that was my intention. https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.cc:184: *mime_type = meta_info_.mime_type_; On 2012/07/13 22:38:55, wtc wrote: > > Nit: it seems that we only need to set *mime_type if > meta_info_.mime_type_result_ is true. I've also thought like that. But just in case of my insufficient knowledge I kept the functionality as it was before. Do you think I should change that? https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.cc:287: DidSeek(-1); On 2012/07/13 22:38:55, wtc wrote: > > BUG?: Why do we pass -1 to DidSeek here? It seems that > we should pass rv instead. -1 is to force DidSeek to go to the path of error processing. Passing rv could make sense in two cases: - Seek() could finish synchronously and return the resulting position; - DidSeek() paid attention to input value in case of an error. Neither of these cases can actually happen although abstractly thinking maybe asynchronous Seek() finishing synchronously is theoretically possible. Do we want to make code safe for this theoretical possibility? https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.cc:289: DidSeek(byte_range_.first_byte_position()); On 2012/07/13 22:38:55, wtc wrote: > > This DidSeek call is a little hard to understand because > we didn't call Seek in this case. (I figured out why it's > necessary.) Is adding a comment explaining that would be good enough?
https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... File net/url_request/url_request_file_job.cc (right): https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... net/url_request/url_request_file_job.cc:287: DidSeek(-1); On 2012/07/13 23:51:21, pivanof wrote: > On 2012/07/13 22:38:55, wtc wrote: > > > > BUG?: Why do we pass -1 to DidSeek here? It seems that > > we should pass rv instead. > > -1 is to force DidSeek to go to the path of error processing. Passing rv could > make sense in two cases: > - Seek() could finish synchronously and return the resulting position; > - DidSeek() paid attention to input value in case of an error. > Neither of these cases can actually happen although abstractly thinking maybe > asynchronous Seek() finishing synchronously is theoretically possible. Do we > want to make code safe for this theoretical possibility? Just noticed that comment to FileStream::Seek() says "Upon success, ERR_IO_PENDING is returned". So Seek() cannot finish synchronously and if its result is not ERR_IO_PENDING then it was unsuccessful. So -1 here cannot be a bug.
On 2012/07/13 23:51:21, pivanof wrote: > https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... > File net/url_request/url_request_file_job.cc (right): > > https://chromiumcodereview.appspot.com/10695110/diff/1004/net/url_request/url... > net/url_request/url_request_file_job.cc:184: *mime_type = meta_info_.mime_type_; > On 2012/07/13 22:38:55, wtc wrote: > > > > Nit: it seems that we only need to set *mime_type if > > meta_info_.mime_type_result_ is true. > > I've also thought like that. But just in case of my insufficient knowledge I > kept the functionality as it was before. Do you think I should change that? I've looked through GetMimeTypeFromFile() and saw that it indeed doesn't change the mime_type string if it returns false. So I've updated the patch with this nit as long as with the other fixes too.
Review comments on patch set 6: Thanks for updating the CL. Just some coding style and comment nits. http://codereview.chromium.org/10695110/diff/24001/net/url_request/url_reques... File net/url_request/url_request_file_job.cc (right): http://codereview.chromium.org/10695110/diff/24001/net/url_request/url_reques... net/url_request/url_request_file_job.cc:293: // stream_.Seek() didn't finish successfully, so pass an intentionally stream_.Seek => stream_->Seek http://codereview.chromium.org/10695110/diff/24001/net/url_request/url_reques... File net/url_request/url_request_file_job.h (right): http://codereview.chromium.org/10695110/diff/24001/net/url_request/url_reques... net/url_request/url_request_file_job.h:69: // Flag showing whether the file name is actually points to a directory. Nit: remove "is", or change "points" to "pointing". Since "points" is usually associated with links (such as symbolic links), I think it's better to simply say "whether the file is actually a directory" or "whether the file name actually refers to a directory". http://codereview.chromium.org/10695110/diff/24001/net/url_request/url_reques... net/url_request/url_request_file_job.h:72: FileMetaInfo(); Declare the constructor before the data members. See the Style Guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order http://codereview.chromium.org/10695110/diff/24001/net/url_request/url_reques... net/url_request/url_request_file_job.h:81: // This function fetches file info on a background thread. Nit: here I did mean just "Fetches". In our comments before function declarations, we often omit "This function" for brevity. See line 75 for an example. The Style Guide also shows example of this comment style: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments http://codereview.chromium.org/10695110/diff/24001/net/url_request/url_reques... net/url_request/url_request_file_job.h:91: // Callback after seeking to the necessary position in the file Nit: the use of "necessary" sounds a little strange. If you don't like "specified" as I suggested, how about "requested"?
Avoid disk accesses on the wrong thread in URLRequestFileJob while opening and positioning in the file. BUG=59849 TEST=net_unittests
I've updated the CL. On 2012/08/09 16:05:42, wtc wrote: > http://codereview.chromium.org/10695110/diff/24001/net/url_request/url_reques... > net/url_request/url_request_file_job.h:91: // Callback after seeking to the > necessary position in the file > > Nit: the use of "necessary" sounds a little strange. If you > don't like "specified" as I suggested, how about "requested"? I didn't like "specified" because for me it sounds like position was passed as a parameter into some function while in reality it isn't passed directly. "requested" sounds better to me, so I've put it instead of "necessary".
Patch set 7 LGTM. Thanks! I reviewed the CL again because I forgot what it was. So I have some new questions and suggested changes. Sorry about that. Please make the changes you agree with and upload a new patch set. http://codereview.chromium.org/10695110/diff/24001/net/url_request/url_reques... File net/url_request/url_request_file_job.h (right): http://codereview.chromium.org/10695110/diff/24001/net/url_request/url_reques... net/url_request/url_request_file_job.h:91: // Callback after seeking to the necessary position in the file paivanof wrote: > > I didn't like "specified" because for me it sounds like > position was passed as a parameter into some function > while in reality it isn't passed directly. I see. We can also say something like "seeking to the beginning of byte_range_". http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... File net/url_request/url_request_file_job.cc (right): http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.cc:100: stream_.reset(); Could you explain why it is necessary to delete the FileStream object here? Is it not enough to just close the FileStream as the original code does? http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.cc:191: return false; Nit: this can be rewritten as: if (meta_info_.mime_type_result) *mime_type = meta_info_.mime_type; return meta_info_.mime_type_result; http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.cc:232: meta_info->is_directory = platform_info.is_directory; It seems that platform_info may contain junk if the file_util::GetFileInfo() call failed. So perhaps this should be written as: base::PlatformFileInfo platform_info; meta_info->file_exists = file_util::GetFileInfo(file_path, &platform_info); if (meta_info->file_exists) { meta_info->file_size = platform_info.size; meta_info->is_directory = platform_info.is_directory; } http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.cc:290: // stream_->Seek() didn't finish successfully, so pass an intentionally Nit: didn't finish successfully => failed http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.cc:297: // to be (it won't be true only when remaining_bytes_ <= 0). I don't understand the sentence in parentheses (it won't be true only when remaining_bytes_ <= 0) I suggest we just remove that sentence. http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... File net/url_request/url_request_file_job.h (right): http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.h:70: bool file_exists; Nit: I suggest listing the struct members in this order: file_exists file_size is_directory mime_type_result mime_type This order highlights the importance of file_exists and mime_type_results -- if they are false, the members after them are irrelevant. http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.h:86: void DidFetchMetaInfo(const FileMetaInfo* meta_info); Nit: should the input parameter be a const reference instead of a const pointer? Does PostTaskAndReply require this to be a pointer? http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_unittest.cc:2454: TEST_F(URLRequestTest, FileTest_Cancel) { Nit: FileTest_Cancel => FileTestCancel
On 2012/08/14 18:00:40, wtc wrote: > paivanof wrote: > > > > I didn't like "specified" because for me it sounds like > > position was passed as a parameter into some function > > while in reality it isn't passed directly. > > I see. We can also say something like > "seeking to the beginning of byte_range_". So the comment will be: "Callback after seeking to the beginning of |byte_range_| in the file on a background thread"? http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... File net/url_request/url_request_file_job.cc (right): http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.cc:100: stream_.reset(); On 2012/08/14 18:00:40, wtc wrote: > > Could you explain why it is necessary to delete the FileStream > object here? Is it not enough to just close the FileStream > as the original code does? We can't call CloseSync() because it will make IO on the wrong thread. CloseAsync() requires a callback parameter and passing NULL to it looks gross (and I don't know whether it will work). The only way of closing the FileStream left is destructor. http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.cc:297: // to be (it won't be true only when remaining_bytes_ <= 0). On 2012/08/14 18:00:40, wtc wrote: > > I don't understand the sentence in parentheses > (it won't be true only when remaining_bytes_ <= 0) > > I suggest we just remove that sentence. What I meant is that statement before parenthesis (FileStream is positioned where we need it to be) will be actually true except one case -- when |remaining_bytes_| <= 0. Maybe I need rephrase? http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... File net/url_request/url_request_file_job.h (right): http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.h:70: bool file_exists; On 2012/08/14 18:00:40, wtc wrote: > > Nit: I suggest listing the struct members in this order: > file_exists > file_size > is_directory > mime_type_result > mime_type > > This order highlights the importance of file_exists and > mime_type_results -- if they are false, the members after > them are irrelevant. I've placed members like that to avoid unnecessary alignment gaps. Do you think that's less important than such logical order? If we change the order then probably comments should be changed to explain the placement logic, right? http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.h:86: void DidFetchMetaInfo(const FileMetaInfo* meta_info); On 2012/08/14 18:00:40, wtc wrote: > > Nit: should the input parameter be a const reference instead of > a const pointer? Does PostTaskAndReply require this to be > a pointer? IIUC, with const reference PostTaskAndReply will save contents of FileMetaInfo struct at the time of calling base::Bind(), i.e. even before the call to PostTaskAndReply(). And so any changes inside FetchMetaInfo() won't come to DidFetchMetaInfo().
http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... File net/url_request/url_request_file_job.cc (right): http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.cc:297: // to be (it won't be true only when remaining_bytes_ <= 0). I suggest something like this: // We didn't need to call stream_->Seek() at all, so pass // the value that would mean seek success to DidSeek() to // skip the code for handling seek failure. http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... File net/url_request/url_request_file_job.h (right): http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.h:70: bool file_exists; On 2012/08/15 06:36:50, pivanof wrote: > > I've placed members like that to avoid unnecessary alignment gaps. Ah, I see. Then please disregard my suggestion. http://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reques... net/url_request/url_request_file_job.h:92: // on a background thread. Please change this comment to: // Callback after seeking to the beginning of |byte_range_| in // the file on a background thread
eroman, wtc: Now that https://chromiumcodereview.appspot.com/10701050/ is committed I rebased this CL and it's ready to be committed too. So please take a quick final look that nothing bad slipped in during the rebase.
Patch set 9 LGTM. I only reviewed the diffs between patch sets 7 and 9. https://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reque... File net/url_request/url_request_file_job.cc (right): https://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reque... net/url_request/url_request_file_job.cc:232: meta_info->is_directory = platform_info.is_directory; On 2012/08/14 18:00:40, wtc wrote: > > It seems that platform_info may contain junk if the > file_util::GetFileInfo() call failed. So perhaps this should > be written as: > > base::PlatformFileInfo platform_info; > meta_info->file_exists = file_util::GetFileInfo(file_path, &platform_info); > if (meta_info->file_exists) { > meta_info->file_size = platform_info.size; > meta_info->is_directory = platform_info.is_directory; > } This change is probably worth making. I am not 100% sure though, because we still need to set meta_info->file_size and meta_info->is_directory to some dummy values if file_util::GetFileInfo() fails. file_util::GetFileInfo( https://codereview.chromium.org/10695110/diff/43001/net/url_request/url_reque... File net/url_request/url_request_file_job.cc (right): https://codereview.chromium.org/10695110/diff/43001/net/url_request/url_reque... net/url_request/url_request_file_job.cc:287: // stream_->Seek() didn't finish successfully, so pass an intentionally didn't finish successfully => failed https://codereview.chromium.org/10695110/diff/43001/net/url_request/url_reque... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/10695110/diff/43001/net/url_request/url_reque... net/url_request/url_request_unittest.cc:566: TEST_F(URLRequestTest, FileTest_Cancel) { FileTest_Cancel => FileTestCancel
On 2012/11/14 21:05:00, wtc wrote: > https://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reque... > File net/url_request/url_request_file_job.cc (right): > > https://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reque... > net/url_request/url_request_file_job.cc:232: meta_info->is_directory = > platform_info.is_directory; > > On 2012/08/14 18:00:40, wtc wrote: > > > > It seems that platform_info may contain junk if the > > file_util::GetFileInfo() call failed. So perhaps this should > > be written as: > > > > base::PlatformFileInfo platform_info; > > meta_info->file_exists = file_util::GetFileInfo(file_path, &platform_info); > > if (meta_info->file_exists) { > > meta_info->file_size = platform_info.size; > > meta_info->is_directory = platform_info.is_directory; > > } > > This change is probably worth making. I am not 100% sure > though, because we still need to set meta_info->file_size and > meta_info->is_directory to some dummy values if > file_util::GetFileInfo() fails. Done. But why do you think dummy values are needed? I don't see them used anywhere when file doesn't exist. > https://codereview.chromium.org/10695110/diff/43001/net/url_request/url_reque... > File net/url_request/url_request_file_job.cc (right): > > https://codereview.chromium.org/10695110/diff/43001/net/url_request/url_reque... > net/url_request/url_request_file_job.cc:287: // stream_->Seek() didn't finish > successfully, so pass an intentionally > > didn't finish successfully => failed Done. > https://codereview.chromium.org/10695110/diff/43001/net/url_request/url_reque... > File net/url_request/url_request_unittest.cc (right): > > https://codereview.chromium.org/10695110/diff/43001/net/url_request/url_reque... > net/url_request/url_request_unittest.cc:566: TEST_F(URLRequestTest, > FileTest_Cancel) { > > FileTest_Cancel => FileTestCancel Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/paivanof@gmail.com/10695110/44004
Patch set 10 LGTM. Thanks. https://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reque... File net/url_request/url_request_file_job.cc (right): https://codereview.chromium.org/10695110/diff/31001/net/url_request/url_reque... net/url_request/url_request_file_job.cc:232: meta_info->is_directory = platform_info.is_directory; pivanof wrote: > Done. But why do you think dummy values are needed? I > don't see them used anywhere when file doesn't exist. The dummy values are for extra protection, just in case someone still uses meta_info->file_size and meta_info->is_directory even though meta_info->file_exists is false. We can set meta_info->file_size to -1 and meta_info->is_directory to false in that case. NOTE: platform_info.size is initialized to 0, whereas meta_info->file_size is initialized to -1. It would be nice to be consistent.
On 2012/11/17 02:15:29, wtc wrote: > pivanof wrote: > > Done. But why do you think dummy values are needed? I > > don't see them used anywhere when file doesn't exist. > > The dummy values are for extra protection, just in > case someone still uses meta_info->file_size and > meta_info->is_directory even though meta_info->file_exists > is false. We can set meta_info->file_size to -1 and > meta_info->is_directory to false in that case. > > NOTE: platform_info.size is initialized to 0, whereas > meta_info->file_size is initialized to -1. It would be > nice to be consistent. Right, I forgot that we added constructor FileMetainfo and so we already have the dummy values. So we should be fine here. I've changed default to 0 for consistency. PTAL.
Patch set 11 LGTM. Thanks. https://codereview.chromium.org/10695110/diff/52014/net/url_request/url_reque... File net/url_request/url_request_file_job.cc (right): https://codereview.chromium.org/10695110/diff/52014/net/url_request/url_reque... net/url_request/url_request_file_job.cc:94: FileMetaInfo* meta_info = new FileMetaInfo(); The original form (without "()") should also be OK.
On 2012/11/19 19:48:01, wtc wrote: > https://codereview.chromium.org/10695110/diff/52014/net/url_request/url_reque... > File net/url_request/url_request_file_job.cc (right): > > https://codereview.chromium.org/10695110/diff/52014/net/url_request/url_reque... > net/url_request/url_request_file_job.cc:94: FileMetaInfo* meta_info = new > FileMetaInfo(); > > The original form (without "()") should also be OK. Yes, I know that it will work the same way. But I think that because skipping parenthesis makes difference only when there's no constructor doing that with defined constructor may be confusing for the code reader.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/paivanof@gmail.com/10695110/52014
Change committed as 168626
This change made URLRequestTest.FileTestMultipleRanges faling on some Win bots. http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%28dbg%29%282%... In that test, events are expected to happen in the following order: (1) URLRequest calls URLRequestFileJob::SetExtraRequestHeaders. (2) URLRequestFileJob::SetExtraRequestHeaders calls NotifyDone with FAILED because it does not support multiple ranges.n (3) URLRequest calls URLRequestFileJob::Start (4) URLRequestFileJob performs some file operations (5) The test calls file_util::Delete. It began failing because the execution order of (4) and (5) became nondeterministic.
hashimoto: thank you for letting us know and reverting this CL. pivanof: I think I understand what the problem is, but I don't have a good solution. What I wrote below is the result of looking at this for about 20 minutes. It could be wrong. https://chromiumcodereview.appspot.com/10695110/diff/52014/net/url_request/ur... File net/url_request/url_request_file_job.cc (left): https://chromiumcodereview.appspot.com/10695110/diff/52014/net/url_request/ur... net/url_request/url_request_file_job.cc:265: return; Perhaps the fix is to carry this check to the URLRequestFileJob::DidFetchMetaInfo method in the new code? The weak pointer factory cannot handle this case because |this| is still valid, but this->request_ (the URLRequest object) is NULL. But I think the stream_->Open and stream_->Seek calls will still be problematic, and I don't have a good solution...
Actually it looks to me that the problem actually wasn't introduced by this commit, it probably was introduced by https://chromiumcodereview.appspot.com/10701050/. Maybe this commit just aggravated some race and it became visible. I think what happens is URLRequestFileJob opens FileStream for the temporary file, then it calls NotifyDone with error, then either Kill() or destructor is called. But FileStream doesn't close file in destructor, it puts job into WorkerPool that will eventually close file. But before it has a chance to do that test is checking on the main thread that it can delete the file and it fails. I think this deletion check in the test is invalid and should be removed. I'm not sure what to do with temporary file though, how to eventually delete it and not leave in filesystem after test is finished. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
