Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2588133003:
[Predator] Add clusterfuzz callstack filters. (Closed)
|
Created:
4 years ago by Sharu Jiang Modified:
4 years ago CC:
chromium-reviews, infra-reviews+infra_chromium.org, inferno Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Predator] Add predator callstack filters.
BUG=
TBR=stgao@chromium.org
Review-Url: https://codereview.chromium.org/2588133003
Committed: https://chromium.googlesource.com/infra/infra/+/c416faa59d11063703c077a3657192b0bb7423bd
Patch Set 1 #
Total comments: 59
Patch Set 2 : Fix nits. #Patch Set 3 : Rebase. #
Messages
Total messages: 15 (7 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Predator] Add predator callstack filters. BUG= ========== to ========== [Predator] Add predator callstack filters. BUG= TBR=stgao@chromium.org ==========
katesonia@chromium.org changed reviewers: + mbarbella@chromium.org, stgao@chromium.org, wrengr@chromium.org
PTAL.
lgtm with nits https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... File appengine/findit/crash/callstack_filters.py (right): https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:16: # When checking the null pointer deference, any deference of a address within "deference" -> "dereference" "a address" -> "an address" https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:17: # the threshold is considered as null pointer deference. Be consistent with ditto https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:84: class FilterJavaJreSdkFrames(CallStackFilter): It's inconsistent to lowercase JRE and SDK here when we keep acronyms like JIT uppercased. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:116: """Filter all v8 frames if conditions met. should clarify whether it's all conditions or any condition. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:127: crash_address: Address where crash happens. type? https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:207: lambda f: not V8_DEP_PATH_MARKER in f.dep_path, stack_buffer.frames) The syntax "x not in xs" is more standard than "not x in xs" https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... File appengine/findit/crash/test/callstack_filters_test.py (right): https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:44: """Tests ``KeepTopNFrames`` works as expected.""" "tests ``..." -> "tests that ``..." https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:62: """Tests if top_n_frames is None, filter does nothing.""" "tests if" -> "tests that if" https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:76: """Tests ``FilterJavaJreSdkFrames`` works as expected.""" "tests ``..." -> "tests that ``..." https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:79: """Tests the filter does nothing for non-java stack.""" "tests the" -> "tests that the" https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:109: """Tests ``KeepV8FramesIfV8GeneratedJITCrash`` works as expected.""" "tests ``..." -> "tests that ``..." https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:112: """Tests ``KeepV8FramesIfV8GeneratedJITCrash`` keeps v8 frames.""" "tests ``..." -> "tests that ``..." https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:132: """Tests filter does nothing for non v8 generated JIT crash.""" "tests filter" -> tests that filter(ing)" https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:148: """Tests ``FilterV8FramesForV8APIBindingCode`` works as expected.""" "tests ``..." -> "tests that ``..." https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:151: """Tests filtering nothing if the stack has less than two frames.""" "tests filtering nothing" -> "tests that filter(ing) does nothing" https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:164: """Tests filtering V8 frames form V8 binding blink code.""" "form" -> from" https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:214: """Tests filtering nothing for non v8 api binding stack.""" "tests filtering nothing" -> "tests that filter(ing) does nothing" https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:230: """Tests ``FilterFramesAfterBlinkGeneratedCode`` works as expected.""" "tests ``..." -> "tests that ``..." https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:233: """Tests filtering nothing if there is no blink binding generated code.""" "tests filtering nothing" -> "tests that filter(ing) does nothing" https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:247: """Tests filtering nothing if there is no blink binding generated code.""" "tests filtering nothing" -> "tests that filter(ing) does nothing" https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:266: """Tests ``FilterV8FramesIfV8NotInTopFrames`` works as expected.""" "tests ``..." -> "tests that ``..." https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:269: """Tests filtering v8 frames if there is no v8 frame in top frames.""" unclear https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:288: """Tests filtering nothing if there is v8 frame in top frames.""" "tests filtering nothing" -> "tests that filter(ing) does nothing" https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:306: """Tests filtering nothing for empty stack buffer.""" "tests filtering nothing" -> "tests that filter(ing) does nothing"
lgtm with nits. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... File appengine/findit/crash/callstack_filters.py (right): https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:22: V8_API_CC_FILE_PATH = 'src/api.cc' Could we make a list instead of two separate files? https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:85: """Filter out package names from Java JRE/SDK. nit: "Filter" -> "Filters" Same for other occurrences below. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:124: the crash (likely a bindings issue). nit: Could we fill in the whole line before moving to the next line? https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:129: Returns: What is the Returns for? I'm confused here. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:183: break What if there are two frames with generated code?
https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... File appengine/findit/crash/callstack_filters.py (right): https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:16: # When checking the null pointer deference, any deference of a address within On 2016/12/19 23:51:05, wrengr wrote: > "deference" -> "dereference" > > "a address" -> "an address" Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:17: # the threshold is considered as null pointer deference. Be consistent with On 2016/12/19 23:51:05, wrengr wrote: > ditto Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:22: V8_API_CC_FILE_PATH = 'src/api.cc' On 2016/12/20 19:07:36, stgao (slow on Monday) wrote: > Could we make a list instead of two separate files? Actually making it seperate would make the implementation shorter, or we will need to write for loop and it takes more lines... https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:84: class FilterJavaJreSdkFrames(CallStackFilter): On 2016/12/19 23:51:05, wrengr wrote: > It's inconsistent to lowercase JRE and SDK here when we keep acronyms like JIT > uppercased. This is because if it is ``FilterJavaJRESDKFrames``, it will be hard to separate ``JRESDK``, or if I added And in it, ``FilterJavaJREAndSDKFrames`` is a bit strange... Any good naming idea? https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:85: """Filter out package names from Java JRE/SDK. On 2016/12/20 19:07:36, stgao (slow on Monday) wrote: > nit: "Filter" -> "Filters" > > Same for other occurrences below. Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:116: """Filter all v8 frames if conditions met. On 2016/12/19 23:51:05, wrengr wrote: > should clarify whether it's all conditions or any condition. Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:124: the crash (likely a bindings issue). On 2016/12/20 19:07:36, stgao (slow on Monday) wrote: > nit: Could we fill in the whole line before moving to the next line? Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:127: crash_address: Address where crash happens. On 2016/12/19 23:51:05, wrengr wrote: > type? Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:129: Returns: On 2016/12/20 19:07:36, stgao (slow on Monday) wrote: > What is the Returns for? I'm confused here. Oops, this should be removed. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:183: break On 2016/12/20 19:07:36, stgao (slow on Monday) wrote: > What if there are two frames with generated code? This is to filter generated frames and frames after them, so we should break after we found the first generated code frame. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:207: lambda f: not V8_DEP_PATH_MARKER in f.dep_path, stack_buffer.frames) On 2016/12/19 23:51:05, wrengr wrote: > The syntax "x not in xs" is more standard than "not x in xs" Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... File appengine/findit/crash/test/callstack_filters_test.py (right): https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:44: """Tests ``KeepTopNFrames`` works as expected.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests ``..." -> "tests that ``..." Done. I omitted the "that" to save space, guess I shouldn't do that... https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:62: """Tests if top_n_frames is None, filter does nothing.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests if" -> "tests that if" Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:76: """Tests ``FilterJavaJreSdkFrames`` works as expected.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests ``..." -> "tests that ``..." Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:79: """Tests the filter does nothing for non-java stack.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests the" -> "tests that the" Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:109: """Tests ``KeepV8FramesIfV8GeneratedJITCrash`` works as expected.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests ``..." -> "tests that ``..." Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:112: """Tests ``KeepV8FramesIfV8GeneratedJITCrash`` keeps v8 frames.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests ``..." -> "tests that ``..." Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:132: """Tests filter does nothing for non v8 generated JIT crash.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests filter" -> tests that filter(ing)" Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:148: """Tests ``FilterV8FramesForV8APIBindingCode`` works as expected.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests ``..." -> "tests that ``..." Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:151: """Tests filtering nothing if the stack has less than two frames.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests filtering nothing" -> "tests that filter(ing) does nothing" Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:164: """Tests filtering V8 frames form V8 binding blink code.""" On 2016/12/19 23:51:06, wrengr wrote: > "form" -> from" Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:214: """Tests filtering nothing for non v8 api binding stack.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests filtering nothing" -> "tests that filter(ing) does nothing" Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:233: """Tests filtering nothing if there is no blink binding generated code.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests filtering nothing" -> "tests that filter(ing) does nothing" Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:247: """Tests filtering nothing if there is no blink binding generated code.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests filtering nothing" -> "tests that filter(ing) does nothing" Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:266: """Tests ``FilterV8FramesIfV8NotInTopFrames`` works as expected.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests ``..." -> "tests that ``..." Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:269: """Tests filtering v8 frames if there is no v8 frame in top frames.""" On 2016/12/19 23:51:06, wrengr wrote: > unclear I tried to explain this in one line... it seems that doesn't work very well. Separated it into several lines. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:288: """Tests filtering nothing if there is v8 frame in top frames.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests filtering nothing" -> "tests that filter(ing) does nothing" Done. https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/test/callstack_filters_test.py:306: """Tests filtering nothing for empty stack buffer.""" On 2016/12/19 23:51:06, wrengr wrote: > "tests filtering nothing" -> "tests that filter(ing) does nothing" Done.
https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... File appengine/findit/crash/callstack_filters.py (right): https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:84: class FilterJavaJreSdkFrames(CallStackFilter): On 2016/12/22 01:59:43, Sharu Jiang wrote: > On 2016/12/19 23:51:05, wrengr wrote: > > It's inconsistent to lowercase JRE and SDK here when we keep acronyms like JIT > > uppercased. > > This is because if it is ``FilterJavaJRESDKFrames``, it will be hard to separate > ``JRESDK``, or if I added And in it, ``FilterJavaJREAndSDKFrames`` is a bit > strange... > > Any good naming idea? yeah, JRESDK is hard to read too (though slightly less hard, imo). Don't have any good suggestions, alas
https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... File appengine/findit/crash/callstack_filters.py (right): https://codereview.chromium.org/2588133003/diff/20001/appengine/findit/crash/... appengine/findit/crash/callstack_filters.py:84: class FilterJavaJreSdkFrames(CallStackFilter): On 2016/12/22 23:31:51, wrengr wrote: > On 2016/12/22 01:59:43, Sharu Jiang wrote: > > On 2016/12/19 23:51:05, wrengr wrote: > > > It's inconsistent to lowercase JRE and SDK here when we keep acronyms like > JIT > > > uppercased. > > > > This is because if it is ``FilterJavaJRESDKFrames``, it will be hard to > separate > > ``JRESDK``, or if I added And in it, ``FilterJavaJREAndSDKFrames`` is a bit > > strange... > > > > Any good naming idea? > > yeah, JRESDK is hard to read too (though slightly less hard, imo). Don't have > any good suggestions, alas Ok... let's leave it as it is then.
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2588133003/#ps60001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482453775410550,
"parent_rev": "e47b1bfb1b44f13037fd98207c42bd8b7a703749", "commit_rev":
"c416faa59d11063703c077a3657192b0bb7423bd"}
Message was sent while issue was closed.
Description was changed from ========== [Predator] Add predator callstack filters. BUG= TBR=stgao@chromium.org ========== to ========== [Predator] Add predator callstack filters. BUG= TBR=stgao@chromium.org Review-Url: https://codereview.chromium.org/2588133003 Committed: https://chromium.googlesource.com/infra/infra/+/c416faa59d11063703c077a365719... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/infra/infra/+/c416faa59d11063703c077a365719... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
