Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
247543002:
Add support for more ASAN errors generation to chrome://crash/... (Closed)
|
Created:
6 years, 8 months ago by Sébastien Marchand Modified:
6 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd support for more ASAN errors generation to chrome://crash/...
Adds the following crash url to chrome/chromium builds when SYZYASAN is defined (built with syzyasan=1 in GYP_DEFINES):
heap-corrupted-block
This allows easy validation of the instrumentation/reporting for a given build without having to track down a known/unfixed memory error.
R=chrisha@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265942
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address Chris comments. #
Total comments: 4
Patch Set 3 : Address Chris comments - 2. #
Total comments: 2
Patch Set 4 : Address Chris nit. #Messages
Total messages: 22 (0 generated)
PTAL. jochen@: You're not an owner for content\renderer but jamesr@ is the only owner of this folder and he's gone for a few week... As you're an owner for content\ you should inherit the ownership of content\renderer ?
https://codereview.chromium.org/247543002/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/247543002/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:207: #pragma warning(disable: 4509) Comment as to what warning you are disabling? https://codereview.chromium.org/247543002/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:209: // NOTE(sebmarchand): We intentionally corrupt a memory block her in order to here* in order https://codereview.chromium.org/247543002/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:214: // the function from being instrumented. This way the underflow won't be this* function from being instrumented.
Thanks, PTAnL. https://codereview.chromium.org/247543002/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/247543002/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:207: #pragma warning(disable: 4509) On 2014/04/22 16:28:01, chrisha wrote: > Comment as to what warning you are disabling? Done. https://codereview.chromium.org/247543002/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:209: // NOTE(sebmarchand): We intentionally corrupt a memory block her in order to On 2014/04/22 16:28:01, chrisha wrote: > here* in order Done. https://codereview.chromium.org/247543002/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:214: // the function from being instrumented. This way the underflow won't be On 2014/04/22 16:28:01, chrisha wrote: > this* function from being instrumented. Done.
https://codereview.chromium.org/247543002/diff/20001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/247543002/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:206: // This code trigger a C4509 warning as we're using an object with a destructor triggers* https://codereview.chromium.org/247543002/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:208: // exception and the object's destructor will be called. We'll always catch the exception? Isn't it because no exception will ever actually be thrown?
Thanks, one last look ? https://codereview.chromium.org/247543002/diff/20001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/247543002/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:206: // This code trigger a C4509 warning as we're using an object with a destructor On 2014/04/22 17:25:44, chrisha wrote: > triggers* Done. https://codereview.chromium.org/247543002/diff/20001/content/renderer/render_... content/renderer/render_frame_impl.cc:208: // exception and the object's destructor will be called. On 2014/04/22 17:25:44, chrisha wrote: > We'll always catch the exception? Isn't it because no exception will ever > actually be thrown? Oh right sorry ! (I had the wrong logic in mind...)
lgtm with one last nit... let me know if you can't find an owner in a reasonable time frame, and we'll land it TBR. https://codereview.chromium.org/247543002/diff/40001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/247543002/diff/40001/content/renderer/render_... content/renderer/render_frame_impl.cc:239: static const char kHeapCorruptedBlock[] = "/heap-corrupted-block"; Heap-corrupted sounds kind of like the heap itself corrupted the block. Maybe 'corrupt-heap-block' would be better?
Thanks, I'll wait to see if jochen@ can TBR this otherwise we'll TBR it. https://codereview.chromium.org/247543002/diff/40001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/247543002/diff/40001/content/renderer/render_... content/renderer/render_frame_impl.cc:239: static const char kHeapCorruptedBlock[] = "/heap-corrupted-block"; On 2014/04/22 17:52:20, chrisha wrote: > Heap-corrupted sounds kind of like the heap itself corrupted the block. Maybe > 'corrupt-heap-block' would be better? Done.
I meant: I'll wait to see if jochen@ can *LGTM* this...
lgtm
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/247543002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/247543002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/247543002/60001
Message was sent while issue was closed.
Change committed as 265942
Message was sent while issue was closed.
On 2014/04/24 16:49:51, I haz the power (commit-bot) wrote: > Change committed as 265942 Debug Win Build causes compilation error: d:\src\w\crbk\src\content\renderer\render_frame_impl.cc(226) : error C2712: Cannot use __try in functions that require object unwinding
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/250793002/ by sebmarchand@chromium.org. The reason for reverting is: This break the debug build.. |
