Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10391178:
1. Enable large object pointer offset check in release build. (Closed)
|
Created:
8 years, 7 months ago by kaiwang Modified:
8 years, 7 months ago CC:
chromium-reviews, erikwright (departed), Aaron Boodman, mihaip-chromium-reviews_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
Description1. Enable large object pointer offset check in release build.
Following code will now cause a check error:
char* p = reinterpret_cast<char*>(malloc(kMaxSize + 1));
free(p + 1);
2. Remove a duplicated error reporting function "DieFromBadFreePointer", can
use "InvalidGetAllocatedSize".
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138775
Patch Set 1 #Patch Set 2 : #
Total comments: 30
Patch Set 3 : #
Total comments: 23
Patch Set 4 : #
Total comments: 11
Patch Set 5 : #
Total comments: 7
Patch Set 6 : #
Messages
Total messages: 15 (0 generated)
https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... File base/allocator/allocator.gyp (right): https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator.gyp:430: nit: remove line. https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator.gyp:525: # This is tricky.. We add this include dir so alternate_timer.cc will I'd suggest asking rvargas about this and be sure he's fine with it. https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator.gyp:526: # use TCMalloc's logging.h, instead of the intuitional chromium nit: delete "intuitional" https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator.gyp:527: # base/logging.h as in other building targets. nit: building-->build https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... File base/allocator/allocator_extension_thunks.cc (right): https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator_extension_thunks.cc:14: // allocator_unittests and TCMalloc tests work on windows. That target has nit: TCMalloc --> tcmalloc https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator_extension_thunks.cc:15: // to perform/ tcmalloc-specific initialization on windows, but it cannot depend nit: typo perform/ -->perform https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... File base/allocator/allocator_shim.h (right): https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator_shim.h:17: void* TCMallocDoMallocForTest(size_t size); nit: probably TcmallocDoMallocForTest() https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/tcma... File base/allocator/tcmalloc_free_check_test.cc (right): https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/tcma... base/allocator/tcmalloc_free_check_test.cc:16: reinterpret_cast<volatile int*>(TCMallocDoMallocForTest(kMaxSize + 1)); nit: No need for volatile here. https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/tcma... base/allocator/tcmalloc_free_check_test.cc:17: ASSERT_DEATH(TCMallocDoFreeForTest(const_cast<int*>(p + kAlignment)), Perhaps try adding small powers of two: 1, 2, 4, 8, ... 1024 https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/tcma... base/allocator/tcmalloc_free_check_test.cc:21: TEST(TCMallocFreeCheck, BadPointerInSubsequentPagesOfTheLargeObject) { nit: suggest better name: BadPageAlignedPointerInsideLargeObject https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/tcma... base/allocator/tcmalloc_free_check_test.cc:24: ASSERT_DEATH(TCMallocDoFreeForTest(const_cast<int*>(p + kPageSize)), perhaps iterate at page boundaries (but stay inside object) https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... File third_party/tcmalloc/chromium/src/tcmalloc.cc (left): https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... third_party/tcmalloc/chromium/src/tcmalloc.cc:1175: ASSERT(span != NULL && span->start == p); nit: might as well keep this, so that merges go easier. It has no release perf impact. https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... File third_party/tcmalloc/chromium/src/tcmalloc.cc (right): https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... third_party/tcmalloc/chromium/src/tcmalloc.cc:1161: if (cl == 0) { nit: add comment: // Mimic debug code on done below with pageheap_lock held. https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... third_party/tcmalloc/chromium/src/tcmalloc.cc:1162: // Make sure ptr is inside the first page of the span. nit: pointer https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... third_party/tcmalloc/chromium/src/tcmalloc.cc:1166: "Pointer not pointed to the start of a span"); nit: "Pointer isn't pointing to start of span"
https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... File base/allocator/allocator.gyp (right): https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator.gyp:430: On 2012/05/17 01:36:41, jar wrote: > nit: remove line. Done. https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator.gyp:525: # This is tricky.. We add this include dir so alternate_timer.cc will Will add as a reviewer https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator.gyp:526: # use TCMalloc's logging.h, instead of the intuitional chromium On 2012/05/17 01:36:41, jar wrote: > nit: delete "intuitional" Done. https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator.gyp:527: # base/logging.h as in other building targets. On 2012/05/17 01:36:41, jar wrote: > nit: building-->build Done. https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... File base/allocator/allocator_extension_thunks.cc (right): https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator_extension_thunks.cc:14: // allocator_unittests and TCMalloc tests work on windows. That target has On 2012/05/17 01:36:41, jar wrote: > nit: TCMalloc --> tcmalloc Done. https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator_extension_thunks.cc:15: // to perform/ tcmalloc-specific initialization on windows, but it cannot depend On 2012/05/17 01:36:41, jar wrote: > nit: typo perform/ -->perform Done. https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... File base/allocator/allocator_shim.h (right): https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator_shim.h:17: void* TCMallocDoMallocForTest(size_t size); Did some code search, it appears TCMalloc appears much more times than Tcmalloc in class and function names. So I suggest to keep with TCMalloc for consistency. https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/tcma... File base/allocator/tcmalloc_free_check_test.cc (right): https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/tcma... base/allocator/tcmalloc_free_check_test.cc:16: reinterpret_cast<volatile int*>(TCMallocDoMallocForTest(kMaxSize + 1)); On 2012/05/17 01:36:41, jar wrote: > nit: No need for volatile here. Done. https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/tcma... base/allocator/tcmalloc_free_check_test.cc:17: ASSERT_DEATH(TCMallocDoFreeForTest(const_cast<int*>(p + kAlignment)), On 2012/05/17 01:36:41, jar wrote: > Perhaps try adding small powers of two: 1, 2, 4, 8, ... 1024 Done. https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/tcma... base/allocator/tcmalloc_free_check_test.cc:21: TEST(TCMallocFreeCheck, BadPointerInSubsequentPagesOfTheLargeObject) { On 2012/05/17 01:36:41, jar wrote: > nit: suggest better name: > BadPageAlignedPointerInsideLargeObject Done. https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... File third_party/tcmalloc/chromium/src/tcmalloc.cc (left): https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... third_party/tcmalloc/chromium/src/tcmalloc.cc:1175: ASSERT(span != NULL && span->start == p); On 2012/05/17 01:36:41, jar wrote: > nit: might as well keep this, so that merges go easier. It has no release perf > impact. Done. https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... File third_party/tcmalloc/chromium/src/tcmalloc.cc (right): https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... third_party/tcmalloc/chromium/src/tcmalloc.cc:1161: if (cl == 0) { On 2012/05/17 01:36:41, jar wrote: > nit: add comment: > // Mimic debug code on done below with pageheap_lock held. Done. https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... third_party/tcmalloc/chromium/src/tcmalloc.cc:1162: // Make sure ptr is inside the first page of the span. On 2012/05/17 01:36:41, jar wrote: > nit: pointer Done. https://chromiumcodereview.appspot.com/10391178/diff/4003/third_party/tcmallo... third_party/tcmalloc/chromium/src/tcmalloc.cc:1166: "Pointer not pointed to the start of a span"); On 2012/05/17 01:36:41, jar wrote: > nit: > "Pointer isn't pointing to start of span" Done.
just some nits https://chromiumcodereview.appspot.com/10391178/diff/13003/base/allocator/all... File base/allocator/allocator.gyp (right): https://chromiumcodereview.appspot.com/10391178/diff/13003/base/allocator/all... base/allocator/allocator.gyp:431: # This library is linked in to libbase, allocator_unittests and TCMalloc nit: libbase -> base https://chromiumcodereview.appspot.com/10391178/diff/13003/base/allocator/all... base/allocator/allocator.gyp:432: # tests. It can't depend on either and nothing else should depend on it - If you want to refer to this as tcmalloc tests, the target should be named tcmalloc_tests https://chromiumcodereview.appspot.com/10391178/diff/13003/base/allocator/all... base/allocator/allocator.gyp:523: # use TCMalloc's logging.h, instead of the chromium base/logging.h as say that we don't want to depend on base. (why not?) https://chromiumcodereview.appspot.com/10391178/diff/13003/base/allocator/all... base/allocator/allocator.gyp:526: remove line https://chromiumcodereview.appspot.com/10391178/diff/13003/base/allocator/all... File base/allocator/allocator_extension_thunks.cc (right): https://chromiumcodereview.appspot.com/10391178/diff/13003/base/allocator/all... base/allocator/allocator_extension_thunks.cc:17: // This target sits in the middle - libbase, allocator_unittests and tcmalloc nit: libbase -> base https://chromiumcodereview.appspot.com/10391178/diff/13003/base/allocator/tcm... File base/allocator/tcmalloc_free_check_test.cc (right): https://chromiumcodereview.appspot.com/10391178/diff/13003/base/allocator/tcm... base/allocator/tcmalloc_free_check_test.cc:1: // Copyright 2012 Google Inc. All Rights Reserved. Use chromium header https://chromiumcodereview.appspot.com/10391178/diff/13003/base/allocator/tcm... base/allocator/tcmalloc_free_check_test.cc:4: #include "base/allocator/allocator_shim.h" add a line https://chromiumcodereview.appspot.com/10391178/diff/13003/base/allocator/tcm... base/allocator/tcmalloc_free_check_test.cc:29: // tcmalloc will give a generaal error of invalid pointer. typo generaal https://chromiumcodereview.appspot.com/10391178/diff/13003/third_party/tcmall... File third_party/tcmalloc/chromium/src/tcmalloc.cc (left): https://chromiumcodereview.appspot.com/10391178/diff/13003/third_party/tcmall... third_party/tcmalloc/chromium/src/tcmalloc.cc:1752: *p += 2; // Segv. Does this mean that we can no longer distinguish the case by looking at a crash dump?
http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator.g... base/allocator/allocator.gyp:431: # This library is linked in to libbase, allocator_unittests and TCMalloc On 2012/05/19 01:08:04, rvargas wrote: > nit: libbase -> base Done. http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator.g... base/allocator/allocator.gyp:432: # tests. It can't depend on either and nothing else should depend on it - Done, modified comment and renamed to tcmalloc_unittests http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator.g... base/allocator/allocator.gyp:523: # use TCMalloc's logging.h, instead of the chromium base/logging.h as Maybe I'm wrong. base is defined in base.gypi, I thought the only way to depend on it is to include it into this gyp? They all other targets here will depend on base. http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator.g... base/allocator/allocator.gyp:526: The above comment is only targeting one line of the includes. I think adding this newline will help clarify this? http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator_e... File base/allocator/allocator_extension_thunks.cc (right): http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator_e... base/allocator/allocator_extension_thunks.cc:17: // This target sits in the middle - libbase, allocator_unittests and tcmalloc On 2012/05/19 01:08:04, rvargas wrote: > nit: libbase -> base Done. http://codereview.chromium.org/10391178/diff/13003/base/allocator/tcmalloc_fr... File base/allocator/tcmalloc_free_check_test.cc (right): http://codereview.chromium.org/10391178/diff/13003/base/allocator/tcmalloc_fr... base/allocator/tcmalloc_free_check_test.cc:1: // Copyright 2012 Google Inc. All Rights Reserved. oops.. done http://codereview.chromium.org/10391178/diff/13003/base/allocator/tcmalloc_fr... base/allocator/tcmalloc_free_check_test.cc:4: #include "base/allocator/allocator_shim.h" On 2012/05/19 01:08:04, rvargas wrote: > add a line Done. http://codereview.chromium.org/10391178/diff/13003/base/allocator/tcmalloc_fr... base/allocator/tcmalloc_free_check_test.cc:29: // tcmalloc will give a generaal error of invalid pointer. On 2012/05/19 01:08:04, rvargas wrote: > typo generaal Done. http://codereview.chromium.org/10391178/diff/13003/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/tcmalloc.cc (left): http://codereview.chromium.org/10391178/diff/13003/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/tcmalloc.cc:1752: *p += 2; // Segv. But we can see the back trace in dump right? Can to also see the error log??
The actual byte count in my concern below may not be a big deal.... but when such a pattern is used throughout the binary, the cost becomes very significant. Such costs have already caused us to re-think LOG() vs DLOG() use (as an example). https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... File base/allocator/allocator_shim.h (right): https://chromiumcodereview.appspot.com/10391178/diff/4003/base/allocator/allo... base/allocator/allocator_shim.h:17: void* TCMallocDoMallocForTest(size_t size); ok On 2012/05/19 00:12:27, kaiwang wrote: > Did some code search, it appears TCMalloc appears much more times than Tcmalloc > in class and function names. So I suggest to keep with TCMalloc for consistency. https://chromiumcodereview.appspot.com/10391178/diff/17003/third_party/tcmall... File third_party/tcmalloc/chromium/src/tcmalloc.cc (right): https://chromiumcodereview.appspot.com/10391178/diff/17003/third_party/tcmall... third_party/tcmalloc/chromium/src/tcmalloc.cc:1167: "Pointer is not pointing to the start of a span"); I appreciate that the text of these messages is only here to assure that we have a death-test with a matching log.... but I'm sad that "test code" causes bloat in the release version (re: strings), beyond the if/else execution cost of the CHECK. Is there any way you can propose to not have the text appear in release versions? Maybe there should be yet another flavor of macro based on OFFICIAL_BUILD (or such) that discards the text, and leaves it to us to check stacks to determine the cause of a CHECK. Suggestions?
http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator.g... base/allocator/allocator.gyp:523: # use TCMalloc's logging.h, instead of the chromium base/logging.h as On 2012/05/21 18:11:42, kaiwang wrote: > Maybe I'm wrong. base is defined in base.gypi, I thought the only way to depend > on it is to include it into this gyp? They all other targets here will depend on > base. I'm not sure what you mean by "include it into this gyp". Each target can have different requirements than other targets on the same file. If you want to depend on base, just add another line to the dependencies section (just like this project depends on gtest): '..\base.gyp:base' (note the use of base.gyp, as opposed to gypi). I thought you were trying to avoid that dependency for some reason. http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator.g... base/allocator/allocator.gyp:526: On 2012/05/21 18:11:42, kaiwang wrote: > The above comment is only targeting one line of the includes. I think adding > this newline will help clarify this? That's a good point, I missed that. But then, it is better to have this line before the others, especially because the list should be alpha ordered in any case :) ... and that points me to the other lists of this target: they are not correctly ordered. http://codereview.chromium.org/10391178/diff/13003/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/tcmalloc.cc (left): http://codereview.chromium.org/10391178/diff/13003/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/tcmalloc.cc:1752: *p += 2; // Segv. On 2012/05/21 18:11:42, kaiwang wrote: > But we can see the back trace in dump right? Can to also see the error log?? The stack can be optimized away (although having a log statement probably prevents the optimization, which may not be a good thing by itself) ... and logs are not part of crash dumps. On the other hand, I don't think people know the meaning of these magic values in a crash (I didn't). I've never been a fan of low level libraries logging by themselves (by default). In this particular case, it seems like the log would never be generated ('cause we crash), except in tightly controlled scenarios, and that means it's just added bloat.
https://chromiumcodereview.appspot.com/10391178/diff/17003/base/allocator/all... File base/allocator/allocator.gyp (right): https://chromiumcodereview.appspot.com/10391178/diff/17003/base/allocator/all... base/allocator/allocator.gyp:513: # Part of chromium code(instead of TCMalloc). We don't include the space before open paren https://chromiumcodereview.appspot.com/10391178/diff/17003/base/allocator/all... base/allocator/allocator.gyp:516: '../atomicops_internals_x86_gcc.cc', This is gcc specific? What happens on Windows/Mac? https://chromiumcodereview.appspot.com/10391178/diff/17003/base/allocator/all... base/allocator/allocator.gyp:523: # This is tricky.. We add this include dir so alternate_timer.cc will .. should be . https://chromiumcodereview.appspot.com/10391178/diff/17003/third_party/tcmall... File third_party/tcmalloc/chromium/src/tcmalloc.cc (right): https://chromiumcodereview.appspot.com/10391178/diff/17003/third_party/tcmall... third_party/tcmalloc/chromium/src/tcmalloc.cc:1159: // The span is not in use! A double free? At this point cl could be anything. Don't these checks need to be in the block of code that is executed when you are sure you are about to call Static::pageheap()->Delete(span)? https://chromiumcodereview.appspot.com/10391178/diff/17003/third_party/tcmall... third_party/tcmalloc/chromium/src/tcmalloc.cc:1164: CHECK_CONDITION_PRINT(span->start == p, It seems safe to replace the last two checks with a check for span->start << kPageShift == ptr.
http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator.g... base/allocator/allocator.gyp:523: # use TCMalloc's logging.h, instead of the chromium base/logging.h as You are right. This makes this target a lot more simpler! http://codereview.chromium.org/10391178/diff/13003/base/allocator/allocator.g... base/allocator/allocator.gyp:526: Putting tcmalloc/src before ../.. is because tcmalloc dir must be checked first for header files, or building will fail. But now I depend on base, all these tricky things are not necessary :) http://codereview.chromium.org/10391178/diff/17003/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/10391178/diff/17003/base/allocator/allocator.g... base/allocator/allocator.gyp:516: '../atomicops_internals_x86_gcc.cc', This is removed since this is now depending on base http://codereview.chromium.org/10391178/diff/17003/base/allocator/allocator.g... base/allocator/allocator.gyp:523: # This is tricky.. We add this include dir so alternate_timer.cc will ? which ..? http://codereview.chromium.org/10391178/diff/17003/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/tcmalloc.cc (right): http://codereview.chromium.org/10391178/diff/17003/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/tcmalloc.cc:1159: // The span is not in use! A double free? oops, sorry I accidentally removed a {} pair. These checks should be before ValidateAllocatedRegion, so error message will be the same in debug and release builds (while ValidateAllocatedRegion does some extra check not available in release build). Also moving chromium modified code together is good for later merging http://codereview.chromium.org/10391178/diff/17003/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/tcmalloc.cc:1164: CHECK_CONDITION_PRINT(span->start == p, Done. Good idea http://codereview.chromium.org/10391178/diff/17003/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/tcmalloc.cc:1167: "Pointer is not pointing to the start of a span"); Actually for this specific case, the size will not increase, because CHECK_CONDITION still have a string which is set to the condition parameter. (You can see from above that the length is about the same.. If size of tcmalloc library (release version) is actually an issue, I think we can make some code change to internal_logging.h and logging.h. But might be in another CL :)
LGTM with comment below. Thanks!!!! https://chromiumcodereview.appspot.com/10391178/diff/18002/third_party/tcmall... File third_party/tcmalloc/chromium/src/tcmalloc.cc (right): https://chromiumcodereview.appspot.com/10391178/diff/18002/third_party/tcmall... third_party/tcmalloc/chromium/src/tcmalloc.cc:953: static inline void* CheckedMallocResult(void *result) { This was the real typo: CheckMallocResult https://chromiumcodereview.appspot.com/10391178/diff/18002/third_party/tcmall... third_party/tcmalloc/chromium/src/tcmalloc.cc:1160: // The span is not in use! A double free? nit: Suggest comment: Check to see if object was already freed. https://chromiumcodereview.appspot.com/10391178/diff/18002/third_party/tcmall... third_party/tcmalloc/chromium/src/tcmalloc.cc:1162: "Freeing a span not in use"); nit: Object was not in-use
Thanks for your reviews! I'll try to submit tonight http://codereview.chromium.org/10391178/diff/18002/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/tcmalloc.cc (right): http://codereview.chromium.org/10391178/diff/18002/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/tcmalloc.cc:953: static inline void* CheckedMallocResult(void *result) { On 2012/05/23 00:04:46, jar wrote: > This was the real typo: > > CheckMallocResult Done. http://codereview.chromium.org/10391178/diff/18002/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/tcmalloc.cc:1160: // The span is not in use! A double free? On 2012/05/23 00:04:46, jar wrote: > nit: Suggest comment: > Check to see if object was already freed. Done. http://codereview.chromium.org/10391178/diff/18002/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/tcmalloc.cc:1162: "Freeing a span not in use"); On 2012/05/23 00:04:46, jar wrote: > nit: Object was not in-use Done.
LGTM http://codereview.chromium.org/10391178/diff/18002/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/internal_logging.h (right): http://codereview.chromium.org/10391178/diff/18002/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/internal_logging.h:118: if (!(cond)) { \ Do we have UNLIKELY or EXPECT_FALSE or whatnot defined in config.h? This is a good place for "if (UNLIKELY(...)) ..." (Same comment applies to CHECK_CONDITION.)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaiwang@chromium.org/10391178/18003
Try job failure for 10391178-18003 (retry) (retry) on win_rel for step "runhooks". It's a second try, previously, step "sync_unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaiwang@chromium.org/10391178/18003
Change committed as 138775 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
