Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
430943003:
[Findit] Plain objects to represent and parse stack trace. (Closed)
Description[Findit] Plain objects to represent and parse stack trace.
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288270
Patch Set 1 #
Total comments: 60
Patch Set 2 : addressed code review and added crash_utils file #Patch Set 3 : changed wrong reference to stack frame #
Total comments: 4
Patch Set 4 : Added functionality in stacktrace to easily add new parse rules for different chrome builds #
Total comments: 40
Patch Set 5 : Fixed comments and changed variable names #
Total comments: 26
Patch Set 6 : Addressed code review and changed the stacktrace parsing logic to correctly look at the type of cal… #
Total comments: 48
Patch Set 7 : addressed codereview. #
Total comments: 9
Patch Set 8 : addressed codereview #Patch Set 9 : reupload #
Messages
Total messages: 23 (0 generated)
Hi Jason, Thanks for improving the code style. It is much better now. However, I still have many comments for this patch. Some are code style. But more are on design. Once you address my comments, I believe your CL would be good to commit. Thanks, Shuotao Gao https://codereview.chromium.org/430943003/diff/1/tools/findit/README File tools/findit/README (right): https://codereview.chromium.org/430943003/diff/1/tools/findit/README#newcode1 tools/findit/README:1: Usage: python findit.py key, where key is a testcase key in cluster-fuzz. In order to use the script, a config file containing API key from cluster-fuzz must be in the same folder as the script. A new API key can be retrieved from https://cluster-fuzz.appspot.com/#help. How about leaving this file in the fourth patch? The findit.py is not in this patch yet. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... File tools/findit/component_dictionary.py (right): https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:9: It maps each component (blink, chrome, etc) to a component dictionary. component dictionary? or should be file dictionary? https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:15: self.components = components Do we need to save |components| here? It could be derived from component_dic.keys() https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:19: self.component_dic[component] = FileDictionary() As FileDictionary is used by ComponentDictionary, FileDictionary should go first. Same for classes in stacktrace.py https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:25: """Returns a component dictionary for a given component.""" FileDictionary or component dictionary? https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:28: def FromStacktrace(self, stack_list): |stack_list| is misleading here. Should it be |stack_frame_list| or |stack_frames|? https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:32: This empty line could be removed. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:34: # crash, ignore this line How do you know this here? https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:36: if component not in self.components: we could do below instead: if component not in self.component_dic: https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:68: file_name: name of the crashed file I think file_name is not needed, as it could be derived from file_path by os.path.basename(file_path) https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:74: Empty line is not needed. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:97: def GetPathDic(self, file_name): For these Get functions here, could we just use file_path? Because file_name is derivable from it. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:6: import findit_for_crash_utils as findit_utils How about adding a file crash_utils.py? And include the needed findit_utils.NormalizePathLinux in it for the moment? https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:9: class Stacktrace(object): Rename to |StacktraceInfo|? Because it contains multiple stack traces. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:20: """Parses stacktrace and returns normalized stacktrace. We do not return anything within this function actually. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:22: If there are multiple stack frames within the stacktrace, What's the 'stack frame' here? Does it refer to a call stack of a thread? https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:43: # Reset the stack list, and assume we start from main thread # Comments end with a period like this. Please also check all comments in all files. This style issue appears in many places. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:57: stack_frame_index = int(parts[0][1:]) Use regex to get the stack_frame_index instead of a splitting here. stack_frame_index_pattern = re.compile(r'#(\d+)') match = stack_frame_index_pattern.match(line) if not match: continue stack_frame_index = match.group(1) https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:63: # If this frame is from main thread frame? https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:64: if not seen_zero: Could |seen_zero| be renamed to |seen_main_thread|? https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:93: self.stack_list.append(current_stack) I think different builds (asan, lsan, tsan, msan) will have different format of stacktrace output. Would you mind making this function as a separate class SanitizerStackTraceParser? We could also add an interface like below and make SanitizerStackTraceParser implement it: class StackTraceParser(object): def support(self, chrome_build_type): # Return True if the build type is supported by this parser, otherwise return False. raise NotImplemented() def parse(self, stack_trace_string): # Parse the stack trace, and return an instance of StackTraceInfo. raise NotImplemented() The design pattern here is chain of responsibility. It is extendable if we want to support more types of stack traces. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:96: """Extracts information from stacktrace. stacktrace seems misused here. Should be stack frame, right? https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:110: crash_component = parts[-2] The way we guess |crash_component| here is hacky. Would you mind adding a TODO for improvement later by integrating with the DEPS file? https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:144: self.line_list = [] |line_list| -> |frames| or |frame_list|? https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:150: def GetFirstN(self, n): check out of boundary? https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:155: """Represents a line in stacktrace. We refer this as stack frames. Maybe renaming it to StackFrame and |stack_frame_index| to |index|? https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:160: file_name: name of the file that crashed |file_name| could be derived from |file_path|. Could add a property function which returns the file_name from the file_path. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:163: crashed_line: line of the file that caused the crash Will |line_number| be more clear? https://codereview.chromium.org/430943003/diff/1/tools/findit/urls.config File tools/findit/urls.config (right): https://codereview.chromium.org/430943003/diff/1/tools/findit/urls.config#new... tools/findit/urls.config:1: [svn_chromium] As this file is not used yet, maybe include it in a later patch that uses it?
If some comments are not clear to you, please feel free to drop by my desk and ask questions :)
I also update the title and description. For later CLs, please use a more clear description.
https://codereview.chromium.org/430943003/diff/1/tools/findit/README File tools/findit/README (right): https://codereview.chromium.org/430943003/diff/1/tools/findit/README#newcode1 tools/findit/README:1: Usage: python findit.py key, where key is a testcase key in cluster-fuzz. In order to use the script, a config file containing API key from cluster-fuzz must be in the same folder as the script. A new API key can be retrieved from https://cluster-fuzz.appspot.com/#help. On 2014/07/31 01:52:23, Shuotao wrote: > How about leaving this file in the fourth patch? > The findit.py is not in this patch yet. Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... File tools/findit/component_dictionary.py (right): https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:9: It maps each component (blink, chrome, etc) to a component dictionary. On 2014/07/31 01:52:24, Shuotao wrote: > component dictionary? or should be file dictionary? Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:15: self.components = components On 2014/07/31 01:52:23, Shuotao wrote: > Do we need to save |components| here? > > It could be derived from component_dic.keys() Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:19: self.component_dic[component] = FileDictionary() On 2014/07/31 01:52:23, Shuotao wrote: > As FileDictionary is used by ComponentDictionary, FileDictionary should go > first. > > Same for classes in stacktrace.py Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:25: """Returns a component dictionary for a given component.""" On 2014/07/31 01:52:24, Shuotao wrote: > FileDictionary or component dictionary? Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:28: def FromStacktrace(self, stack_list): On 2014/07/31 01:52:24, Shuotao wrote: > |stack_list| is misleading here. > Should it be |stack_frame_list| or |stack_frames|? Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:32: On 2014/07/31 01:52:23, Shuotao wrote: > This empty line could be removed. Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:34: # crash, ignore this line On 2014/07/31 01:52:24, Shuotao wrote: > How do you know this here? Updated the comment to explain this in a better way https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:36: if component not in self.components: On 2014/07/31 01:52:24, Shuotao wrote: > we could do below instead: > > if component not in self.component_dic: Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:68: file_name: name of the crashed file On 2014/07/31 01:52:23, Shuotao wrote: > I think file_name is not needed, as it could be derived from file_path by > os.path.basename(file_path) I think because stacktrace line already has file_name as one of its fields, it might be OK to just pass it in instead of recalculating it. If we want to remove file name overall, I'm not sure if it is better to calculate every time or to just pass it as an argument. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:74: On 2014/07/31 01:52:23, Shuotao wrote: > Empty line is not needed. Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:97: def GetPathDic(self, file_name): On 2014/07/31 01:52:23, Shuotao wrote: > For these Get functions here, could we just use file_path? Because file_name is > derivable from it. Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:6: import findit_for_crash_utils as findit_utils On 2014/07/31 01:52:24, Shuotao wrote: > How about adding a file crash_utils.py? And include the needed > findit_utils.NormalizePathLinux in it for the moment? Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:9: class Stacktrace(object): On 2014/07/31 01:52:24, Shuotao wrote: > Rename to |StacktraceInfo|? Because it contains multiple stack traces. I named it stacktrace because this object exists one for release build stacktrace and one for debug build stacktrace, so it would be natural to call it just stacktrace. Also I'm not sure if adding Info would imply that this object is a list of callstacks. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:20: """Parses stacktrace and returns normalized stacktrace. On 2014/07/31 01:52:24, Shuotao wrote: > We do not return anything within this function actually. Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:22: If there are multiple stack frames within the stacktrace, On 2014/07/31 01:52:24, Shuotao wrote: > What's the 'stack frame' here? Does it refer to a call stack of a thread? changed the description https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:43: # Reset the stack list, and assume we start from main thread On 2014/07/31 01:52:24, Shuotao wrote: > # Comments end with a period like this. > > Please also check all comments in all files. > This style issue appears in many places. Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:57: stack_frame_index = int(parts[0][1:]) On 2014/07/31 01:52:25, Shuotao wrote: > Use regex to get the stack_frame_index instead of a splitting here. > > stack_frame_index_pattern = re.compile(r'#(\d+)') > match = stack_frame_index_pattern.match(line) > if not match: > continue > > stack_frame_index = match.group(1) Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:63: # If this frame is from main thread On 2014/07/31 01:52:24, Shuotao wrote: > frame? Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:64: if not seen_zero: On 2014/07/31 01:52:24, Shuotao wrote: > Could |seen_zero| be renamed to |seen_main_thread|? Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:93: self.stack_list.append(current_stack) On 2014/07/31 01:52:24, Shuotao wrote: > I think different builds (asan, lsan, tsan, msan) will have different format of > stacktrace output. > > Would you mind making this function as a separate class > SanitizerStackTraceParser? > > We could also add an interface like below and make SanitizerStackTraceParser > implement it: > > class StackTraceParser(object): > > def support(self, chrome_build_type): > # Return True if the build type is supported by this parser, otherwise > return False. > raise NotImplemented() > > def parse(self, stack_trace_string): > # Parse the stack trace, and return an instance of StackTraceInfo. > raise NotImplemented() > > > The design pattern here is chain of responsibility. > It is extendable if we want to support more types of stack traces. I agree that something needs to be done about this file, but I think it might be a bit overkill. In order to handle multiple builds, we only need different rules for extracting file path/line/function from a line. So maybe create bunch of classes with rules on how to parse a line for certain builds, instead of classes to parse the whole stacktrace? https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:96: """Extracts information from stacktrace. On 2014/07/31 01:52:25, Shuotao wrote: > stacktrace seems misused here. Should be stack frame, right? Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:110: crash_component = parts[-2] On 2014/07/31 01:52:24, Shuotao wrote: > The way we guess |crash_component| here is hacky. > Would you mind adding a TODO for improvement later by integrating with the DEPS > file? This is just a totally mis named variable. changed the name. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:144: self.line_list = [] On 2014/07/31 01:52:25, Shuotao wrote: > |line_list| -> |frames| or |frame_list|? Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:150: def GetFirstN(self, n): On 2014/07/31 01:52:24, Shuotao wrote: > check out of boundary? Python automatically checks for boundary when using slicing. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:155: """Represents a line in stacktrace. On 2014/07/31 01:52:25, Shuotao wrote: > We refer this as stack frames. Maybe renaming it to StackFrame and > |stack_frame_index| to |index|? Done. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:160: file_name: name of the file that crashed On 2014/07/31 01:52:24, Shuotao wrote: > |file_name| could be derived from |file_path|. Could add a property function > which returns the file_name from the file_path. Same as the comment in component_dictionary file. https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:163: crashed_line: line of the file that caused the crash On 2014/07/31 01:52:25, Shuotao wrote: > Will |line_number| be more clear? changed to crashed_line_number https://codereview.chromium.org/430943003/diff/1/tools/findit/urls.config File tools/findit/urls.config (right): https://codereview.chromium.org/430943003/diff/1/tools/findit/urls.config#new... tools/findit/urls.config:1: [svn_chromium] On 2014/07/31 01:52:25, Shuotao wrote: > As this file is not used yet, maybe include it in a later patch that uses it? Done.
sorry, found out i missed some stack frame reference in comments.
https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/1/tools/findit/stacktrace.py#n... tools/findit/stacktrace.py:9: class Stacktrace(object): On 2014/07/31 18:41:14, jeun wrote: > On 2014/07/31 01:52:24, Shuotao wrote: > > Rename to |StacktraceInfo|? Because it contains multiple stack traces. > I named it stacktrace because this object exists one for release build > stacktrace and one for debug build stacktrace, so it would be natural to call it > just stacktrace. Also I'm not sure if adding Info would imply that this object > is a list of callstacks. Sound good. In that case, the comment "Contains release build stacktrace and debug build stacktrace" might need updating. https://codereview.chromium.org/430943003/diff/40001/tools/findit/component_d... File tools/findit/component_dictionary.py (right): https://codereview.chromium.org/430943003/diff/40001/tools/findit/component_d... tools/findit/component_dictionary.py:4: import os Usually we leave a blank line between code and the header. https://codereview.chromium.org/430943003/diff/40001/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/40001/tools/findit/stacktrace.... tools/findit/stacktrace.py:40: seen_main_thread = False reach_new_callstack? https://codereview.chromium.org/430943003/diff/40001/tools/findit/stacktrace.... tools/findit/stacktrace.py:44: # match the line to see if it is a valid stack frame nit: comment style https://codereview.chromium.org/430943003/diff/40001/tools/findit/stacktrace.... tools/findit/stacktrace.py:90: def ExtractFromStackFrame(self, line, chrome_build): Used within class only. Add underscore before function name.
Below are stacktrace formats we want to support in the end. Please find a way to make it easy to support new format of the stacktrace for various build types. https://cluster-fuzz.appspot.com/testcase?key=5724692910440448 https://cluster-fuzz.appspot.com/testcase?key=6434453301755904 https://cluster-fuzz.appspot.com/testcase?key=6412275298598912 https://cluster-fuzz.appspot.com/testcase?key=5538230126510080 https://cluster-fuzz.appspot.com/testcase?key=5637162789765120 https://cluster-fuzz.appspot.com/testcase?key=6253282185969664 https://cluster-fuzz.appspot.com/testcase?key=5633621887025152 http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
Hi Jason, This CL is quite good now. Once you address my comments, please go ahead and ask CF folks for review. Thanks, Shuotao Gao https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... File tools/findit/component_dictionary.py (right): https://codereview.chromium.org/430943003/diff/1/tools/findit/component_dicti... tools/findit/component_dictionary.py:68: file_name: name of the crashed file On 2014/07/31 18:41:14, jeun wrote: > On 2014/07/31 01:52:23, Shuotao wrote: > > I think file_name is not needed, as it could be derived from file_path by > > os.path.basename(file_path) > I think because stacktrace line already has file_name as one of its fields, it > might be OK to just pass it in instead of recalculating it. If we want to remove > file name overall, I'm not sure if it is better to calculate every time or to > just pass it as an argument. OK, that's fine. It is a problem of time and memory. https://codereview.chromium.org/430943003/diff/60001/tools/findit/component_d... File tools/findit/component_dictionary.py (right): https://codereview.chromium.org/430943003/diff/60001/tools/findit/component_d... tools/findit/component_dictionary.py:96: def FromStacktrace(self, stack_frame_list): Could you please give a better name for this function? It seems not that clear. https://codereview.chromium.org/430943003/diff/60001/tools/findit/component_d... tools/findit/component_dictionary.py:99: for stackframe in stack_frame_list: stackframe -> stack_frame? Let's make it consistent in the code. https://codereview.chromium.org/430943003/diff/60001/tools/findit/crash_utils.py File tools/findit/crash_utils.py (right): https://codereview.chromium.org/430943003/diff/60001/tools/findit/crash_utils... tools/findit/crash_utils.py:10: def NormalizePathLinux(path): Could we include this function only in this patch? As other functions are not used yet, I'm not sure whether they are needed and how they could be improved or the like. https://codereview.chromium.org/430943003/diff/60001/tools/findit/crash_utils... tools/findit/crash_utils.py:21: # TODO(jeun): integrate with parsing DEPS file style issue: comment. https://codereview.chromium.org/430943003/diff/60001/tools/findit/crash_utils... tools/findit/crash_utils.py:104: for i in range(10): For the retry, it might be better to make it as a parameter with a default value. https://codereview.chromium.org/430943003/diff/60001/tools/findit/crash_utils... tools/findit/crash_utils.py:116: time.sleep(0.1) Sleep interval could be parameterized too. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:1: # Copyright (c) 2014 The Chromium Authors. All rights reserved. It might be better to re-order the classes in this file, as I pointed out in my code review. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:7: import crash_utils One blank line between groups of imports. For grouping, please refer to http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Imports_format... https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:21: def ParseStacktrace(self, stacktrace, chrome_build): I'm OK if you don't want to create a parser class for the parsing logic. If CF folks point it out, then we'd better do that as in the comment of my first code review. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:30: chrome_build: a string containing the job type of the crash Could we reword "job type" to "build type"? Same for other occurrences. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:49: is_new_callstack = self.CheckIfNewStackFrame(line, chrome_build) "CheckIfNewStackFrame" is not that clear. Based on the name, we are looking for a new stack frame. However, we are checking for a new stack. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:52: # If this callstack is from main thread, update the boolean. Comment needs updating. Please make sure comments are up-to-date after you update code during code review. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:86: self.stack_list.append(current_stack) bug: if there is no frame found, an empty stack (initialized in line 45) will be added. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:88: def CheckIfNewStackFrame(self, line, chrome_build): Will this function be used outside of this class? https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:89: """Check if this line is the start of the new stack frame. why new stack frame? https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:91: Since each builds have different syntax of stacktrace, the logic for syntax -> format? https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:117: # If this line does not represent the crashing information, continue. continue? https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:136: the line of crash line content or line number? https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:164: def GetStackFromMainThread(self): Is it true that the first callstack is from the main thread for all build types? https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:187: """Represents a line in stacktrace. "a frame in callstack"?
I have addressed comments from Shoutao and added blocks/comments throughout the code. Please review my code. Thanks! https://codereview.chromium.org/430943003/diff/60001/tools/findit/component_d... File tools/findit/component_dictionary.py (right): https://codereview.chromium.org/430943003/diff/60001/tools/findit/component_d... tools/findit/component_dictionary.py:96: def FromStacktrace(self, stack_frame_list): On 2014/08/01 17:22:20, Shuotao wrote: > Could you please give a better name for this function? It seems not that clear. Done. https://codereview.chromium.org/430943003/diff/60001/tools/findit/component_d... tools/findit/component_dictionary.py:99: for stackframe in stack_frame_list: On 2014/08/01 17:22:20, Shuotao wrote: > stackframe -> stack_frame? > Let's make it consistent in the code. Done. https://codereview.chromium.org/430943003/diff/60001/tools/findit/crash_utils.py File tools/findit/crash_utils.py (right): https://codereview.chromium.org/430943003/diff/60001/tools/findit/crash_utils... tools/findit/crash_utils.py:10: def NormalizePathLinux(path): On 2014/08/01 17:22:20, Shuotao wrote: > Could we include this function only in this patch? > > As other functions are not used yet, I'm not sure whether they are needed and > how they could be improved or the like. I am not sure what is the best way of doing this, since all other commits (parser, match etc) also use this utils file. Is it OK to add this file to all CLs? https://codereview.chromium.org/430943003/diff/60001/tools/findit/crash_utils... tools/findit/crash_utils.py:21: # TODO(jeun): integrate with parsing DEPS file On 2014/08/01 17:22:20, Shuotao wrote: > style issue: comment. Done. https://codereview.chromium.org/430943003/diff/60001/tools/findit/crash_utils... tools/findit/crash_utils.py:104: for i in range(10): On 2014/08/01 17:22:20, Shuotao wrote: > For the retry, it might be better to make it as a parameter with a default > value. Done. https://codereview.chromium.org/430943003/diff/60001/tools/findit/crash_utils... tools/findit/crash_utils.py:116: time.sleep(0.1) On 2014/08/01 17:22:20, Shuotao wrote: > Sleep interval could be parameterized too. Done. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:1: # Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/08/01 17:22:21, Shuotao wrote: > It might be better to re-order the classes in this file, as I pointed out in my > code review. Reorder the classes as in reorder the order of imports? https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:7: import crash_utils On 2014/08/01 17:22:21, Shuotao wrote: > One blank line between groups of imports. > For grouping, please refer to > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Imports_format... Done. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:21: def ParseStacktrace(self, stacktrace, chrome_build): On 2014/08/01 17:22:21, Shuotao wrote: > I'm OK if you don't want to create a parser class for the parsing logic. > > If CF folks point it out, then we'd better do that as in the comment of my first > code review. Acknowledged. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:30: chrome_build: a string containing the job type of the crash On 2014/08/01 17:22:21, Shuotao wrote: > Could we reword "job type" to "build type"? Same for other occurrences. Done. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:49: is_new_callstack = self.CheckIfNewStackFrame(line, chrome_build) On 2014/08/01 17:22:21, Shuotao wrote: > "CheckIfNewStackFrame" is not that clear. Based on the name, we are looking for > a new stack frame. However, we are checking for a new stack. Done. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:52: # If this callstack is from main thread, update the boolean. On 2014/08/01 17:22:21, Shuotao wrote: > Comment needs updating. > > Please make sure comments are up-to-date after you update code during code > review. I think this comment is correct. If it is new call stack and we have not reached the new callstack, it means that this callstack is from main thread. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:86: self.stack_list.append(current_stack) On 2014/08/01 17:22:21, Shuotao wrote: > bug: if there is no frame found, an empty stack (initialized in line 45) will be > added. Done. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:88: def CheckIfNewStackFrame(self, line, chrome_build): On 2014/08/01 17:22:21, Shuotao wrote: > Will this function be used outside of this class? Renamed with two underscores. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:89: """Check if this line is the start of the new stack frame. On 2014/08/01 17:22:21, Shuotao wrote: > why new stack frame? changed to callstack https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:91: Since each builds have different syntax of stacktrace, the logic for On 2014/08/01 17:22:21, Shuotao wrote: > syntax -> format? Done. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:117: # If this line does not represent the crashing information, continue. On 2014/08/01 17:22:20, Shuotao wrote: > continue? changed to return false. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:136: the line of crash On 2014/08/01 17:22:20, Shuotao wrote: > line content or line number? changed to crashed line number. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:164: def GetStackFromMainThread(self): On 2014/08/01 17:22:21, Shuotao wrote: > Is it true that the first callstack is from the main thread for all build types? I believe this is a valid assumption, from the stacktraces I have seen. https://codereview.chromium.org/430943003/diff/60001/tools/findit/stacktrace.... tools/findit/stacktrace.py:187: """Represents a line in stacktrace. On 2014/08/01 17:22:21, Shuotao wrote: > "a frame in callstack"? Done.
In general it looks pretty good. Most of these comments are minor nits, though a few are very important to address. https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils.py File tools/findit/crash_utils.py (right): https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:60: temp = regression.split(':') s/temp/revisions/ https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:94: def GetDataFromURL(url, retry=10, sleep_time=0.1): Using the name retry makes it sound like a boolean. Change to retries. https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:216: return '<a href="%s">%s<\\a>' % (link, to_add) Depending on how this is used, this is scary. Use something like https://docs.python.org/2/library/cgi.html#cgi.escape to sanitize the input. Also, the closing tag should be </a>. https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:220: """Returns a string representation of a list . Nit: there's an extra space before the period. Could you also mention that it omits the brackets, it would be easier than having to read through the code to find out how the list is being prettified. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:49: # Check if current line is the start of new callstack This comment isn't needed. The next line could stand on its own pretty well with a minor name change. Also, remember to end comments with punctuation. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:50: is_new_callstack = self.__CheckIfNewCallStack(line, chrome_build) __CheckIfNewCallStack is a slightly awkward name. Maybe just something like __IsStartOfNewStack. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:63: stack_priority += 1 A while ago, I thought we discussed that free and allocation stacks should have the same priority as one another. We didn't really have any data to go on at the time, but we just guessed that they could both be equally useful. Either way, having the priority set by the order of the stack alone might cause some issues down the road. You might want to try to have a way to actually detect what kind of stack you are working with and set priority based on that, rather than the order you find it in. Hypothetically, if we realize later on that the allocation stack provides much better information than the free stack, changing how this works later could be problematic with this implementation. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:67: parsed_stack_frame_line = self.__ExtractFromStackFrame( This is more of a general comment, but try to name functions in such a way that you can tell what they do from the name alone where possible. In this case, it's pretty clear from the comment above what this is doing. It would be even better if the name was clear enough that the comment wasn't necessary. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:74: (function, file_path, crashed_line_number) = parsed_stack_frame_line Instead of dealing with multiple return values from this, maybe it would be better to create a single helper to create the StackFrame object from the required information. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:116: stack_frame_index_pattern = re.compile(r'#(\d+)') Make sure that there is only whitespace before the #. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:147: stack_frame_index_pattern = re.compile(r'#(\d+)') These three lines seem to be the same in both cases, so just move it to before the if statement to avoid the duplication. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:178: def GetStackFromMainThread(self): It makes more sense to call this the crash stack than the stack from the main thread. There's a good chance that all of the stacks are from the same thread, they just have different meaning. This came up in a few different places, so it would be worth going back through to check for other uses like this. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:196: def GetFirstN(self, n): Consider renaming to something like GetTopNFrames. Depending on how this is used, it will make what is going on a bit clearer.
In general it looks pretty good. Most of these comments are minor nits, though a few are very important to address. https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils.py File tools/findit/crash_utils.py (right): https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:60: temp = regression.split(':') s/temp/revisions/ https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:94: def GetDataFromURL(url, retry=10, sleep_time=0.1): Using the name retry makes it sound like a boolean. Change to retries. https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:216: return '<a href="%s">%s<\\a>' % (link, to_add) Depending on how this is used, this is scary. Use something like https://docs.python.org/2/library/cgi.html#cgi.escape to sanitize the input. Also, the closing tag should be </a>. https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:220: """Returns a string representation of a list . Nit: there's an extra space before the period. Could you also mention that it omits the brackets, it would be easier than having to read through the code to find out how the list is being prettified. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:49: # Check if current line is the start of new callstack This comment isn't needed. The next line could stand on its own pretty well with a minor name change. Also, remember to end comments with punctuation. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:50: is_new_callstack = self.__CheckIfNewCallStack(line, chrome_build) __CheckIfNewCallStack is a slightly awkward name. Maybe just something like __IsStartOfNewStack. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:63: stack_priority += 1 A while ago, I thought we discussed that free and allocation stacks should have the same priority as one another. We didn't really have any data to go on at the time, but we just guessed that they could both be equally useful. Either way, having the priority set by the order of the stack alone might cause some issues down the road. You might want to try to have a way to actually detect what kind of stack you are working with and set priority based on that, rather than the order you find it in. Hypothetically, if we realize later on that the allocation stack provides much better information than the free stack, changing how this works later could be problematic with this implementation. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:67: parsed_stack_frame_line = self.__ExtractFromStackFrame( This is more of a general comment, but try to name functions in such a way that you can tell what they do from the name alone where possible. In this case, it's pretty clear from the comment above what this is doing. It would be even better if the name was clear enough that the comment wasn't necessary. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:74: (function, file_path, crashed_line_number) = parsed_stack_frame_line Instead of dealing with multiple return values from this, maybe it would be better to create a single helper to create the StackFrame object from the required information. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:116: stack_frame_index_pattern = re.compile(r'#(\d+)') Make sure that there is only whitespace before the #. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:147: stack_frame_index_pattern = re.compile(r'#(\d+)') These three lines seem to be the same in both cases, so just move it to before the if statement to avoid the duplication. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:178: def GetStackFromMainThread(self): It makes more sense to call this the crash stack than the stack from the main thread. There's a good chance that all of the stacks are from the same thread, they just have different meaning. This came up in a few different places, so it would be worth going back through to check for other uses like this. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:196: def GetFirstN(self, n): Consider renaming to something like GetTopNFrames. Depending on how this is used, it will make what is going on a bit clearer.
Hi Martin, I have addressed your code review. Would you take a look at the changes and give some feedback? Thanks! https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils.py File tools/findit/crash_utils.py (right): https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:60: temp = regression.split(':') On 2014/08/06 21:12:24, mbarbella wrote: > s/temp/revisions/ Done. https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:94: def GetDataFromURL(url, retry=10, sleep_time=0.1): On 2014/08/06 21:12:24, mbarbella wrote: > Using the name retry makes it sound like a boolean. Change to retries. Done. https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:216: return '<a href="%s">%s<\\a>' % (link, to_add) On 2014/08/06 21:12:24, mbarbella wrote: > Depending on how this is used, this is scary. Use something like > https://docs.python.org/2/library/cgi.html#cgi.escape to sanitize the input. > Also, the closing tag should be </a>. added cgi.escape to link and changed the closing tag. https://codereview.chromium.org/430943003/diff/80001/tools/findit/crash_utils... tools/findit/crash_utils.py:220: """Returns a string representation of a list . On 2014/08/06 21:12:24, mbarbella wrote: > Nit: there's an extra space before the period. Could you also mention that it > omits the brackets, it would be easier than having to read through the code to > find out how the list is being prettified. Added explanation on how the string is prettified. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:49: # Check if current line is the start of new callstack On 2014/08/06 21:12:25, mbarbella wrote: > This comment isn't needed. The next line could stand on its own pretty well with > a minor name change. Also, remember to end comments with punctuation. Done. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:50: is_new_callstack = self.__CheckIfNewCallStack(line, chrome_build) On 2014/08/06 21:12:24, mbarbella wrote: > __CheckIfNewCallStack is a slightly awkward name. Maybe just something like > __IsStartOfNewStack. Done. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:63: stack_priority += 1 On 2014/08/06 21:12:25, mbarbella wrote: > A while ago, I thought we discussed that free and allocation stacks should have > the same priority as one another. We didn't really have any data to go on at the > time, but we just guessed that they could both be equally useful. > > Either way, having the priority set by the order of the stack alone might cause > some issues down the road. You might want to try to have a way to actually > detect what kind of stack you are working with and set priority based on that, > rather than the order you find it in. Hypothetically, if we realize later on > that the allocation stack provides much better information than the free stack, > changing how this works later could be problematic with this implementation. i have fixed so that it would look at the marker within the stacktrace to infer which priority it should get. I think it should work. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:67: parsed_stack_frame_line = self.__ExtractFromStackFrame( On 2014/08/06 21:12:24, mbarbella wrote: > This is more of a general comment, but try to name functions in such a way that > you can tell what they do from the name alone where possible. In this case, it's > pretty clear from the comment above what this is doing. It would be even better > if the name was clear enough that the comment wasn't necessary. Done. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:74: (function, file_path, crashed_line_number) = parsed_stack_frame_line On 2014/08/06 21:12:24, mbarbella wrote: > Instead of dealing with multiple return values from this, maybe it would be > better to create a single helper to create the StackFrame object from the > required information. Done. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:116: stack_frame_index_pattern = re.compile(r'#(\d+)') On 2014/08/06 21:12:25, mbarbella wrote: > Make sure that there is only whitespace before the #. This line is deleted. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:147: stack_frame_index_pattern = re.compile(r'#(\d+)') On 2014/08/06 21:12:25, mbarbella wrote: > These three lines seem to be the same in both cases, so just move it to before > the if statement to avoid the duplication. Done. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:178: def GetStackFromMainThread(self): On 2014/08/06 21:12:25, mbarbella wrote: > It makes more sense to call this the crash stack than the stack from the main > thread. There's a good chance that all of the stacks are from the same thread, > they just have different meaning. This came up in a few different places, so it > would be worth going back through to check for other uses like this. Done. https://codereview.chromium.org/430943003/diff/80001/tools/findit/stacktrace.... tools/findit/stacktrace.py:196: def GetFirstN(self, n): On 2014/08/06 21:12:24, mbarbella wrote: > Consider renaming to something like GetTopNFrames. Depending on how this is > used, it will make what is going on a bit clearer. Done.
https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... File tools/findit/component_dictionary.py (right): https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:17: self.file_dic = {} s/dic/dict everywhere. https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:32: self.file_dic[file_name] = {} I don't understand why you use file_name as key. file_name is not unique. Just use full file_path and ignore file_name completely s/self.file_dic[file_name][file_path]/self.file_dic[file_path] https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:34: if file_path not in self.file_dic[file_name]: Why all these seperate ifs. All this initialization has to be done once when there there is no file_name in file_dict. If not, just initialize all these vars. https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:38: self.file_dic[file_name][file_path]['lines'] = [] s/lines/stack_frames https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:41: self.file_dic[file_name][file_path]['stack_frame_index'] = [] s/stack_frame_index/stack_frame_indices. it is not just one index, right ? https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:83: self.component_dic = {} s/component_dic/component_dict https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:92: def GetFileDic(self, component): s/Dic(/Dict( everywhere please https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... File tools/findit/crash_utils.py (right): https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:29: if normalized_path.startswith( Look for the first src/ in path. Build location are very different for different job types. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:34: if '../../' in normalized_path: This is bad and hacky (how about three ../../../). try using abs_path to see if it rids of .. this should be the first step in normalication. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:41: if './' in normalized_path: Why this ? No comments in code ? Why ? https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:71: if start_range.startswith('r'): Use lstrip and then don't need an if. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:92: return data new line before this line. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:114: break Why not just return data.read() then don't need 121-124 and just return None there. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:157: def GuessIfSamePath(path1, path2): SamePath naming is wrong, this should be like SameSubPath. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:177: def FindMinStackFrameNum(stack_frame_index, priorities): s/Num/Number. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:181: stack_frame_index: A list of list containing stack position. list of list ? https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:202: return float('inf') define float('inf') in a global var and use throughout code with a better name. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:271: line_minus_n = range(line - 3, line + 1) don't hardcode numbers in code, put them in global at start of file or in a configuration number. also this number should be like 5. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:271: line_minus_n = range(line - 3, line + 1) Also, you shouldn't go backward, just forward by 5 lines. Any line change can't affect anything before it. give me a scenario. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:285: to_add = changed_line s/to_add/pick better meaningful name. https://codereview.chromium.org/430943003/diff/120001/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/120001/tools/findit/stacktrace... tools/findit/stacktrace.py:31: chrome_build: A string containing the build type of the crash. Why chrome_build ? why not build_type. https://codereview.chromium.org/430943003/diff/120001/tools/findit/stacktrace... tools/findit/stacktrace.py:58: # If this is from freed or prev-alloc, add the callstack we have s/previous-alloc/allocation stack https://codereview.chromium.org/430943003/diff/120001/tools/findit/stacktrace... tools/findit/stacktrace.py:221: # Currently only supports blink and chromium. Why not FIXME ?
https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... File tools/findit/crash_utils.py (right): https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:218: return '<a href="%s">%s</a>' % (sanitized_link, to_add) This is still potentially unsafe. You need to sanitize both of these, and the link needs to use quote=True.
Hi Shoutao, Can you do the final review? Thanks! https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... File tools/findit/component_dictionary.py (right): https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:17: self.file_dic = {} On 2014/08/07 15:38:47, aarya wrote: > s/dic/dict everywhere. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:32: self.file_dic[file_name] = {} On 2014/08/07 15:38:46, aarya wrote: > I don't understand why you use file_name as key. file_name is not unique. Just > use full file_path and ignore file_name completely > > s/self.file_dic[file_name][file_path]/self.file_dic[file_path] It is easier and faster to check if a crashing file occurs in any CLs using file name than file path. I also make sure that the matching file also has same path. https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:34: if file_path not in self.file_dic[file_name]: On 2014/08/07 15:38:47, aarya wrote: > Why all these seperate ifs. All this initialization has to be done once when > there there is no file_name in file_dict. If not, just initialize all these > vars. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:38: self.file_dic[file_name][file_path]['lines'] = [] On 2014/08/07 15:38:46, aarya wrote: > s/lines/stack_frames changed to line_numbers (the first name was a bit misleading) https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:41: self.file_dic[file_name][file_path]['stack_frame_index'] = [] On 2014/08/07 15:38:46, aarya wrote: > s/stack_frame_index/stack_frame_indices. it is not just one index, right ? Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:83: self.component_dic = {} On 2014/08/07 15:38:47, aarya wrote: > s/component_dic/component_dict Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/component_... tools/findit/component_dictionary.py:92: def GetFileDic(self, component): On 2014/08/07 15:38:47, aarya wrote: > s/Dic(/Dict( everywhere please Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... File tools/findit/crash_utils.py (right): https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:29: if normalized_path.startswith( On 2014/08/07 15:38:47, aarya wrote: > Look for the first src/ in path. Build location are very different for different > job types. Made it so that it would look for /build/. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:34: if '../../' in normalized_path: On 2014/08/07 15:38:47, aarya wrote: > This is bad and hacky (how about three ../../../). try using abs_path to see if > it rids of .. this should be the first step in normalication. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:41: if './' in normalized_path: On 2014/08/07 15:38:48, aarya wrote: > Why this ? No comments in code ? Why ? Removed and used abspath. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:71: if start_range.startswith('r'): On 2014/08/07 15:38:47, aarya wrote: > Use lstrip and then don't need an if. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:92: return data On 2014/08/07 15:38:47, aarya wrote: > new line before this line. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:114: break On 2014/08/07 15:38:47, aarya wrote: > Why not just return data.read() then don't need 121-124 and just return None > there. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:157: def GuessIfSamePath(path1, path2): On 2014/08/07 15:38:47, aarya wrote: > SamePath naming is wrong, this should be like SameSubPath. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:177: def FindMinStackFrameNum(stack_frame_index, priorities): On 2014/08/07 15:38:47, aarya wrote: > s/Num/Number. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:181: stack_frame_index: A list of list containing stack position. On 2014/08/07 15:38:48, aarya wrote: > list of list ? yes, each sublist is a list of stack frames in one match. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:202: return float('inf') On 2014/08/07 15:38:47, aarya wrote: > define float('inf') in a global var and use throughout code with a better name. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:218: return '<a href="%s">%s</a>' % (sanitized_link, to_add) On 2014/08/07 15:48:41, mbarbella wrote: > This is still potentially unsafe. You need to sanitize both of these, and the > link needs to use quote=True. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:271: line_minus_n = range(line - 3, line + 1) On 2014/08/07 15:38:47, aarya wrote: > don't hardcode numbers in code, put them in global at start of file or in a > configuration number. also this number should be like 5. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:271: line_minus_n = range(line - 3, line + 1) On 2014/08/07 15:38:47, aarya wrote: > Also, you shouldn't go backward, just forward by 5 lines. Any line change can't > affect anything before it. give me a scenario. it is a backward by 5 lines from crashed line, instead of forward 5 lines from changed lines. So I think they are the same? https://codereview.chromium.org/430943003/diff/120001/tools/findit/crash_util... tools/findit/crash_utils.py:285: to_add = changed_line On 2014/08/07 15:38:47, aarya wrote: > s/to_add/pick better meaningful name. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/120001/tools/findit/stacktrace... tools/findit/stacktrace.py:31: chrome_build: A string containing the build type of the crash. On 2014/08/07 15:38:48, aarya wrote: > Why chrome_build ? why not build_type. Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/stacktrace... tools/findit/stacktrace.py:58: # If this is from freed or prev-alloc, add the callstack we have On 2014/08/07 15:38:48, aarya wrote: > s/previous-alloc/allocation stack Done. https://codereview.chromium.org/430943003/diff/120001/tools/findit/stacktrace... tools/findit/stacktrace.py:221: # Currently only supports blink and chromium. On 2014/08/07 15:38:48, aarya wrote: > Why not FIXME ? Done.
Add a few more comments. https://codereview.chromium.org/430943003/diff/140001/tools/findit/component_... File tools/findit/component_dictionary.py (right): https://codereview.chromium.org/430943003/diff/140001/tools/findit/component_... tools/findit/component_dictionary.py:8: class FileDicttionary(object): Misspell: Dicttionary -> Dictionary. Please do a search for 'Dictt' in all files. https://codereview.chromium.org/430943003/diff/140001/tools/findit/crash_util... File tools/findit/crash_utils.py (right): https://codereview.chromium.org/430943003/diff/140001/tools/findit/crash_util... tools/findit/crash_utils.py:10: One more empty line above. https://codereview.chromium.org/430943003/diff/140001/tools/findit/crash_util... tools/findit/crash_utils.py:261: It seems the code has empty lines after for. Would you please remove all of them after for? https://codereview.chromium.org/430943003/diff/140001/tools/findit/crash_util... tools/findit/crash_utils.py:262: # Also check previous 3 lines. Comment needs updating, as 3 is invalid now. https://codereview.chromium.org/430943003/diff/140001/tools/findit/stacktrace.py File tools/findit/stacktrace.py (right): https://codereview.chromium.org/430943003/diff/140001/tools/findit/stacktrace... tools/findit/stacktrace.py:11: class Stacktrace(object): As StackFrame is used by CallStack and CallStack is used by Stacktrace. The order they show up in the code should be the same.
https://codereview.chromium.org/430943003/diff/140001/tools/findit/component_... File tools/findit/component_dictionary.py (right): https://codereview.chromium.org/430943003/diff/140001/tools/findit/component_... tools/findit/component_dictionary.py:8: class FileDicttionary(object): On 2014/08/07 21:03:37, Shuotao wrote: > Misspell: Dicttionary -> Dictionary. > > Please do a search for 'Dictt' in all files. Done. https://codereview.chromium.org/430943003/diff/140001/tools/findit/crash_util... File tools/findit/crash_utils.py (right): https://codereview.chromium.org/430943003/diff/140001/tools/findit/crash_util... tools/findit/crash_utils.py:10: On 2014/08/07 21:03:37, Shuotao wrote: > One more empty line above. Done. https://codereview.chromium.org/430943003/diff/140001/tools/findit/crash_util... tools/findit/crash_utils.py:261: On 2014/08/07 21:03:37, Shuotao wrote: > It seems the code has empty lines after for. Would you please remove all of them > after for? Done. https://codereview.chromium.org/430943003/diff/140001/tools/findit/crash_util... tools/findit/crash_utils.py:262: # Also check previous 3 lines. On 2014/08/07 21:03:37, Shuotao wrote: > Comment needs updating, as 3 is invalid now. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeun@chromium.org/430943003/160001
Message was sent while issue was closed.
Change committed as 288270 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
