Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(104)

Issue 55333002: Make possible to check memory allocations inside chromium (Closed)

Created:
7 years, 1 month ago by kbalazs
Modified:
6 years, 8 months ago
CC:
chromium-reviews, erikwright+watch_chromium.org, dmikurube+memory_chromium.org, Chris Evans, Brad Chen (chromium), bradnelson, noelallen1
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Make possible to check memory allocations inside chromium This patch implements UncheckedMalloc and UncheckedCalloc for Linux. Previously it was only possible on Mac. The motivation is to let callers handle OOM gracefully instead of aborting. When tcmalloc is used this is implemented via a weak symbol that is overridden by tcmalloc. This way we get around the problem that neither base cannot depend on tcmalloc and vica versa. Unfortunately weak symbols are not supported on Windows. To make that work on Windows one will have to do some build system craft that is similar of what we do to replace the system malloc with tcmalloc. This cl does not try to solve the more controversial problem of disallowing the OOM handler for third party libraries under special circumstances. BUG=73751 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258681

Patch Set 1 #

Total comments: 2

Patch Set 2 : Make possible to check memory allocations inside chromium #

Total comments: 1

Patch Set 3 : Make possible to check memory allocations inside chromium #

Total comments: 3

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : #

Total comments: 3

Patch Set 11 : #

Total comments: 1

Patch Set 12 : #

Patch Set 13 : fix windows build #

Total comments: 2

Patch Set 14 : fix linking issue with sandbox_linux_unittests #

Patch Set 15 : fix android_clang_dbg build #

Patch Set 16 : make base depend on allcator if necessary #

Patch Set 17 : incorporated review comments #

Patch Set 18 : rebased patch - upload to try #

Total comments: 1

Patch Set 19 : use tc_malloc_skip_new_handler indirectly #

Patch Set 20 : fix component and windows build #

Total comments: 8

Patch Set 21 : incorporated comments from @willchan and @jin #

Patch Set 22 : weak symbol #

Total comments: 1

Patch Set 23 : weak symbol 2 #

Total comments: 6

Patch Set 24 : weak symbol, added unit test #

Total comments: 15

Patch Set 25 : improve testing code #

Total comments: 5

Patch Set 26 : last minute unit test fixes #

Patch Set 27 : fixes inspired by trybots #

Unified diffs Side-by-side diffs Delta from patch set Stats (+195 lines, -25 lines) Patch
M base/base.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +1 line, -0 lines 0 comments Download
M base/process/memory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +16 lines, -8 lines 0 comments Download
A base/process/memory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +30 lines, -0 lines 0 comments Download
M base/process/memory_linux.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 2 chunks +29 lines, -0 lines 0 comments Download
M base/process/memory_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +22 lines, -6 lines 0 comments Download
M base/process/memory_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 4 chunks +63 lines, -11 lines 0 comments Download
M base/process/memory_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +11 lines, -0 lines 0 comments Download
M third_party/tcmalloc/README.chromium View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -0 lines 0 comments Download
M third_party/tcmalloc/chromium/src/debugallocation.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +8 lines, -0 lines 0 comments Download
M third_party/tcmalloc/chromium/src/tcmalloc.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +14 lines, -0 lines 0 comments Download

Messages

Total messages: 167 (0 generated)
kbalazs
@avi: I'd like to you to share your opinion about the concept @willchain: I'd like ...
7 years, 1 month ago (2013-10-31 21:37:52 UTC) #1
Scott Hess - ex-Googler
Drive-by comment. https://codereview.chromium.org/55333002/diff/1/base/process/memory_linux.cc File base/process/memory_linux.cc (right): https://codereview.chromium.org/55333002/diff/1/base/process/memory_linux.cc#newcode206 base/process/memory_linux.cc:206: memset(result, 0, size); size -> alloc_size.
7 years, 1 month ago (2013-10-31 21:54:38 UTC) #2
Ken Russell (switch to Gerrit)
+dslomov, cevans Thank you for implementing this! I would very much like to hook this ...
7 years, 1 month ago (2013-10-31 22:12:48 UTC) #3
Dai Mikurube (NOT FULLTIME)
Just a drive-by nit. https://codereview.chromium.org/55333002/diff/90001/third_party/tcmalloc/README.chromium File third_party/tcmalloc/README.chromium (right): https://codereview.chromium.org/55333002/diff/90001/third_party/tcmalloc/README.chromium#newcode95 third_party/tcmalloc/README.chromium:95: - Added tc_try_malloc, which is ...
7 years, 1 month ago (2013-11-01 05:53:56 UTC) #4
Avi (use Gerrit)
This looks plausible but I don't know enough about the internals of the memory code ...
7 years, 1 month ago (2013-11-01 20:04:12 UTC) #5
Avi (use Gerrit)
On 2013/11/01 20:04:12, Avi wrote: > This looks plausible but I don't know enough about ...
7 years, 1 month ago (2013-11-01 20:05:55 UTC) #6
kbalazs
> And in case you're wondering, yes, I'm OK with this. I wish there were ...
7 years, 1 month ago (2013-11-01 23:42:44 UTC) #7
kbalazs
Patches coming until it builds on every platform.
7 years, 1 month ago (2013-11-01 23:43:12 UTC) #8
Scott Hess - ex-Googler
On 2013/11/01 23:42:44, kbalazs wrote: > > And in case you're wondering, yes, I'm OK ...
7 years, 1 month ago (2013-11-01 23:58:16 UTC) #9
Avi (use Gerrit)
On 2013/11/01 23:58:16, shess wrote: > You could have: > > // If the buffer ...
7 years, 1 month ago (2013-11-02 03:45:03 UTC) #10
willchan no longer on Chromium
https://codereview.chromium.org/55333002/diff/180001/base/process/memory_win.cc File base/process/memory_win.cc (right): https://codereview.chromium.org/55333002/diff/180001/base/process/memory_win.cc#newcode9 base/process/memory_win.cc:9: #include "base/allocator/unchecked_malloc.h" Why? https://codereview.chromium.org/55333002/diff/180001/third_party/tcmalloc/chromium/src/debugallocation.cc File third_party/tcmalloc/chromium/src/debugallocation.cc (right): https://codereview.chromium.org/55333002/diff/180001/third_party/tcmalloc/chromium/src/debugallocation.cc#newcode1434 third_party/tcmalloc/chromium/src/debugallocation.cc:1434: ...
7 years, 1 month ago (2013-11-02 21:11:44 UTC) #11
willchan no longer on Chromium
The right way to do this would be to expose a new tcmalloc function. tc_malloc_unchecked() ...
7 years, 1 month ago (2013-11-03 02:04:44 UTC) #12
kbalazs
> > https://codereview.chromium.org/55333002/diff/180001/base/process/memory_win.cc#newcode9 > base/process/memory_win.cc:9: #include "base/allocator/unchecked_malloc.h" > Why? Left behind by mistake. > > ...
7 years, 1 month ago (2013-11-04 15:25:31 UTC) #13
kbalazs
On 2013/11/03 02:04:44, willchan wrote: > The right way to do this would be to ...
7 years, 1 month ago (2013-11-04 15:26:03 UTC) #14
kbalazs
I'm confused by the win bot result. This is the error: gyp: name 'win_use_allocator_shim' is ...
7 years, 1 month ago (2013-11-04 15:46:15 UTC) #15
kbalazs
Changed the interface to make WARN_UNUSED_RESULT useful, fixed layering violation.
7 years, 1 month ago (2013-11-04 20:37:09 UTC) #16
willchan no longer on Chromium
I'm adding the gperftools maintainer to see what his thoughts are here.
7 years, 1 month ago (2013-11-05 00:27:42 UTC) #17
Ken Russell (switch to Gerrit)
I'm not qualified to review this patch but the structure looks good to me and ...
7 years, 1 month ago (2013-11-05 01:52:31 UTC) #18
Dmitry Lomov (no reviews)
On 2013/11/05 01:52:31, Ken Russell wrote: > I'm not qualified to review this patch but ...
7 years, 1 month ago (2013-11-05 13:44:05 UTC) #19
kbalazs
The linux bot seems sick: ../../sandbox/linux/services/credentials.cc:8:28: fatalerror: sys/capability.h: No such file or directory This is ...
7 years, 1 month ago (2013-11-05 14:17:48 UTC) #20
Avi (use Gerrit)
On 2013/11/05 14:17:48, kbalazs wrote: > The linux bot seems sick: > ../../sandbox/linux/services/credentials.cc:8:28: fatalerror: sys/capability.h: ...
7 years, 1 month ago (2013-11-05 14:51:35 UTC) #21
alkondratenko
On 2013/11/03 02:04:44, willchan wrote: > The right way to do this would be to ...
7 years, 1 month ago (2013-11-05 18:44:16 UTC) #22
Ken Russell (switch to Gerrit)
The missing sys/capability.h header file is caused by a missing build dependency on the bot. ...
7 years, 1 month ago (2013-11-05 19:20:33 UTC) #23
kbalazs
On 2013/11/05 18:44:16, alkondratenko wrote: > On 2013/11/03 02:04:44, willchan wrote: > > The right ...
7 years, 1 month ago (2013-11-05 20:44:40 UTC) #24
kbalazs
Seems like only the naming issue is unresolved. What about tc_default_malloc (default as it does ...
7 years, 1 month ago (2013-11-06 16:39:15 UTC) #25
alkondratenko
On 2013/11/06 16:39:15, kbalazs wrote: > Seems like only the naming issue is unresolved. > ...
7 years, 1 month ago (2013-11-06 22:11:04 UTC) #26
kbalazs
Renamed the new function to tc_malloc_skip_new_handler
7 years, 1 month ago (2013-11-06 22:57:37 UTC) #27
timvolodine1
after doing a gclient sync on Linux checkout I now get: ../../sandbox/linux/services/credentials.cc:9:28: fatal error: sys/capability.h: ...
7 years, 1 month ago (2013-11-07 11:25:15 UTC) #28
timvolodine1
ah I see, sudo build/install-build-deps.sh seems to fix the issue. On Thu, Nov 7, 2013 ...
7 years, 1 month ago (2013-11-07 11:31:58 UTC) #29
willchan no longer on Chromium
Sorry for the reviewing delay. I've been at IETF all week. I'll try to get ...
7 years, 1 month ago (2013-11-07 15:10:02 UTC) #30
Ken Russell (switch to Gerrit)
Possible to get a review on this soon?
7 years, 1 month ago (2013-11-12 03:39:15 UTC) #31
willchan no longer on Chromium
https://codereview.chromium.org/55333002/diff/660001/base/process/memory_linux.cc File base/process/memory_linux.cc (right): https://codereview.chromium.org/55333002/diff/660001/base/process/memory_linux.cc#newcode144 base/process/memory_linux.cc:144: if (alloc_size && (alloc_size / size) != num_items) Please ...
7 years, 1 month ago (2013-11-12 16:04:19 UTC) #32
willchan no longer on Chromium
Oh, and please update the CL description explaining the implementation (exposing a new API in ...
7 years, 1 month ago (2013-11-12 16:06:35 UTC) #33
kbalazs
https://codereview.chromium.org/55333002/diff/660001/base/process/memory_linux.cc File base/process/memory_linux.cc (right): https://codereview.chromium.org/55333002/diff/660001/base/process/memory_linux.cc#newcode144 base/process/memory_linux.cc:144: if (alloc_size && (alloc_size / size) != num_items) On ...
7 years, 1 month ago (2013-11-12 18:40:45 UTC) #34
kbalazs
Updated UncheckedCalloc to be consistent with tcmalloc's calloc, updated cl description
7 years, 1 month ago (2013-11-12 18:41:42 UTC) #35
kbalazs
Can we finish this? Eventually it turned out that the Blink folks rather want to ...
7 years, 1 month ago (2013-11-13 23:21:52 UTC) #36
willchan no longer on Chromium
Sorry, I realize you aren't aware that I'm only part-time employed at Google. I am ...
7 years, 1 month ago (2013-11-14 00:16:23 UTC) #37
kbalazs
On 2013/11/14 00:16:23, willchan wrote: > Sorry, I realize you aren't aware that I'm only ...
7 years, 1 month ago (2013-11-14 00:30:16 UTC) #38
willchan no longer on Chromium
lgtm https://codereview.chromium.org/55333002/diff/760001/third_party/tcmalloc/chromium/src/debugallocation.cc File third_party/tcmalloc/chromium/src/debugallocation.cc (right): https://codereview.chromium.org/55333002/diff/760001/third_party/tcmalloc/chromium/src/debugallocation.cc#newcode1434 third_party/tcmalloc/chromium/src/debugallocation.cc:1434: extern "C" PERFTOOLS_DLL_DECL void* tc_malloc_skip_new_handler(size_t size) __THROW { ...
7 years, 1 month ago (2013-11-14 00:31:25 UTC) #39
willchan no longer on Chromium
No need to apologize, I appreciate pings. I just realized that you weren't aware of ...
7 years, 1 month ago (2013-11-14 00:32:32 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/900001
7 years, 1 month ago (2013-11-14 00:48:44 UTC) #41
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, ...
7 years, 1 month ago (2013-11-14 02:04:09 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/1160001
7 years, 1 month ago (2013-11-14 16:39:12 UTC) #43
commit-bot: I haz the power
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=175049
7 years, 1 month ago (2013-11-14 18:20:05 UTC) #44
kbalazs
I have this issue: ./out/Release/sandbox_linux_unittests: symbol lookup error: /home/balazs/src/chromium/src/out/Release/lib/libbase.so: undefined symbol: tc_malloc_skip_new_handler Both content_shell and ...
7 years, 1 month ago (2013-11-15 00:04:59 UTC) #45
kbalazs
I had to add allocator.gyp:allocator as a dependency for sandbox_linux_unittests. This is somewhat common, grepping ...
7 years, 1 month ago (2013-11-15 01:11:53 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/1360001
7 years, 1 month ago (2013-11-15 16:16:45 UTC) #47
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=36548
7 years, 1 month ago (2013-11-15 16:45:42 UTC) #48
Markus (顧孟勤)
lgtm I don't sufficiently understand GYP to say whether this is the best fix. It ...
7 years, 1 month ago (2013-11-15 21:15:39 UTC) #49
kbalazs
Yes I think this is a problem with base. It does not depend on allocator. ...
7 years, 1 month ago (2013-11-15 23:27:59 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/1360001
7 years, 1 month ago (2013-11-18 17:48:40 UTC) #51
commit-bot: I haz the power
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_clang_dbg&number=93608
7 years, 1 month ago (2013-11-18 18:20:29 UTC) #52
kbalazs
There is a seemingly unrelated error on android_clang_dbg: In file included from ../../third_party/tcmalloc/chromium/src/base/atomicops.h:95: ../../third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-v6plus.h:219:25:error: value ...
7 years, 1 month ago (2013-11-18 19:00:11 UTC) #53
Dai Mikurube (NOT FULLTIME)
On 2013/11/18 19:00:11, kbalazs wrote: > There is a seemingly unrelated error on android_clang_dbg: > ...
7 years, 1 month ago (2013-11-20 03:51:32 UTC) #54
kbalazs
On 2013/11/20 03:51:32, Dai Mikurube wrote: > On 2013/11/18 19:00:11, kbalazs wrote: > > There ...
7 years, 1 month ago (2013-11-20 18:11:30 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/1740001
7 years, 1 month ago (2013-11-20 20:11:55 UTC) #56
commit-bot: I haz the power
Retried try job too often on linux_chromeos for step(s) cacheinvalidation_unittests, dbus_unittests, google_apis_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=176833
7 years, 1 month ago (2013-11-20 21:23:54 UTC) #57
kbalazs
This will not work. Other unittests are failing with the same error that sandbox_unittests had ...
7 years, 1 month ago (2013-11-20 22:58:44 UTC) #58
kbalazs
Could somebody review it with the latest changes in base.gyp? The rest is the same.
7 years, 1 month ago (2013-11-21 00:28:45 UTC) #59
kbalazs
@willchan: could you review it again with the latest change in base.gyp?
7 years ago (2013-11-25 15:57:45 UTC) #60
willchan no longer on Chromium
Sorry, I thought this CL was done and ignored all updates. I believe adding this ...
7 years ago (2013-11-27 01:48:27 UTC) #61
kbalazs
On 2013/11/27 01:48:27, willchan wrote: > Sorry, I thought this CL was done and ignored ...
7 years ago (2013-11-27 17:06:15 UTC) #62
willchan no longer on Chromium
On 2013/11/27 17:06:15, kbalazs wrote: > On 2013/11/27 01:48:27, willchan wrote: > > Sorry, I ...
7 years ago (2013-11-28 03:37:07 UTC) #63
willchan no longer on Chromium
I think your solution is fine for now. It's possible we could use weak symbols ...
7 years ago (2013-11-28 03:38:27 UTC) #64
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/2090001
7 years ago (2013-12-02 16:50:12 UTC) #65
commit-bot: I haz the power
Retried try job too often on linux_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&number=197143
7 years ago (2013-12-02 18:36:07 UTC) #66
kbalazs
On 2013/12/02 18:36:07, I haz the power (commit-bot) wrote: > Retried try job too often ...
7 years ago (2013-12-02 22:53:06 UTC) #67
kbalazs
On 2013/12/02 22:53:06, kbalazs wrote: > On 2013/12/02 18:36:07, I haz the power (commit-bot) wrote: ...
7 years ago (2013-12-03 00:32:38 UTC) #68
Ken Russell (switch to Gerrit)
On 2013/12/03 00:32:38, kbalazs wrote: > On 2013/12/02 22:53:06, kbalazs wrote: > > On 2013/12/02 ...
7 years ago (2013-12-03 21:53:53 UTC) #69
kbalazs
Adding nacl owners in the hope that they can help. :)
7 years ago (2013-12-04 21:51:20 UTC) #70
kbalazs
On 2013/12/03 21:53:53, Ken Russell wrote: > On 2013/12/03 00:32:38, kbalazs wrote: > > On ...
7 years ago (2013-12-04 21:53:59 UTC) #71
kbalazs
On 2013/12/04 21:51:20, kbalazs wrote: > Adding nacl owners in the hope that they can ...
7 years ago (2013-12-04 22:00:20 UTC) #72
kbalazs
Btw I could not reproduce this locally. I used this command: python chrome/test/nacl_test_injection/buildbot_chrome_nacl_stage.py --disable_glibc --disable_tests=run_ppapi_ppb_image_data_browser_test ...
7 years ago (2013-12-04 22:03:35 UTC) #73
bradn
So I'm told by ncbray that this is probably actual memory corruption in chrome that ...
7 years ago (2013-12-04 22:48:11 UTC) #74
kbalazs
I still cannot reproduce this, both chrome and the nacl tests work fine (Linux, 64 ...
7 years ago (2013-12-05 01:14:20 UTC) #75
Nick Bray (chromium)
On 2013/12/05 01:14:20, kbalazs wrote: > I still cannot reproduce this, both chrome and the ...
7 years ago (2013-12-05 02:06:52 UTC) #76
kbalazs
On 2013/12/05 02:06:52, Nick Bray (chromium) wrote: > On 2013/12/05 01:14:20, kbalazs wrote: > > ...
7 years ago (2013-12-05 16:24:20 UTC) #77
kbalazs
The tests do not really work well in my 32 bit build but not failing ...
7 years ago (2013-12-06 00:26:01 UTC) #78
kbalazs
I have no clue what to do with this. If someone could log in to ...
7 years ago (2013-12-12 23:10:21 UTC) #79
willchan no longer on Chromium
Sorry I haven't been around, I'm only part-time. I don't have an answer here for ...
7 years ago (2013-12-12 23:12:42 UTC) #80
kbalazs
nacl_integration was green on linux_rel_preciese32. It does not give too much information but at least ...
7 years ago (2013-12-13 18:08:43 UTC) #81
Nico
https://codereview.chromium.org/55333002/diff/2110001/base/allocator/allocator_shim.cc File base/allocator/allocator_shim.cc (right): https://codereview.chromium.org/55333002/diff/2110001/base/allocator/allocator_shim.cc#newcode388 base/allocator/allocator_shim.cc:388: *result = je_malloc(size); Don't these need break statements?
6 years, 11 months ago (2014-01-08 03:20:13 UTC) #82
kbalazs
On 2014/01/08 03:20:13, Nico wrote: > https://codereview.chromium.org/55333002/diff/2110001/base/allocator/allocator_shim.cc > File base/allocator/allocator_shim.cc (right): > > https://codereview.chromium.org/55333002/diff/2110001/base/allocator/allocator_shim.cc#newcode388 > ...
6 years, 11 months ago (2014-01-13 19:46:28 UTC) #83
kbalazs
I was able to reproduce this. It is component=static_library that makes a difference. And the ...
6 years, 11 months ago (2014-01-13 20:00:39 UTC) #84
kbalazs
I was so blind... All we need is an indirection across base and tcmalloc. I ...
6 years, 11 months ago (2014-01-13 22:57:55 UTC) #85
kbalazs
On 2014/01/13 22:57:55, kbalazs wrote: > I was so blind... All we need is an ...
6 years, 11 months ago (2014-01-13 23:47:29 UTC) #86
Ken Russell (switch to Gerrit)
On 2014/01/13 23:47:29, kbalazs wrote: > On 2014/01/13 22:57:55, kbalazs wrote: > > I was ...
6 years, 11 months ago (2014-01-14 07:07:53 UTC) #87
willchan no longer on Chromium
Can you update the changelist description? I don't understand what your latest changes are doing. ...
6 years, 11 months ago (2014-01-15 00:25:04 UTC) #88
kbalazs
On 2014/01/15 00:25:04, willchan wrote: > Can you update the changelist description? I don't understand ...
6 years, 11 months ago (2014-01-15 21:37:38 UTC) #89
kbalazs
Something weird happens on linux_chromeos: python ../../../scripts/slave/runtest.py --target Release ... ... Traceback (most recent call ...
6 years, 11 months ago (2014-01-15 22:24:56 UTC) #90
Nico
On Wed, Jan 15, 2014 at 2:24 PM, <b.kelemen@samsung.com> wrote: > Something weird happens on ...
6 years, 11 months ago (2014-01-15 22:34:52 UTC) #91
Nico
On Wed, Jan 15, 2014 at 2:34 PM, Nico Weber <thakis@chromium.org> wrote: > On Wed, ...
6 years, 11 months ago (2014-01-15 22:36:50 UTC) #92
kbalazs
> > > > It's in the tree, just not in the part folks usually ...
6 years, 11 months ago (2014-01-15 22:51:15 UTC) #93
willchan no longer on Chromium
I fixed your CL desc to say "link" instead of "use" TCMalloc directly. Since it's ...
6 years, 11 months ago (2014-01-16 01:30:03 UTC) #94
Nico
On Wed, Jan 15, 2014 at 5:30 PM, <willchan@chromium.org> wrote: > I fixed your CL ...
6 years, 11 months ago (2014-01-16 01:52:17 UTC) #95
Nico
On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org> wrote: > On Wed, ...
6 years, 11 months ago (2014-01-16 01:52:28 UTC) #96
willchan no longer on Chromium
On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org> wrote: > On Wed, ...
6 years, 11 months ago (2014-01-16 01:58:03 UTC) #97
Nico
After reading the patch, can tc_malloc_skip_new_handler in memory_linux.cc be every something different from tc_malloc_skip_new_handler in ...
6 years, 11 months ago (2014-01-16 02:08:38 UTC) #98
willchan no longer on Chromium
It theoretically could, but we have no plans in practice to ever do that. The ...
6 years, 11 months ago (2014-01-16 02:11:50 UTC) #99
Nico
On Wed, Jan 15, 2014 at 6:11 PM, William Chan (陈智昌) <willchan@chromium.org>wrote: > It theoretically ...
6 years, 11 months ago (2014-01-16 02:12:59 UTC) #100
kbalazs
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_linux.cc File base/process/memory_linux.cc (right): https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_linux.cc#newcode141 base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size, void** result) { In case of ...
6 years, 11 months ago (2014-01-16 02:24:48 UTC) #101
willchan no longer on Chromium
On Wed, Jan 15, 2014 at 6:12 PM, Nico Weber <thakis@chromium.org> wrote: > On Wed, ...
6 years, 11 months ago (2014-01-16 02:37:19 UTC) #102
kbalazs
On 2014/01/16 02:12:59, Nico wrote: > On Wed, Jan 15, 2014 at 6:11 PM, William ...
6 years, 11 months ago (2014-01-16 02:47:12 UTC) #103
kbalazs
> But maybe we can add it to content, since ContentMainRunner already calls > tcmalloc ...
6 years, 11 months ago (2014-01-16 02:53:04 UTC) #104
willchan no longer on Chromium
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_linux.cc File base/process/memory_linux.cc (right): https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_linux.cc#newcode141 base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size, void** result) { On 2014/01/16 02:24:50, ...
6 years, 11 months ago (2014-01-16 02:54:42 UTC) #105
kbalazs
On 2014/01/16 01:30:03, willchan wrote: > I fixed your CL desc to say "link" instead ...
6 years, 11 months ago (2014-01-16 03:11:40 UTC) #106
willchan no longer on Chromium
On Wed, Jan 15, 2014 at 7:11 PM, <b.kelemen@samsung.com> wrote: > On 2014/01/16 01:30:03, willchan ...
6 years, 11 months ago (2014-01-16 03:14:15 UTC) #107
kbalazs
On 2014/01/16 02:54:42, willchan wrote: > https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_linux.cc > File base/process/memory_linux.cc (right): > > https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_linux.cc#newcode141 > ...
6 years, 11 months ago (2014-01-16 03:24:56 UTC) #108
kbalazs
> allocator is not part of libbase, yet it's in the base/ directory. So > ...
6 years, 11 months ago (2014-01-16 03:46:41 UTC) #109
kbalazs
@willchan: so, should I update the patch, or wait for you with the static initializer ...
6 years, 11 months ago (2014-01-17 18:01:16 UTC) #110
willchan no longer on Chromium
On 2014/01/17 18:01:16, kbalazs wrote: > @willchan: so, should I update the patch, or wait ...
6 years, 10 months ago (2014-01-28 22:03:43 UTC) #111
jln (very slow on Chromium)
I didn't read the whole thread, but new (nothrow) wasn't enough to satisfy this need? ...
6 years, 10 months ago (2014-01-29 01:34:33 UTC) #112
kbalazs
On 2014/01/29 01:34:33, jln wrote: > I didn't read the whole thread, but new (nothrow) ...
6 years, 10 months ago (2014-01-29 16:28:35 UTC) #113
kbalazs
On 2014/01/28 22:03:43, willchan wrote: > On 2014/01/17 18:01:16, kbalazs wrote: > > @willchan: so, ...
6 years, 10 months ago (2014-01-29 16:35:40 UTC) #114
kbalazs
I don't see any real issues on the try bots. PTAL
6 years, 10 months ago (2014-02-04 19:24:01 UTC) #115
willchan no longer on Chromium
jln: Did that comment satisfy you? b.keleman: My concern is that the behavioral differences here ...
6 years, 10 months ago (2014-02-04 19:45:50 UTC) #116
kbalazs
On 2014/02/04 19:45:50, willchan wrote: > jln: Did that comment satisfy you? > > b.keleman: ...
6 years, 10 months ago (2014-02-04 21:02:31 UTC) #117
jln (very slow on Chromium)
On 2014/02/04 19:45:50, willchan wrote: > jln: Did that comment satisfy you? Yes, the comment ...
6 years, 10 months ago (2014-02-04 22:03:56 UTC) #118
Ken Russell (switch to Gerrit)
On 2014/02/04 21:02:31, kbalazs wrote: > > I agree but I don't think it is ...
6 years, 10 months ago (2014-02-05 00:29:01 UTC) #119
kbalazs
Ping. Someone should make a decision :)
6 years, 10 months ago (2014-02-12 00:08:32 UTC) #120
Ken Russell (switch to Gerrit)
willchan@ told me in the hallway last week that he wanted to get this CL ...
6 years, 10 months ago (2014-02-12 00:11:52 UTC) #121
willchan no longer on Chromium
Sorry, I dropped the ball here. I think we should definitely add support for this. ...
6 years, 10 months ago (2014-02-12 00:11:59 UTC) #122
willchan no longer on Chromium
nooooo, i didn't press the "publish" button yesterday and lost all my comments when my ...
6 years, 10 months ago (2014-02-12 19:26:16 UTC) #123
kbalazs
On 2014/02/12 19:26:16, willchan wrote: > nooooo, i didn't press the "publish" button yesterday and ...
6 years, 10 months ago (2014-02-13 00:03:02 UTC) #124
kbalazs
On 2014/02/13 00:03:02, kbalazs wrote: > On 2014/02/12 19:26:16, willchan wrote: > > nooooo, i ...
6 years, 10 months ago (2014-02-13 00:55:26 UTC) #125
willchan no longer on Chromium
Sorry, I forgot that you wanted to support this on Windows too. Why not use ...
6 years, 10 months ago (2014-02-13 01:21:18 UTC) #126
kbalazs
On 2014/02/13 01:21:18, willchan wrote: > Sorry, I forgot that you wanted to support this ...
6 years, 10 months ago (2014-02-13 18:23:51 UTC) #127
willchan no longer on Chromium
On 2014/02/13 18:23:51, kbalazs wrote: > On 2014/02/13 01:21:18, willchan wrote: > > Sorry, I ...
6 years, 10 months ago (2014-02-13 21:02:59 UTC) #128
kbalazs
> > > > I did not know about that thing :) Where can I ...
6 years, 10 months ago (2014-02-13 22:18:22 UTC) #129
willchan no longer on Chromium
On 2014/02/13 22:18:22, kbalazs wrote: > > > > > > I did not know ...
6 years, 10 months ago (2014-02-13 22:24:38 UTC) #130
kbalazs
> We don't use the google-perftools function patching strategy as we found it to > ...
6 years, 10 months ago (2014-02-13 22:49:30 UTC) #131
willchan no longer on Chromium
Even if you get blocked here on the Windows side of the solution, I'd urge ...
6 years, 10 months ago (2014-02-13 22:57:00 UTC) #132
kbalazs
On 2014/02/13 22:57:00, willchan wrote: > Even if you get blocked here on the Windows ...
6 years, 10 months ago (2014-02-14 20:55:17 UTC) #133
kbalazs
Uploaded the weak symbol version, will update description if we agree this is the way ...
6 years, 10 months ago (2014-02-14 22:23:03 UTC) #134
jar (doing other things)
On 2014/02/13 22:57:00, willchan wrote: > Even if you get blocked here on the Windows ...
6 years, 10 months ago (2014-02-15 00:37:43 UTC) #135
kbalazs
> Glancing at the code... and thinking back.... when TCMalloc fails in malloc it > ...
6 years, 10 months ago (2014-02-15 01:15:28 UTC) #136
jar (doing other things)
The big thing you need is test code. I think there were several large bugs... ...
6 years, 10 months ago (2014-02-26 03:59:50 UTC) #137
Scott Hess - ex-Googler
I think the use of a boolean return with the pointer by reference was because ...
6 years, 10 months ago (2014-02-26 04:16:44 UTC) #138
kbalazs
On 2014/02/26 04:16:44, shess wrote: > I think the use of a boolean return with ...
6 years, 9 months ago (2014-02-27 15:42:33 UTC) #139
kbalazs
Added unit test. PTAL.
6 years, 9 months ago (2014-03-06 20:09:07 UTC) #140
kbalazs
On 2014/03/06 20:09:07, kbalazs wrote: > Added unit test. PTAL. Will update commit message later... ...
6 years, 9 months ago (2014-03-06 20:09:48 UTC) #141
kbalazs
Could someone take a look?
6 years, 9 months ago (2014-03-13 22:12:13 UTC) #142
Scott Hess - ex-Googler
Just reviewed the test, I assume everything else is rebasing noise. [Might be worth posting ...
6 years, 9 months ago (2014-03-13 22:30:31 UTC) #143
kbalazs
On 2014/03/13 22:30:31, shess wrote: > Just reviewed the test, I assume everything else is ...
6 years, 9 months ago (2014-03-13 23:06:28 UTC) #144
kbalazs
https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_unittest.cc File base/process/memory_unittest.cc (right): https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_unittest.cc#newcode386 base/process/memory_unittest.cc:386: SetUpInDeathAssert(); On 2014/03/13 22:30:33, shess wrote: > This shouldn't ...
6 years, 9 months ago (2014-03-13 23:06:58 UTC) #145
Scott Hess - ex-Googler
OK, since you said it needed a new overall review, I tried to give it ...
6 years, 9 months ago (2014-03-13 23:18:42 UTC) #146
kbalazs
PTAL https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_mac.mm File base/process/memory_mac.mm (right): https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_mac.mm#newcode511 base/process/memory_mac.mm:511: return *result = g_old_malloc(malloc_default_zone(), size); On 2014/03/13 23:18:45, ...
6 years, 9 months ago (2014-03-18 20:03:53 UTC) #147
Scott Hess - ex-Googler
LGTM. IMHO, you should fix my nits and not do another review cycle. I'd hate ...
6 years, 9 months ago (2014-03-18 21:10:37 UTC) #148
kbalazs
The CQ bit was checked by b.kelemen@samsung.com
6 years, 9 months ago (2014-03-21 05:39:44 UTC) #149
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/3290001
6 years, 9 months ago (2014-03-21 05:40:15 UTC) #150
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-21 05:41:57 UTC) #151
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
6 years, 9 months ago (2014-03-21 05:41:59 UTC) #152
kbalazs
The CQ bit was checked by b.kelemen@samsung.com
6 years, 9 months ago (2014-03-21 16:00:31 UTC) #153
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/3290001
6 years, 9 months ago (2014-03-21 16:01:14 UTC) #154
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-21 17:42:28 UTC) #155
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) net_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=286048
6 years, 9 months ago (2014-03-21 17:42:30 UTC) #156
kbalazs
The CQ bit was checked by b.kelemen@samsung.com
6 years, 9 months ago (2014-03-21 19:58:40 UTC) #157
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/3290001
6 years, 9 months ago (2014-03-21 19:58:55 UTC) #158
commit-bot: I haz the power
Change committed as 258681
6 years, 9 months ago (2014-03-21 22:07:07 UTC) #159
kbalazs
On 2014/03/21 22:07:07, I haz the power (commit-bot) wrote: > Change committed as 258681 I ...
6 years, 9 months ago (2014-03-21 23:40:34 UTC) #160
Scott Hess - ex-Googler
On 2014/03/21 23:40:34, kbalazs wrote: > On 2014/03/21 22:07:07, I haz the power (commit-bot) wrote: ...
6 years, 9 months ago (2014-03-21 23:42:03 UTC) #161
earthdok
UncheckedCalloc() and UncheckedMalloc() would crash on OOM under AddressSanitizer, because ASan's allocator crashes on OOM ...
6 years, 9 months ago (2014-03-26 15:37:13 UTC) #162
kbalazs
On 2014/03/26 15:37:13, earthdok wrote: > UncheckedCalloc() and UncheckedMalloc() would crash on OOM under > ...
6 years, 9 months ago (2014-03-26 17:11:22 UTC) #163
Scott Hess - ex-Googler
On 2014/03/26 17:11:22, kbalazs wrote: > On 2014/03/26 15:37:13, earthdok wrote: > > UncheckedCalloc() and ...
6 years, 9 months ago (2014-03-26 17:19:51 UTC) #164
earthdok
Sorry for the late reply - forgot to CC myself on this review. On 2014/03/26 ...
6 years, 9 months ago (2014-03-28 13:24:50 UTC) #165
Scott Hess - ex-Googler
On 2014/03/28 13:24:50, earthdok wrote: > However, I would like > to have a better ...
6 years, 9 months ago (2014-03-28 14:09:18 UTC) #166
alkondratenko
6 years, 8 months ago (2014-04-02 05:12:36 UTC) #167
Message was sent while issue was closed.
On 2014/03/21 22:07:07, I haz the power (commit-bot) wrote:
> Change committed as 258681

Corresponding upstream commit can be seen here:
https://github.com/alk/gperftools/commit/0399af1019240e2d9127a588ddc8e31ff465...

Feel free to review it as well.

Powered by Google App Engine
This is Rietveld 408576698