Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(572)

Issue 273880044: Workaround for ftruncate issues.

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 2 months ago by Alan Leung Chromium
Modified:
9 years, 1 month ago
Reviewers:
thestig
Base URL:
https://chromium.googlesource.com/breakpad/breakpad/src.git@master
Visibility:
Public.

Description

Workaround 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -3 lines) Patch
M client/minidump_file_writer.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +69 lines, -3 lines 0 comments Download

Messages

Total messages: 29
Alan Leung Chromium
9 years, 2 months ago (2015-10-16 21:56:58 UTC) #1
thestig
https://codereview.appspot.com/273880044/diff/1/client/linux/handler/exception_handler.cc File client/linux/handler/exception_handler.cc (right): https://codereview.appspot.com/273880044/diff/1/client/linux/handler/exception_handler.cc#newcode644 client/linux/handler/exception_handler.cc:644: ignore_result(ftruncate(minidump_descriptor_.fd(), 0)); What about this guy?
9 years, 2 months ago (2015-10-16 22:27:13 UTC) #2
thestig
Is it possible to avoid all the "aligned" variables, and instead just provide a wrapper ...
9 years, 2 months ago (2015-10-16 22:32:06 UTC) #3
Alan Leung Chromium
About using: if defined(__ANDROID__). I don't think it is going to work for us. I ...
9 years, 2 months ago (2015-10-16 23:13:03 UTC) #4
thestig
On 2015/10/16 23:13:03, Alan Leung Chromium wrote: > About using: if defined(__ANDROID__). > > I ...
9 years, 2 months ago (2015-10-16 23:17:13 UTC) #5
Alan Leung Chromium
> Maybe have some Android-only global for whether you are on M or not, initialize ...
9 years, 2 months ago (2015-10-19 21:44:29 UTC) #6
thestig
On 2015/10/19 21:44:29, Alan Leung Chromium wrote: > > Maybe have some Android-only global for ...
9 years, 2 months ago (2015-10-19 22:57:16 UTC) #7
Alan Leung Chromium
> Can the renderer process do a check on startup, see if it's on M, ...
9 years, 2 months ago (2015-10-19 23:52:47 UTC) #8
thestig
On 2015/10/19 23:52:47, Alan Leung Chromium wrote: > > Can the renderer process do a ...
9 years, 2 months ago (2015-10-20 00:07:28 UTC) #9
Alan Leung Chromium
I like this idea. Let me try it.
9 years, 2 months ago (2015-10-20 18:53:14 UTC) #10
Alan Leung Chromium
On 2015/10/20 18:53:14, Alan Leung Chromium wrote: > I like this idea. > > Let ...
9 years, 2 months ago (2015-10-22 08:49:17 UTC) #11
thestig
Looking... what happened to the ftruncate wrapper idea? https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writer.cc File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writer.cc#newcode48 client/minidump_file_writer.cc:48: #include ...
9 years, 2 months ago (2015-10-22 16:30:31 UTC) #12
Alan Leung Chromium
On 2015/10/22 16:30:31, thestig wrote: > Looking... what happened to the ftruncate wrapper idea? > ...
9 years, 2 months ago (2015-10-22 18:51:42 UTC) #13
Alan Leung Chromium
https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writer.cc File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writer.cc#newcode48 client/minidump_file_writer.cc:48: #include <android/log.h> On 2015/10/22 16:30:31, thestig wrote: > nit: ...
9 years, 2 months ago (2015-10-22 18:51:47 UTC) #14
thestig
On 2015/10/22 18:51:47, Alan Leung Chromium wrote: > https://codereview.appspot.com/273880044/diff/40001/client/minidump_file_writer.cc#newcode50 > client/minidump_file_writer.cc:50: extern bool IsAndroidMarshMellow(); > ...
9 years, 2 months ago (2015-10-23 22:51:19 UTC) #15
Alan Leung Chromium
On 2015/10/23 22:51:19, thestig wrote: > On 2015/10/22 18:51:47, Alan Leung Chromium wrote: > > ...
9 years, 2 months ago (2015-10-24 03:53:34 UTC) #16
Alan Leung Chromium
On 2015/10/24 03:53:34, Alan Leung Chromium wrote: > On 2015/10/23 22:51:19, thestig wrote: > > ...
9 years, 1 month ago (2015-10-27 19:55:04 UTC) #17
thestig
https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_writer.cc File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_writer.cc#newcode48 client/minidump_file_writer.cc:48: namespace { nit: add some line breaks https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_writer.cc#newcode57 client/minidump_file_writer.cc:57: ...
9 years, 1 month ago (2015-10-28 01:23:01 UTC) #18
Alan Leung Chromium
PTAL https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_writer.cc File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_writer.cc#newcode48 client/minidump_file_writer.cc:48: namespace { On 2015/10/28 01:23:01, thestig wrote: > ...
9 years, 1 month ago (2015-10-28 18:18:03 UTC) #19
thestig
Can you address the outstanding comment from patch set 6? https://codereview.appspot.com/273880044/diff/120001/client/minidump_file_writer.cc File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/120001/client/minidump_file_writer.cc#newcode60 ...
9 years, 1 month ago (2015-10-28 19:03:28 UTC) #20
Alan Leung Chromium
Addressed the missing comments. PtAL https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_writer.cc File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/100001/client/minidump_file_writer.cc#newcode250 client/minidump_file_writer.cc:250: if (NeedsFTruncateWorkAround()) { On ...
9 years, 1 month ago (2015-10-29 20:41:03 UTC) #21
Alan Leung Chromium
On 2015/10/29 20:41:03, Alan Leung Chromium wrote: > Addressed the missing comments. > > PtAL ...
9 years, 1 month ago (2015-11-02 20:44:47 UTC) #22
thestig
I've not forgotten about this. It's just been a very busy Monday following a day ...
9 years, 1 month ago (2015-11-03 02:27:52 UTC) #23
thestig
https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_writer.cc File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_writer.cc#newcode63 client/minidump_file_writer.cc:63: int result = ftruncate(file, sys_lseek(file, 0, SEEK_END)); If you ...
9 years, 1 month ago (2015-11-03 02:57:20 UTC) #24
thestig
On 2015/11/03 02:57:20, thestig wrote: > When NeedsFTruncateWorkAround() returns true, many test cases in > ...
9 years, 1 month ago (2015-11-03 07:05:20 UTC) #25
Alan Leung Chromium
Hi PTAL: ReportID='5829eee0914a9ad8' is an example upload with this patch. https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_writer.cc File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/160001/client/minidump_file_writer.cc#newcode63 ...
9 years, 1 month ago (2015-11-04 22:43:34 UTC) #26
thestig
lgtm with some nits: https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_writer.cc File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_writer.cc#newcode56 client/minidump_file_writer.cc:56: if (g_checked_need_ftruncate_workaround) { Funny indentation. ...
9 years, 1 month ago (2015-11-05 04:20:31 UTC) #27
Alan Leung Chromium
https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_writer.cc File client/minidump_file_writer.cc (right): https://codereview.appspot.com/273880044/diff/190001/client/minidump_file_writer.cc#newcode56 client/minidump_file_writer.cc:56: if (g_checked_need_ftruncate_workaround) { On 2015/11/05 04:20:31, thestig wrote: > ...
9 years, 1 month ago (2015-11-05 19:56:22 UTC) #28
thestig
9 years, 1 month ago (2015-11-05 23:46:51 UTC) #29
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b