Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
545983005:
CdmFileIOImpl: Refactor Write() implementation. (Closed)
|
Created:
6 years, 3 months ago by xhwang Modified:
6 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, yusukes+watch_chromium.org, tzik, binji+watch_chromium.org, raymes+watch_chromium.org, eme-reviews_chromium.org, teravest+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, ihf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionCdmFileIOImpl: Refactor Write() implementation.
Previously we do a two step write: SetLength() followed by Write(). However,
if CdmFileIOImpl is destroyed after SetLength() but before Write(), the file
will be corrupted (having the new length but the old data). This CL refactor the
write operation to: WriteToTempFile() followed by Rename(). This way, if
CdmFileIOImpl is destroyed after WriteToTempFile() but before Rename(), the
original file will not be touched.
Other refactions done in this CL:
- Add STATE_ERROR state.
- Move OpenFile() from Open() to Read() and renamed it to OpenFileForRead().
- Update/Add test cases to cover new changes.
BUG=410630
TEST=Updated unittests.
Committed: https://crrev.com/5b083339c79a386488d4ce979ed4983c5f7dea0b
Cr-Commit-Position: refs/heads/master@{#295430}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Cannot use "ERROR" on Windows!!! Also polished tests. #
Total comments: 33
Patch Set 3 : comments addressed #
Total comments: 8
Patch Set 4 : comments addressed #
Total comments: 1
Messages
Total messages: 21 (3 generated)
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org, tzik@chromium.org
ddorwin: Please review everything. tzik: I never used the Rename() API before. Please help check that I am using it correctly. Any other suggestions on this CL are also more than welcome! https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... media/cdm/ppapi/cdm_file_io_impl.cc:473: WRITE_ERROR); tzik: It seems when calling Rename(), file_ref_ should be opened, but the target file-ref (pp::FileRef(file_system_, file_name_.c_str()) here) should not be opened. This works for me, but I'd like you to help check whether I am using the API correctly. https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... media/cdm/ppapi/cdm_file_io_impl.cc:499: file_io_.Close(); It seems this is not needed if I reset the resource immediately below: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/file_io.... But it doesn't hurt to be explicit I guess... https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... media/cdm/ppapi/cdm_file_io_impl.cc:500: file_io_.detach(); Is this the correct way to set the resource to null? I ignored the returned resource, will I leak the resource? https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/resource...
https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... media/cdm/ppapi/cdm_file_io_impl.cc:500: file_io_.detach(); On 2014/09/15 23:56:15, xhwang wrote: > Is this the correct way to set the resource to null? > > I ignored the returned resource, will I leak the resource? > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/resource... Reading the documentation, this appears to leak. Var's Detach is more explicit about this: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/var.h&q=... Resource has a protected Clear() function, but it does not appear to be exposed. I think you might need to delete the object. :( https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:78: // The dtor is private. |this| can only be destructed through Close(). "destructor" should fit https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:82: // Call sequence: Open() -> OpenFileSystem() -> FILE_OPENED. FILE_OPENED does not exist anymore and is no longer correct. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:105: if (file_name_str.find('/') != std::string::npos || combine into one block with above? https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:274: state_ = STATE_OPENED; So it is STATE_FILE_SYSTEM_OPENED https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:286: // Open file for read. If file doesn't exist, create a new empty file. We should update the CDM.h API to allow the CDM to specify that it does not want a file to be created. Actually, now that we have moved the actual file opening to Read/Write, we don't need to create the file if it doesn't exist so we can just drop the CREATE flag. If we do that, are we able to distinguish doesn't exist from other read errors? We might need a new state to report to the CDM. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:311: // finished What is the purpose of the finished/not finished part of this figure? I don't remember. Does it still make sense? https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:389: // Create the file if it doesn't exist. Truncate the file to length 0 if it Should we UMA the case where we find a temp file? That indicates that a previous write was interrupted or the move failed. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:413: if (io_buffer_.empty()) { Does this indicate we were told to write 0 bytes? Maybe add a comment. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:466: void CdmFileIOImpl::RenameTempFile() { Comment that this is actually a move and thus overwrites the existing file? Maybe in the .h. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:488: Reset(); Note: The period between line 471 and here is the remaining period of concern. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:500: file_io_.detach(); Why detach? This is leaking a reference I believe. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... File media/cdm/ppapi/cdm_file_io_impl.h (right): https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.h:67: STATE_OPENED, Is this FS or FILE opened? https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.h:160: // Format: /<original_file_name> "original"? Maybe "requested"? https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... File media/cdm/ppapi/cdm_file_io_test.cc (right): https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_test.cc:207: // Read to make sure data is written. ... sure original data... https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_test.cc:315: // Overwite file with zero bytes. For completeness, should we write and confirm kData (demonstrates writing a smaller size)? https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_test.cc:370: // Write() didn't finish and the content of the file is not modified. Yay! :)
xhwang@chromium.org changed reviewers: + dmichael@chromium.org
+dmichael: Please see the question inline about how to release a pepper resource. Thanks! https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... media/cdm/ppapi/cdm_file_io_impl.cc:500: file_io_.detach(); On 2014/09/16 18:07:36, ddorwin wrote: > On 2014/09/15 23:56:15, xhwang wrote: > > Is this the correct way to set the resource to null? > > > > I ignored the returned resource, will I leak the resource? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/resource... > > Reading the documentation, this appears to leak. Var's Detach is more explicit > about this: > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/var.h&q=... > > Resource has a protected Clear() function, but it does not appear to be exposed. > I think you might need to delete the object. :( +dmichael: What's the recommended way to destroy a Resource (e.g. pp::FileRef or pp::FileIO)? We can't use Clear() and detach() seems to leak the resource....
On 2014/09/16 19:56:21, xhwang wrote: > +dmichael: Please see the question inline about how to release a pepper > resource. Thanks! > > https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... > File media/cdm/ppapi/cdm_file_io_impl.cc (right): > > https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... > media/cdm/ppapi/cdm_file_io_impl.cc:500: file_io_.detach(); > On 2014/09/16 18:07:36, ddorwin wrote: > > On 2014/09/15 23:56:15, xhwang wrote: > > > Is this the correct way to set the resource to null? > > > > > > I ignored the returned resource, will I leak the resource? > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/resource... > > > > Reading the documentation, this appears to leak. Var's Detach is more explicit > > about this: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/var.h&q=... > > > > Resource has a protected Clear() function, but it does not appear to be > exposed. > > I think you might need to delete the object. :( > > +dmichael: What's the recommended way to destroy a Resource (e.g. pp::FileRef or > pp::FileIO)? We can't use Clear() and detach() seems to leak the resource.... Also feel free to review the rest of the CL, especially to see whether I am using pepper correctly. Thanks!
https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... media/cdm/ppapi/cdm_file_io_impl.cc:500: file_io_.detach(); On 2014/09/16 19:56:21, xhwang wrote: > On 2014/09/16 18:07:36, ddorwin wrote: > > On 2014/09/15 23:56:15, xhwang wrote: > > > Is this the correct way to set the resource to null? > > > > > > I ignored the returned resource, will I leak the resource? > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/resource... > > > > Reading the documentation, this appears to leak. Var's Detach is more explicit > > about this: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/var.h&q=... > > > > Resource has a protected Clear() function, but it does not appear to be > exposed. > > I think you might need to delete the object. :( > > +dmichael: What's the recommended way to destroy a Resource (e.g. pp::FileRef or > pp::FileIO)? We can't use Clear() and detach() seems to leak the resource.... You can just do file_io_ = pp::FileIO();
Write() -> Rename() looks good to me. Don't multiple CdmFileIOs try to write single file at the same time?
comments addressed
On 2014/09/17 04:41:37, tzik wrote: > Write() -> Rename() looks good to me. > Don't multiple CdmFileIOs try to write single file at the same time? We have a lock to prevent this: https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/ppapi/cd...
I think I addressed all comments. PTAL again. Meanwhile, I'll do more stress test... https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/545983005/diff/1/media/cdm/ppapi/cdm_file_io_... media/cdm/ppapi/cdm_file_io_impl.cc:500: file_io_.detach(); On 2014/09/16 20:46:46, dmichael wrote: > On 2014/09/16 19:56:21, xhwang wrote: > > On 2014/09/16 18:07:36, ddorwin wrote: > > > On 2014/09/15 23:56:15, xhwang wrote: > > > > Is this the correct way to set the resource to null? > > > > > > > > I ignored the returned resource, will I leak the resource? > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/resource... > > > > > > Reading the documentation, this appears to leak. Var's Detach is more > explicit > > > about this: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/var.h&q=... > > > > > > Resource has a protected Clear() function, but it does not appear to be > > exposed. > > > I think you might need to delete the object. :( > > > > +dmichael: What's the recommended way to destroy a Resource (e.g. pp::FileRef > or > > pp::FileIO)? We can't use Clear() and detach() seems to leak the resource.... > You can just do file_io_ = pp::FileIO(); > Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:78: // The dtor is private. |this| can only be destructed through Close(). On 2014/09/16 18:07:36, ddorwin wrote: > "destructor" should fit Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:82: // Call sequence: Open() -> OpenFileSystem() -> FILE_OPENED. On 2014/09/16 18:07:36, ddorwin wrote: > FILE_OPENED does not exist anymore and is no longer correct. Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:105: if (file_name_str.find('/') != std::string::npos || On 2014/09/16 18:07:37, ddorwin wrote: > combine into one block with above? Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:274: state_ = STATE_OPENED; On 2014/09/16 18:07:37, ddorwin wrote: > So it is STATE_FILE_SYSTEM_OPENED Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:286: // Open file for read. If file doesn't exist, create a new empty file. On 2014/09/16 18:07:36, ddorwin wrote: > We should update the CDM.h API to allow the CDM to specify that it does not want > a file to be created. > > Actually, now that we have moved the actual file opening to Read/Write, we don't > need to create the file if it doesn't exist so we can just drop the CREATE flag. > If we do that, are we able to distinguish doesn't exist from other read errors? > We might need a new state to report to the CDM. Changed to impl not to create a new file for read if the file doesn't exist. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:311: // finished On 2014/09/16 18:07:37, ddorwin wrote: > What is the purpose of the finished/not finished part of this figure? I don't > remember. Does it still make sense? Changed "not finished" to "partially read"... Is that more clear? https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:389: // Create the file if it doesn't exist. Truncate the file to length 0 if it On 2014/09/16 18:07:37, ddorwin wrote: > Should we UMA the case where we find a temp file? That indicates that a previous > write was interrupted or the move failed. Good point. But since we are using TRUNCATE, we have no idea whether the file already exists. If we don't use TRUNCATE, the code will be more complicated. I'd hate to complicate the code only for UMA purposes. Let's think more about this... Added TODO. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:413: if (io_buffer_.empty()) { On 2014/09/16 18:07:36, ddorwin wrote: > Does this indicate we were told to write 0 bytes? Maybe add a comment. Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:466: void CdmFileIOImpl::RenameTempFile() { On 2014/09/16 18:07:37, ddorwin wrote: > Comment that this is actually a move and thus overwrites the existing file? > Maybe in the .h. Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:488: Reset(); On 2014/09/16 18:07:36, ddorwin wrote: > Note: The period between line 471 and here is the remaining period of concern. Added comment in .h file https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:500: file_io_.detach(); On 2014/09/16 18:07:37, ddorwin wrote: > Why detach? This is leaking a reference I believe. Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... File media/cdm/ppapi/cdm_file_io_impl.h (right): https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.h:67: STATE_OPENED, On 2014/09/16 18:07:37, ddorwin wrote: > Is this FS or FILE opened? Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.h:160: // Format: /<original_file_name> On 2014/09/16 18:07:37, ddorwin wrote: > "original"? > Maybe "requested"? Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... File media/cdm/ppapi/cdm_file_io_test.cc (right): https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_test.cc:207: // Read to make sure data is written. On 2014/09/16 18:07:37, ddorwin wrote: > ... sure original data... Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_test.cc:315: // Overwite file with zero bytes. On 2014/09/16 18:07:37, ddorwin wrote: > For completeness, should we write and confirm kData (demonstrates writing a > smaller size)? Done. https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_test.cc:370: // Write() didn't finish and the content of the file is not modified. On 2014/09/16 18:07:37, ddorwin wrote: > Yay! :) Acknowledged.
lgtm % comments. Thanks! https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/545983005/diff/20001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:311: // finished On 2014/09/17 21:02:31, xhwang wrote: > On 2014/09/16 18:07:37, ddorwin wrote: > > What is the purpose of the finished/not finished part of this figure? I don't > > remember. Does it still make sense? > > Changed "not finished" to "partially read"... Is that more clear? Acknowledged. https://codereview.chromium.org/545983005/diff/40001/media/cdm/ppapi/cdm_file... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/545983005/diff/40001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:98: if (file_name_str.empty() || file_name_str[0] == '_' || nit: Would be nice to have all the independent checks on separate lines. https://codereview.chromium.org/545983005/diff/40001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:295: if (result != PP_OK && result != PP_ERROR_FILENOTFOUND) { Avoid this by checking the specific error first? https://codereview.chromium.org/545983005/diff/40001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:297: state_ = STATE_ERROR; Who is releasing the lock now? In fact, it's not clear where this is being released except Close(). Does the caller need to call Close before opening another file? https://codereview.chromium.org/545983005/diff/40001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:394: // TODO(xhwang): Find a good way to report to UMA cases where the temporary On 2014/09/17 21:02:31, xhwang wrote: > On 2014/09/16 18:07:37, ddorwin wrote: > > Should we UMA the case where we find a temp file? That indicates that a > previous > > write was interrupted or the move failed. > > Good point. But since we are using TRUNCATE, we have no idea whether the file > already exists. If we don't use TRUNCATE, the code will be more complicated. I'd > hate to complicate the code only for UMA purposes. Let's think more about > this... Added TODO. Thanks. Would the following work (separate CL is fine)? open(WRITE); if (success) setlength(0); else open(WRITE|CREATE);
lgtm
comments addressed
https://codereview.chromium.org/545983005/diff/40001/media/cdm/ppapi/cdm_file... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/545983005/diff/40001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:98: if (file_name_str.empty() || file_name_str[0] == '_' || On 2014/09/17 23:16:38, ddorwin wrote: > nit: Would be nice to have all the independent checks on separate lines. Done. https://codereview.chromium.org/545983005/diff/40001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:295: if (result != PP_OK && result != PP_ERROR_FILENOTFOUND) { On 2014/09/17 23:16:38, ddorwin wrote: > Avoid this by checking the specific error first? I'd like to handle "error" cases first. PP_ERROR_FILENOTFOUND is actually not an error in our case... https://codereview.chromium.org/545983005/diff/40001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:297: state_ = STATE_ERROR; On 2014/09/17 23:16:38, ddorwin wrote: > Who is releasing the lock now? In fact, it's not clear where this is being > released except Close(). Does the caller need to call Close before opening > another file? The FileIO doc says that when a FileIO is opened, it's "in use": https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/ppapi/ap... So we only ReleaseFileLock() if Open() failed (i.e. open file system failed). If Open() succeeded, we won't release the lock until Close() is called. Since we moved OpenFile() from the Open() sequence to the Read() sequence, ReleaseFileLock() is not needed here now. https://codereview.chromium.org/545983005/diff/40001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_impl.cc:394: // TODO(xhwang): Find a good way to report to UMA cases where the temporary On 2014/09/17 23:16:38, ddorwin wrote: > On 2014/09/17 21:02:31, xhwang wrote: > > On 2014/09/16 18:07:37, ddorwin wrote: > > > Should we UMA the case where we find a temp file? That indicates that a > > previous > > > write was interrupted or the move failed. > > > > Good point. But since we are using TRUNCATE, we have no idea whether the file > > already exists. If we don't use TRUNCATE, the code will be more complicated. > I'd > > hate to complicate the code only for UMA purposes. Let's think more about > > this... Added TODO. > > Thanks. Would the following work (separate CL is fine)? > open(WRITE); > if (success) > setlength(0); > else > open(WRITE|CREATE); That'll work, but we are less efficient. As I said, I was a bit hesitating to sacrifice performance/simplicity for logging/UMA purpose... https://codereview.chromium.org/545983005/diff/60001/media/cdm/ppapi/cdm_file... File media/cdm/ppapi/cdm_file_io_test.cc (right): https://codereview.chromium.org/545983005/diff/60001/media/cdm/ppapi/cdm_file... media/cdm/ppapi/cdm_file_io_test.cc:414: START_TEST_CASE("StressTest") I added this test which can pretty reliably repro http:://crbug.com/415401
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/545983005/60001
On 2014/09/18 05:59:20, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patchset/545983005/60001 FYI, the test may be flaky due to http://crbug.com/415401. I'll keep an eye on the try bots. If it is the case, I'll disable those tests that can trigger this failure and land this CL.
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as c449680bcbd6eeb5041111c315478b1206d1a0e3
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5b083339c79a386488d4ce979ed4983c5f7dea0b Cr-Commit-Position: refs/heads/master@{#295430}
Message was sent while issue was closed.
On 2014/09/18 06:58:57, I haz the power (commit-bot) wrote: > Patchset 4 (id:??) landed as > https://crrev.com/5b083339c79a386488d4ce979ed4983c5f7dea0b > Cr-Commit-Position: refs/heads/master@{#295430} The CL is landed and bots look good so far. If the ECKEncryptedMediaTest.FileIOTest test gets flaky, please please do not revert the CL. Ping me please and I'll fix the test. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
