Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1637433003:
Remove use of deprecated CFURLCreateDataAndPropertiesFromResource function. (Closed)
|
Created:
4 years, 11 months ago by ivanpe Modified:
4 years, 10 months ago CC:
google-breakpad-dev_googlegroups.com, Olivier Base URL:
https://chromium.googlesource.com/breakpad/breakpad.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionRemove use of deprecated CFURLCreateDataAndPropertiesFromResource function.
Original change (https://codereview.chromium.org/1527363003/) was failing
in CFReadStreamGetBuffer() call, so changed to CFReadStreamRead() to be
more conservative.
Patch provided by Scott Hancher.
BUG=
R=mark@chromium.org
Committed: https://chromium.googlesource.com/breakpad/breakpad/+/11840d0ffd374c2f70fd93686cc9ee63589c738d
Patch Set 1 #
Total comments: 2
Patch Set 2 : More bulletproof version of working with CFReadStreamRead(). #
Total comments: 2
Patch Set 3 : Addressed the concern over minimizing the calls to CFReadStreamRead(). #Patch Set 4 : Remove whitespace. #
Total comments: 8
Patch Set 5 : Tweaked to conform to stylistic suggestions. #Patch Set 6 : Fixing a merge error. #
Total comments: 2
Patch Set 7 : Moving three lines inside the loop. #Patch Set 8 : Merged with latest changes. #Messages
Total messages: 22 (3 generated)
ivanpe@chromium.org changed reviewers: + mark@chromium.org, ted.mielczarek@gmail.com
LGTM https://codereview.chromium.org/1637433003/diff/1/src/client/mac/handler/mini... File src/client/mac/handler/minidump_generator.cc (right): https://codereview.chromium.org/1637433003/diff/1/src/client/mac/handler/mini... src/client/mac/handler/minidump_generator.cc:152: if (num_bytes_read > 0) { If num_bytes_read == kMaxBufferLength, there may have been more data that we didn’t see. Treat that case as a failure too, unless you implement a read loop to grab the whole thing.
https://codereview.chromium.org/1637433003/diff/1/src/client/mac/handler/mini... File src/client/mac/handler/minidump_generator.cc (right): https://codereview.chromium.org/1637433003/diff/1/src/client/mac/handler/mini... src/client/mac/handler/minidump_generator.cc:152: if (num_bytes_read > 0) { On 2016/01/26 14:08:50, Mark Mentovai wrote: > If num_bytes_read == kMaxBufferLength, there may have been more data that we > didn’t see. Treat that case as a failure too, unless you implement a read loop > to grab the whole thing. Mark, Scott addressed your comment. Can you please take another look.
https://codereview.chromium.org/1637433003/diff/20001/src/client/mac/handler/... File src/client/mac/handler/minidump_generator.cc (right): https://codereview.chromium.org/1637433003/diff/20001/src/client/mac/handler/... src/client/mac/handler/minidump_generator.cc:145: // Actual data file tests: Mac at 480 bytes and iOS at 413 bytes. Now this is irrelevant. https://codereview.chromium.org/1637433003/diff/20001/src/client/mac/handler/... src/client/mac/handler/minidump_generator.cc:150: CFReadStreamRead(read_stream, data_bytes, kMaxBufferLength); I don’t like having this happen twice: I think that this is cleaner and easier to understand: while (true) { CFIndex num_bytes_read = CFReadStreamRead(); if (num_bytes_read < 0) { // I moved this up here into the loop so that num_bytes_read could be scoped to the loop if (data) { CFRelease(data); data = NULL; } break; } else if (num_bytes_read == 0) { break; } else if (!data) { data = CFDataCreateMutable(); } CFDataAppendBytes(); }
On 2016/01/27 18:47:09, Mark Mentovai wrote: > https://codereview.chromium.org/1637433003/diff/20001/src/client/mac/handler/... > File src/client/mac/handler/minidump_generator.cc (right): > > https://codereview.chromium.org/1637433003/diff/20001/src/client/mac/handler/... > src/client/mac/handler/minidump_generator.cc:145: // Actual data file tests: Mac > at 480 bytes and iOS at 413 bytes. > Now this is irrelevant. > > https://codereview.chromium.org/1637433003/diff/20001/src/client/mac/handler/... > src/client/mac/handler/minidump_generator.cc:150: CFReadStreamRead(read_stream, > data_bytes, kMaxBufferLength); > I don’t like having this happen twice: I think that this is cleaner and easier > to understand: > > while (true) { > CFIndex num_bytes_read = CFReadStreamRead(); > if (num_bytes_read < 0) { > // I moved this up here into the loop so that num_bytes_read could be scoped > to the loop > if (data) { > CFRelease(data); > data = NULL; > } > break; > } else if (num_bytes_read == 0) { > break; > } else if (!data) { > data = CFDataCreateMutable(); > } > CFDataAppendBytes(); > } Please land this CL ASAP or revert https://codereview.chromium.org/1527363003/. This is blocking beta on Chrome for iOS.
Description was changed from ========== Remove use of deprecated CFURLCreateDataAndPropertiesFromResource function. Original change (https://codereview.chromium.org/1527363003/) was failing in CFReadStreamGetBuffer() call, so changed to CFReadStreamRead() to be more conservative. Patch provided by Scott Hancher. BUG= ========== to ========== Remove use of deprecated CFURLCreateDataAndPropertiesFromResource function. Original change (https://codereview.chromium.org/1527363003/) was failing in CFReadStreamGetBuffer() call, so changed to CFReadStreamRead() to be more conservative. Patch provided by Scott Hancher. BUG= ==========
Mark, can you please take another look at the latest patch by Scott.
#3 doesn’t really address my comments to #2
On 2016/01/28 19:31:12, Mark Mentovai wrote: > #3 doesn’t really address my comments to #2 Mark, will it be OK to commit the current patch as is so that we can unblock Chrome on iOS? Later, we can work with Scott to address your comment to #2?
On 2016/01/28 19:36:37, ivanpe wrote: > On 2016/01/28 19:31:12, Mark Mentovai wrote: > > #3 doesn’t really address my comments to #2 > > Mark, will it be OK to commit the current patch as is so that we can unblock > Chrome on iOS? Later, we can work with Scott to address your comment to #2? The 2 problems were not reading all the data and calling CFReadStreamRead(). Both of these issues have been addressed. What's the continued issue?
On 2016/01/28 19:39:37, seh1 wrote: > The 2 problems were not reading all the data and calling CFReadStreamRead(). > Both of these issues have been addressed. What's the continued issue? This was my actual comment: https://codereview.chromium.org/1637433003/diff/20001/src/client/mac/handler/... I don’t see the need to have two calls to CFReadStreamRead() at all.
On 2016/01/28 19:42:01, Mark Mentovai wrote: > On 2016/01/28 19:39:37, seh1 wrote: > > The 2 problems were not reading all the data and calling CFReadStreamRead(). > > Both of these issues have been addressed. What's the continued issue? > > This was my actual comment: > > https://codereview.chromium.org/1637433003/diff/20001/src/client/mac/handler/... > > I don’t see the need to have two calls to CFReadStreamRead() at all. More than one call is made only if it's not clear that all the data has been read. If the read amount is below the buffer size, no more data is read. What's the issue with this?
https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... File src/client/mac/handler/minidump_generator.cc (right): https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... src/client/mac/handler/minidump_generator.cc:156: if (num_bytes_read < kMaxBufferLength) break; seh wrote: > More than one call is made only if it's not clear that all the data has been > read. If the read amount is below the buffer size, no more data is read. What's > the issue with this? This line is a bug. https://developer.apple.com/library/mac/documentation/CoreFoundation/Referenc... > This function blocks until at least one byte is available; it does not block > until buffer is filled.
On 2016/01/29 14:40:11, Mark Mentovai wrote: > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > File src/client/mac/handler/minidump_generator.cc (right): > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > src/client/mac/handler/minidump_generator.cc:156: if (num_bytes_read < > kMaxBufferLength) break; > seh wrote: > > More than one call is made only if it's not clear that all the data has been > > read. If the read amount is below the buffer size, no more data is read. > What's > > the issue with this? > > This line is a bug. > > https://developer.apple.com/library/mac/documentation/CoreFoundation/Referenc... > > > This function blocks until at least one byte is available; it does not block > > until buffer is filled. Was simply trying to avoid multiple calls to CFReadStreamRead(), which you objected to. I agree that it could be problematic. Without that addition though, my code is logically the same as your suggestion. Both will call CFReadStreamRead() twice assuming the data is successfully read seeking a return value of 0. So, is this strictly a stylistic objection then? I took your comments as a logical objection.
> Was simply trying to avoid multiple calls to CFReadStreamRead(), How can you say that you’re avoiding multiple calls to CFReadStreamRead() when I see that you’ve still got two of them in the latest patch set? Maybe you misunderstood. I’m not concerned with the program calling CFReadStreamRead() multiple times when directed to do so from a loop. I’m concerned with code hygiene, when you’ve physically written CFReadStreamRead() two times when one would suffice. https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... File src/client/mac/handler/minidump_generator.cc (right): https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... src/client/mac/handler/minidump_generator.cc:148: CFMutableDataRef data = NULL; There is a logic problem. https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... src/client/mac/handler/minidump_generator.cc:150: CFReadStreamRead(read_stream, data_bytes, kMaxBufferLength); Say that this reads some data and returns a positive value… https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... src/client/mac/handler/minidump_generator.cc:156: if (num_bytes_read < kMaxBufferLength) break; …but the positive number is less than kMaxBufferLength… https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... src/client/mac/handler/minidump_generator.cc:156: if (num_bytes_read < kMaxBufferLength) break; …then you’ll break out here… https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... src/client/mac/handler/minidump_generator.cc:159: CFReadStreamRead(read_stream, data_bytes, kMaxBufferLength); …without ever trying to read more… https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... src/client/mac/handler/minidump_generator.cc:171: (CFPropertyListCreateFromXMLData(NULL, data, kCFPropertyListImmutable, …but you haven’t actually read the entire file because CFReadStreamRead() only guarantees that you’ve reached EOF when it returns zero. You may have encountered a short read, and “data” won’t contain the complete contents of the file. https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... src/client/mac/handler/minidump_generator.cc:172: NULL)); So, once again, what do you think about while (true) { CFIndex num_bytes_read = CFReadStreamRead(); // only one if (num_bytes_read < 0) { if (data) { CFRelease(data); data = NULL; } break; // won’t break from the loop until error… } else if (num_bytes_read == 0) { break; // …or genuine EOF } else if (!data) { data = CFDataCreateMutable(); } CFDataAppendBytes(); // still needs another pass here, even if num_bytes_read < kMaxBufferLength }
On 2016/01/31 02:12:05, Mark Mentovai wrote: > > Was simply trying to avoid multiple calls to CFReadStreamRead(), > > How can you say that you’re avoiding multiple calls to CFReadStreamRead() when I > see that you’ve still got two of them in the latest patch set? > > Maybe you misunderstood. I’m not concerned with the program calling > CFReadStreamRead() multiple times when directed to do so from a loop. I’m > concerned with code hygiene, when you’ve physically written CFReadStreamRead() > two times when one would suffice. > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > File src/client/mac/handler/minidump_generator.cc (right): > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > src/client/mac/handler/minidump_generator.cc:148: CFMutableDataRef data = NULL; > There is a logic problem. > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > src/client/mac/handler/minidump_generator.cc:150: CFReadStreamRead(read_stream, > data_bytes, kMaxBufferLength); > Say that this reads some data and returns a positive value… > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > src/client/mac/handler/minidump_generator.cc:156: if (num_bytes_read < > kMaxBufferLength) break; > …but the positive number is less than kMaxBufferLength… > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > src/client/mac/handler/minidump_generator.cc:156: if (num_bytes_read < > kMaxBufferLength) break; > …then you’ll break out here… > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > src/client/mac/handler/minidump_generator.cc:159: CFReadStreamRead(read_stream, > data_bytes, kMaxBufferLength); > …without ever trying to read more… > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > src/client/mac/handler/minidump_generator.cc:171: > (CFPropertyListCreateFromXMLData(NULL, data, kCFPropertyListImmutable, > …but you haven’t actually read the entire file because CFReadStreamRead() only > guarantees that you’ve reached EOF when it returns zero. You may have > encountered a short read, and “data” won’t contain the complete contents of the > file. > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > src/client/mac/handler/minidump_generator.cc:172: NULL)); > So, once again, what do you think about > > while (true) { > CFIndex num_bytes_read = CFReadStreamRead(); // only one > if (num_bytes_read < 0) { > if (data) { > CFRelease(data); > data = NULL; > } > break; // won’t break from the loop until error… > } else if (num_bytes_read == 0) { > break; // …or genuine EOF > } else if (!data) { > data = CFDataCreateMutable(); > } > CFDataAppendBytes(); > // still needs another pass here, even if num_bytes_read < kMaxBufferLength > } You did not read my entire comment. I took your original comment as a logical objection and not strictly a stylistic one. Was trying to satisfy the logical objection and the only way I could do that was to add the break, not that I thought that was really the correct change. I've made the following change to the non-chromium version: const CFIndex kMaxBufferLength = 1024; UInt8 data_bytes[kMaxBufferLength]; CFMutableDataRef data = NULL; while (true) { CFIndex num_bytes_read = CFReadStreamRead(read_stream, data_bytes, kMaxBufferLength); if (num_bytes_read < 0) { if (data) { CFRelease(data); data = NULL; } break; } else if (num_bytes_read == 0) { break; } else if (!data) { data = CFDataCreateMutable(NULL, 0); } CFDataAppendBytes(data, data_bytes, num_bytes_read); } I'm hoping ivanpe@ can merge it over.
On 2016/01/31 08:21:58, seh1 wrote: > On 2016/01/31 02:12:05, Mark Mentovai wrote: > > > Was simply trying to avoid multiple calls to CFReadStreamRead(), > > > > How can you say that you’re avoiding multiple calls to CFReadStreamRead() when > I > > see that you’ve still got two of them in the latest patch set? > > > > Maybe you misunderstood. I’m not concerned with the program calling > > CFReadStreamRead() multiple times when directed to do so from a loop. I’m > > concerned with code hygiene, when you’ve physically written CFReadStreamRead() > > two times when one would suffice. > > > > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > > File src/client/mac/handler/minidump_generator.cc (right): > > > > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > > src/client/mac/handler/minidump_generator.cc:148: CFMutableDataRef data = > NULL; > > There is a logic problem. > > > > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > > src/client/mac/handler/minidump_generator.cc:150: > CFReadStreamRead(read_stream, > > data_bytes, kMaxBufferLength); > > Say that this reads some data and returns a positive value… > > > > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > > src/client/mac/handler/minidump_generator.cc:156: if (num_bytes_read < > > kMaxBufferLength) break; > > …but the positive number is less than kMaxBufferLength… > > > > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > > src/client/mac/handler/minidump_generator.cc:156: if (num_bytes_read < > > kMaxBufferLength) break; > > …then you’ll break out here… > > > > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > > src/client/mac/handler/minidump_generator.cc:159: > CFReadStreamRead(read_stream, > > data_bytes, kMaxBufferLength); > > …without ever trying to read more… > > > > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > > src/client/mac/handler/minidump_generator.cc:171: > > (CFPropertyListCreateFromXMLData(NULL, data, kCFPropertyListImmutable, > > …but you haven’t actually read the entire file because CFReadStreamRead() only > > guarantees that you’ve reached EOF when it returns zero. You may have > > encountered a short read, and “data” won’t contain the complete contents > of the > > file. > > > > > https://codereview.chromium.org/1637433003/diff/60001/src/client/mac/handler/... > > src/client/mac/handler/minidump_generator.cc:172: NULL)); > > So, once again, what do you think about > > > > while (true) { > > CFIndex num_bytes_read = CFReadStreamRead(); // only one > > if (num_bytes_read < 0) { > > if (data) { > > CFRelease(data); > > data = NULL; > > } > > break; // won’t break from the loop until error… > > } else if (num_bytes_read == 0) { > > break; // …or genuine EOF > > } else if (!data) { > > data = CFDataCreateMutable(); > > } > > CFDataAppendBytes(); > > // still needs another pass here, even if num_bytes_read < kMaxBufferLength > > } > > You did not read my entire comment. I took your original comment as a logical > objection and not strictly a stylistic one. Was trying to satisfy the logical > objection and the only way I could do that was to add the break, not that I > thought that was really the correct change. I've made the following change to > the non-chromium version: > const CFIndex kMaxBufferLength = 1024; > UInt8 data_bytes[kMaxBufferLength]; > CFMutableDataRef data = NULL; > while (true) { > CFIndex num_bytes_read = > CFReadStreamRead(read_stream, data_bytes, kMaxBufferLength); > if (num_bytes_read < 0) { > if (data) { > CFRelease(data); > data = NULL; > } > break; > } else if (num_bytes_read == 0) { > break; > } else if (!data) { > data = CFDataCreateMutable(NULL, 0); > } > CFDataAppendBytes(data, data_bytes, num_bytes_read); > } > I'm hoping ivanpe@ can merge it over. The latest patch by Scott is merged over.
LGTM otherwise https://codereview.chromium.org/1637433003/diff/100001/src/client/mac/handler... File src/client/mac/handler/minidump_generator.cc (right): https://codereview.chromium.org/1637433003/diff/100001/src/client/mac/handler... src/client/mac/handler/minidump_generator.cc:146: const CFIndex kMaxBufferLength = 1024; You can move these two inside the loop.
Committing shortly. https://codereview.chromium.org/1637433003/diff/100001/src/client/mac/handler... File src/client/mac/handler/minidump_generator.cc (right): https://codereview.chromium.org/1637433003/diff/100001/src/client/mac/handler... src/client/mac/handler/minidump_generator.cc:146: const CFIndex kMaxBufferLength = 1024; On 2016/02/01 01:03:36, Mark Mentovai wrote: > You can move these two inside the loop. Done.
Description was changed from ========== Remove use of deprecated CFURLCreateDataAndPropertiesFromResource function. Original change (https://codereview.chromium.org/1527363003/) was failing in CFReadStreamGetBuffer() call, so changed to CFReadStreamRead() to be more conservative. Patch provided by Scott Hancher. BUG= ========== to ========== Remove use of deprecated CFURLCreateDataAndPropertiesFromResource function. Original change (https://codereview.chromium.org/1527363003/) was failing in CFReadStreamGetBuffer() call, so changed to CFReadStreamRead() to be more conservative. Patch provided by Scott Hancher. BUG= R=mark@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/11840d0ffd374c2f70fd936... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as 11840d0ffd374c2f70fd93686cc9ee63589c738d (presubmit successful). |
