Issue 273880044: Workaround for ftruncate issues.
|
Created:
8 years, 6 months ago by Alan Leung Chromium Modified:
8 years, 6 months ago Reviewers:
thestig Base URL:
https://chromium.googlesource.com/breakpad/breakpad/src.git@master Visibility:
Public. |
DescriptionWorkaround for ftruncate() issues.
We need to work around a bug in M that prevents us from using ftruncate() in the render process.
To do this, I have removed the calls to ftruncate() when allocating bigger minidump files and strictly depends on write() to append to the end.
It might be less efficient but I suspect this is less of an issue on SDCards. It is much better than not getting crash reports.
BUG=542840
Patch Set 1 #
Total comments: 2
Patch Set 2 : use global variable to check android versions #Patch Set 3 : compile error #
Total comments: 4
Patch Set 4 : whitespace fix #Patch Set 5 : Do test in breakpad #Patch Set 6 : fix non work around case #
Total comments: 11
Patch Set 7 : add comments #
Total comments: 2
Patch Set 8 : address comments #Patch Set 9 : remove types.h #
Total comments: 20
Patch Set 10 : tested on linux #Patch Set 11 : fix spelling #
Total comments: 6
Patch Set 12 : final fixes #MessagesTotal messages: 29
Sign in to reply to this message.
https://codereview.appspot.com/273880044/diff/1/client/linux/handler/exceptio... File client/linux/handler/exception_handler.cc (right): https://codereview.appspot.com/273880044/diff/1/client/linux/handler/exceptio... client/linux/handler/exception_handler.cc:644: ignore_result(ftruncate(minidump_descriptor_.fd(), 0)); What about this guy?
Sign in to reply to this message.
Is it possible to avoid all the "aligned" variables, and instead just provide a
wrapper function for ftruncate() and then use the wrapper everywhere?
// Explain why we need this wrapper, reference bug.
int my_ftruncate(int fd, off_t length) {
if defined(__ANDROID__)
// do some magic
#else
return ftruncate(fd, length);
#endif
}
Sign in to reply to this message.
About using: if defined(__ANDROID__). I don't think it is going to work for us. I want to limit this only to "M". The only way to figure out if we are in "M" is making Java calls in the browser process, determine if we want to workaround it and send that boolean down to renderer. https://codereview.appspot.com/273880044/diff/1/client/linux/handler/exceptio... File client/linux/handler/exception_handler.cc (right): https://codereview.appspot.com/273880044/diff/1/client/linux/handler/exceptio... client/linux/handler/exception_handler.cc:644: ignore_result(ftruncate(minidump_descriptor_.fd(), 0)); On 2015/10/16 22:27:13, thestig wrote: > What about this guy? I don't know of a way to work around this. ExceptionHandler::WriteMinidump() is only used in DumpWithoutCrashing(). As long as we are in browser process, we can still ftruncate(). I guess whoever calls DumpWithoutCrashing() outside of that is out of luck. I don't see any current calls. Do you have other suggestions?
Sign in to reply to this message.
On 2015/10/16 23:13:03, Alan Leung Chromium wrote: > About using: if defined(__ANDROID__). > > I don't think it is going to work for us. I want to limit this only to "M". The > only way to figure out if we are in "M" is making Java calls in the browser > process, determine if we want to workaround it and send that boolean down to > renderer. Maybe have some Android-only global for whether you are on M or not, initialize it early on, and check that in the wrapper? Basically if we can make this less invasive. Also, what happens if someone adds a new call to ftruncate() later on? They'll either have to work out whether their call is safe for M and do more fiddling, or they can just use the wrapper and call it a day. ... then we wait 5-10 years for M deprecation, sigh.
Sign in to reply to this message.
> Maybe have some Android-only global for whether you are on M or not, initialize > it early on, and check that in the wrapper? Basically if we can make this less > invasive. I agree with the idea that we should have some sort of wrapper for Android M. Globals doesn't work in renderer because our renderer process is forked from another system process instead of the browser process. I think there is little we can do besides passing a boolean into it during initialization. May be we should just be blunt and straight up call that boolean "should_work_around_ftruncate" instead of "aligned". > Also, what happens if someone adds a new call to ftruncate() later on? They'll > either have to work out whether their call is safe for M and do more fiddling, > or they can just use the wrapper and call it a day. > > ... then we wait 5-10 years for M deprecation, sigh. Hmmm I need to think about this for a bit. May be I can factor out a separate seek / allocate method in the writer. It'll make it more obvious that we are avoiding ftruncate() but completely stopping someone from making a mistake of calling ftruncate elsewhere is not feasible.
Sign in to reply to this message.
On 2015/10/19 21:44:29, Alan Leung Chromium wrote: > > Maybe have some Android-only global for whether you are on M or not, > initialize > > it early on, and check that in the wrapper? Basically if we can make this less > > invasive. > > I agree with the idea that we should have some sort of wrapper for Android M. > > Globals doesn't work in renderer because our renderer process is forked from > another system process instead of the browser process. > > I think there is little we can do besides passing a boolean into it during > initialization. May be we should just be blunt and > straight up call that boolean "should_work_around_ftruncate" instead of > "aligned". Can the renderer process do a check on startup, see if it's on M, and set the global for its own process? Globals are still per-process on all platforms. It's just a matter of whether they can inherit the global from the process that forked it or not.
Sign in to reply to this message.
> Can the renderer process do a check on startup, see if it's on M, and set the > global for its own process? Globals are still per-process on all platforms. It's > just a matter of whether they can inherit the global from the process that > forked it or not. Ya. That's possible. But that'd be the Chromium CL that does this. But to pass that piece of info into breakpad, MinidumpDescriptor seems to be a good place right? How about something like MindumpDescriptor::IsAndroidMarshMellow()?
Sign in to reply to this message.
On 2015/10/19 23:52:47, Alan Leung Chromium wrote:
> > Can the renderer process do a check on startup, see if it's on M, and set
the
> > global for its own process? Globals are still per-process on all platforms.
> It's
> > just a matter of whether they can inherit the global from the process that
> > forked it or not.
>
> Ya. That's possible. But that'd be the Chromium CL that does this. But to pass
> that piece of info into breakpad, MinidumpDescriptor seems to be a good place
> right?
>
> How about something like MindumpDescriptor::IsAndroidMarshMellow()?
If it's possible to figure it out in Breakpad without any involvement from
Chromium, that's best. e.g.
namespace {
if defined(__ANDROID__)
bool g_is_android_marshmellow = false;
bool g_checked_is_android_marshmellow = false;
bool IsAndroidMarshMellow() {
if (g_checked_is_android_marshmellow)
return g_is_android_marshmellow;
g_checked_is_android_marshmellow = true;
// Do check here, set |g_is_android_marshmellow|.
return g_is_android_marshmellow;
}
#endif
} // namespace
int my_ftruncate(int fd, off_t length) {
if defined(__ANDROID__)
if (IsAndroidMarshMellow())
// do some magic
else
return ftruncate(fd, length);
#else
return ftruncate(fd, length);
#endif
}
Sign in to reply to this message.
I like this idea. Let me try it.
Sign in to reply to this message.
On 2015/10/20 18:53:14, Alan Leung Chromium wrote: > I like this idea. > > Let me try it. PTAL and let me know what you think.
Sign in to reply to this message.
Looking... what happened to the ftruncate wrapper idea? https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... client/minidump_file_writer.cc:48: #include <android/log.h> nit: add a blank line after https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... client/minidump_file_writer.cc:50: extern bool IsAndroidMarshMellow(); Where's this implemented?
Sign in to reply to this message.
On 2015/10/22 16:30:31, thestig wrote: > Looking... what happened to the ftruncate wrapper idea? > > https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... > File client/minidump_file_writer.cc (right): > > https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... > client/minidump_file_writer.cc:48: #include <android/log.h> > nit: add a blank line after > > https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... > client/minidump_file_writer.cc:50: extern bool IsAndroidMarshMellow(); > Where's this implemented? I couldn't find a way to create a coherent wrapper. We are not exactly having different versions of truncate. Instead we are working around not using ftruncate. An alternative where we have two sets of everything: Allocate(), AllocateAndroid(), Copy(), CopyAndroid, Close(), CloseAndroid(). I felt that would leave the file with way too much duplicated code.
Sign in to reply to this message.
https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... client/minidump_file_writer.cc:48: #include <android/log.h> On 2015/10/22 16:30:31, thestig wrote: > nit: add a blank line after Done. https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... client/minidump_file_writer.cc:50: extern bool IsAndroidMarshMellow(); On 2015/10/22 16:30:31, thestig wrote: > Where's this implemented? This has to be done in Chromium, unfortunately.
Sign in to reply to this message.
On 2015/10/22 18:51:47, Alan Leung Chromium wrote: > https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... > client/minidump_file_writer.cc:50: extern bool IsAndroidMarshMellow(); > On 2015/10/22 16:30:31, thestig wrote: > > Where's this implemented? > > This has to be done in Chromium, unfortunately. That doesn't work. It would mean Breakpad can't be built on Android standalone. Why does it have to be in Chromium?
Sign in to reply to this message.
On 2015/10/23 22:51:19, thestig wrote: > On 2015/10/22 18:51:47, Alan Leung Chromium wrote: > > > https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... > > client/minidump_file_writer.cc:50: extern bool IsAndroidMarshMellow(); > > On 2015/10/22 16:30:31, thestig wrote: > > > Where's this implemented? > > > > This has to be done in Chromium, unfortunately. > > That doesn't work. It would mean Breakpad can't be built on Android standalone. Argh, you are right. That wouldn't work. > Why does it have to be in Chromium? It doesn't but it is just easier that way. Let me try to see if I can do the check in pure breakpad.
Sign in to reply to this message.
On 2015/10/24 03:53:34, Alan Leung Chromium wrote: > On 2015/10/23 22:51:19, thestig wrote: > > On 2015/10/22 18:51:47, Alan Leung Chromium wrote: > > > > > > https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writ... > > > client/minidump_file_writer.cc:50: extern bool IsAndroidMarshMellow(); > > > On 2015/10/22 16:30:31, thestig wrote: > > > > Where's this implemented? > > > > > > This has to be done in Chromium, unfortunately. > > > > That doesn't work. It would mean Breakpad can't be built on Android > standalone. > > Argh, you are right. That wouldn't work. > > > > Why does it have to be in Chromium? > > It doesn't but it is just easier that way. Let me try to see if I can do the > check in pure breakpad. Ok spoke with the author of the kernel fix. Since we don't know what kernel cherry picked what changes, the best thing to do is just do a ftruncate and check the errno. This works nicely for us since we can cleanly do that check in breakpad without chromium or java invocations. PTAL.
Sign in to reply to this message.
https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... client/minidump_file_writer.cc:48: namespace { nit: add some line breaks https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... client/minidump_file_writer.cc:57: if (result == -1 && errno == EACCES) { include <errno.h> https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... client/minidump_file_writer.cc:57: if (result == -1 && errno == EACCES) { How do we know this is the M bug and not something else? https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... client/minidump_file_writer.cc:250: if (NeedsFTruncateWorkAround()) { Can you explain how this block works? Why is it ok to just update |size_| and return early without changing |position_| ? https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... client/minidump_file_writer.cc:290: #if defined(__ANDROID__) The previous #if / #else is bad IMO and this now makes that 3 layers deep ... all to save writing some extra "return true;\n}\n}" ?
Sign in to reply to this message.
PTAL https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... client/minidump_file_writer.cc:48: namespace { On 2015/10/28 01:23:01, thestig wrote: > nit: add some line breaks Done. https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... client/minidump_file_writer.cc:48: namespace { On 2015/10/28 01:23:01, thestig wrote: > nit: add some line breaks Done. https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... client/minidump_file_writer.cc:57: if (result == -1 && errno == EACCES) { On 2015/10/28 01:23:01, thestig wrote: > include <errno.h> Done. https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... client/minidump_file_writer.cc:57: if (result == -1 && errno == EACCES) { On 2015/10/28 01:23:01, thestig wrote: > How do we know this is the M bug and not something else? We don't unfortunately. The good thing is that the workaround is still fine when applied to unaffected devices. I have added a comment.
Sign in to reply to this message.
Can you address the outstanding comment from patch set 6? https://codereview.appspot.com/273880044/diff/120001/client/minidump_file_wri... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/120001/client/minidump_file_wri... client/minidump_file_writer.cc:60: int result = ftruncate(file, 0); Can you assert that |file| is of size 0 to sanity check? Otherwise we're truncating some existing file and that seems wrong.
Sign in to reply to this message.
Addressed the missing comments. PtAL https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... client/minidump_file_writer.cc:250: if (NeedsFTruncateWorkAround()) { On 2015/10/28 01:23:01, thestig wrote: > Can you explain how this block works? Why is it ok to just update |size_| and > return early without changing |position_| ? Done. https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... client/minidump_file_writer.cc:290: #if defined(__ANDROID__) On 2015/10/28 01:23:01, thestig wrote: > The previous #if / #else is bad IMO and this now makes that 3 layers deep ... > all to save writing some extra "return true;\n}\n}" ? Done. https://codereview.appspot.com/273880044/diff/120001/client/minidump_file_wri... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/120001/client/minidump_file_wri... client/minidump_file_writer.cc:60: int result = ftruncate(file, 0); On 2015/10/28 19:03:27, thestig wrote: > Can you assert that |file| is of size 0 to sanity check? Otherwise we're > truncating some existing file and that seems wrong. Switched to chop off nothing.
Sign in to reply to this message.
On 2015/10/29 20:41:03, Alan Leung Chromium wrote: > Addressed the missing comments. > > PtAL > > https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... > File client/minidump_file_writer.cc (right): > > https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... > client/minidump_file_writer.cc:250: if (NeedsFTruncateWorkAround()) { > On 2015/10/28 01:23:01, thestig wrote: > > Can you explain how this block works? Why is it ok to just update |size_| and > > return early without changing |position_| ? > > Done. > > https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_wri... > client/minidump_file_writer.cc:290: #if defined(__ANDROID__) > On 2015/10/28 01:23:01, thestig wrote: > > The previous #if / #else is bad IMO and this now makes that 3 layers deep ... > > all to save writing some extra "return true;\n}\n}" ? > > Done. > > https://codereview.appspot.com/273880044/diff/120001/client/minidump_file_wri... > File client/minidump_file_writer.cc (right): > > https://codereview.appspot.com/273880044/diff/120001/client/minidump_file_wri... > client/minidump_file_writer.cc:60: int result = ftruncate(file, 0); > On 2015/10/28 19:03:27, thestig wrote: > > Can you assert that |file| is of size 0 to sanity check? Otherwise we're > > truncating some existing file and that seems wrong. > > Switched to chop off nothing. Ping. In case you have forgotten about it. Let me know if you'd like to see anything changed.
Sign in to reply to this message.
I've not forgotten about this. It's just been a very busy Monday following a day off on Friday. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:63: int result = ftruncate(file, sys_lseek(file, 0, SEEK_END)); What if lseek() returns -1? https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:65: // It very likely that we are running into the kernel bug in M devices. Can you provide a https://crbug.com/NNNNNN reference here as well? https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:75: } nit: blank line after this too. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:76: } // namespace nit: 2 spaces before //, next line too https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:110: CheckNeedsFTruncateWorkAround(file); This doesn't build on non-Android. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:309: || sys_lseek(file_, position, SEEK_SET) == static_cast<off_t>(position)) { nit: || on previous line, just like anywhere else that follows google3 style.
Sign in to reply to this message.
https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:63: int result = ftruncate(file, sys_lseek(file, 0, SEEK_END)); If you are going to do: if (ftruncated(...)) { // handle error } below, then do that here too and omit |result| ? https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:260: #if defined(__ANDROID__) Have you checked to make sure everything still works with this change? Keep in mind there's a position() getter that returns the value of |position_|, so external callers may actually care about its value. When NeedsFTruncateWorkAround() returns true, many test cases in src/client/linux/linux_client_unittest fail: [ FAILED ] ExceptionHandlerTest.InstructionPointerMemory [ FAILED ] ExceptionHandlerTest.InstructionPointerMemoryMinBound [ FAILED ] ExceptionHandlerTest.InstructionPointerMemoryMaxBound [ FAILED ] ExceptionHandlerTest.InstructionPointerMemoryNullPointer [ FAILED ] ExceptionHandlerTest.ModuleInfo [ FAILED ] ExceptionHandlerTest.WriteMinidumpExceptionStream [ FAILED ] ExceptionHandlerTest.GenerateMultipleDumpsWithPath [ FAILED ] ExceptionHandlerTest.AdditionalMemory [ FAILED ] ExceptionHandlerTest.AdditionalMemoryRemove [ FAILED ] ExceptionHandlerTest.WriteMinidumpForChild [ FAILED ] MinidumpWriterTest.MappingInfo [ FAILED ] MinidumpWriterTest.MappingInfoContained [ FAILED ] MinidumpWriterTest.DeletedBinary [ FAILED ] MinidumpWriterTest.AdditionalMemory [ FAILED ] MinidumpWriterTest.InvalidStackPointer [ FAILED ] MinidumpWriterTest.MinidumpSizeLimit https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:262: // If ftrucate() is not available. We simply increase the size beyond the typo: 'ftrucate' and again on line 265. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:263: // current file size. sys_write will expand the file when data is written nit: Refer to sys_write as sys_write() for consistency with the rest of the paragraph.
Sign in to reply to this message.
On 2015/11/03 02:57:20, thestig wrote: > When NeedsFTruncateWorkAround() returns true, many test cases in > src/client/linux/linux_client_unittest fail: Also watch out for run away test binaries as a result of this. :\
Sign in to reply to this message.
Hi PTAL: ReportID='5829eee0914a9ad8' is an example upload with this patch. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:63: int result = ftruncate(file, sys_lseek(file, 0, SEEK_END)); On 2015/11/03 02:57:19, thestig wrote: > If you are going to do: if (ftruncated(...)) { // handle error } below, then do > that here too and omit |result| ? Done. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:63: int result = ftruncate(file, sys_lseek(file, 0, SEEK_END)); On 2015/11/03 02:27:52, thestig wrote: > What if lseek() returns -1? I added a check. I suspect there is nothing we can do. If it fails now, it'll probably fail later so I decided to take no action. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:65: // It very likely that we are running into the kernel bug in M devices. On 2015/11/03 02:27:52, thestig wrote: > Can you provide a https://crbug.com/NNNNNN reference here as well? Done. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:75: } On 2015/11/03 02:27:52, thestig wrote: > nit: blank line after this too. Done. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:76: } // namespace On 2015/11/03 02:27:52, thestig wrote: > nit: 2 spaces before //, next line too Done. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:110: CheckNeedsFTruncateWorkAround(file); On 2015/11/03 02:27:52, thestig wrote: > This doesn't build on non-Android. Done. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:260: #if defined(__ANDROID__) On 2015/11/03 02:57:19, thestig wrote: > Have you checked to make sure everything still works with this change? Keep in > mind there's a position() getter that returns the value of |position_|, so > external callers may actually care about its value. > > When NeedsFTruncateWorkAround() returns true, many test cases in > src/client/linux/linux_client_unittest fail: > > [ FAILED ] ExceptionHandlerTest.InstructionPointerMemory > [ FAILED ] ExceptionHandlerTest.InstructionPointerMemoryMinBound > [ FAILED ] ExceptionHandlerTest.InstructionPointerMemoryMaxBound > [ FAILED ] ExceptionHandlerTest.InstructionPointerMemoryNullPointer > [ FAILED ] ExceptionHandlerTest.ModuleInfo > [ FAILED ] ExceptionHandlerTest.WriteMinidumpExceptionStream > [ FAILED ] ExceptionHandlerTest.GenerateMultipleDumpsWithPath > [ FAILED ] ExceptionHandlerTest.AdditionalMemory > [ FAILED ] ExceptionHandlerTest.AdditionalMemoryRemove > [ FAILED ] ExceptionHandlerTest.WriteMinidumpForChild > [ FAILED ] MinidumpWriterTest.MappingInfo > [ FAILED ] MinidumpWriterTest.MappingInfoContained > [ FAILED ] MinidumpWriterTest.DeletedBinary > [ FAILED ] MinidumpWriterTest.AdditionalMemory > [ FAILED ] MinidumpWriterTest.InvalidStackPointer > [ FAILED ] MinidumpWriterTest.MinidumpSizeLimit I fixed the issue and re-ran the tests. It should be fine now. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:262: // If ftrucate() is not available. We simply increase the size beyond the On 2015/11/03 02:57:19, thestig wrote: > typo: 'ftrucate' and again on line 265. Done. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:263: // current file size. sys_write will expand the file when data is written On 2015/11/03 02:57:19, thestig wrote: > nit: Refer to sys_write as sys_write() for consistency with the rest of the > paragraph. Done. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_wri... client/minidump_file_writer.cc:309: || sys_lseek(file_, position, SEEK_SET) == static_cast<off_t>(position)) { On 2015/11/03 02:27:52, thestig wrote: > nit: || on previous line, just like anywhere else that follows google3 style. This line got removed.
Sign in to reply to this message.
lgtm with some nits: https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_wri... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_wri... client/minidump_file_writer.cc:56: if (g_checked_need_ftruncate_workaround) { Funny indentation. https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_wri... client/minidump_file_writer.cc:63: int offset = sys_lseek(file, 0, SEEK_END); int -> off_t https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_wri... client/minidump_file_writer.cc:285: assert(size); asserts stay on top
Sign in to reply to this message.
https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_wri... File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_wri... client/minidump_file_writer.cc:56: if (g_checked_need_ftruncate_workaround) { On 2015/11/05 04:20:31, thestig wrote: > Funny indentation. Done. https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_wri... client/minidump_file_writer.cc:63: int offset = sys_lseek(file, 0, SEEK_END); On 2015/11/05 04:20:31, thestig wrote: > int -> off_t Done. https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_wri... client/minidump_file_writer.cc:285: assert(size); On 2015/11/05 04:20:31, thestig wrote: > asserts stay on top Done.
Sign in to reply to this message.
You can close this issue now. I landed it for you as: https://chromium.googlesource.com/breakpad/breakpad/+/fe64fd4aa28bea49d930352... I suspect your local client is older or incomplete. Thus you uploaded to codereview.appspot.com, instead of breakpad.appspot.com (the previous review site) or codereview.chromium.org. (current review site) For next time, do git clone https://chromium.googlesource.com/breakpad/breakpad and write the CL in there.
Sign in to reply to this message.
|
