| OLD | NEW |
| 1 # Copyright 2016 The Chromium Authors. All rights reserved. | 1 # Copyright 2016 The Chromium Authors. All rights reserved. |
| 2 # Use of this source code is governed by a BSD-style license that can be | 2 # Use of this source code is governed by a BSD-style license that can be |
| 3 # found in the LICENSE file. | 3 # found in the LICENSE file. |
| 4 | 4 |
| 5 import copy | 5 import copy |
| 6 import json | 6 import json |
| 7 import logging | 7 import logging |
| 8 | 8 |
| 9 from google.appengine.ext import ndb | 9 from google.appengine.ext import ndb |
| 10 | 10 |
| 11 from common import appengine_util | 11 from common import appengine_util |
| 12 from common.chrome_dependency_fetcher import ChromeDependencyFetcher | 12 from common.chrome_dependency_fetcher import ChromeDependencyFetcher |
| 13 from common import constants | 13 from common import constants |
| 14 from crash.component import Component |
| 15 from crash.component_classifier import ComponentClassifier |
| 14 from crash.crash_report import CrashReport | 16 from crash.crash_report import CrashReport |
| 17 from crash.project import Project |
| 18 from crash.project_classifier import ProjectClassifier |
| 15 from libs import time_util | 19 from libs import time_util |
| 16 from model import analysis_status | 20 from model import analysis_status |
| 17 from model.crash.crash_config import CrashConfig | 21 from model.crash.crash_config import CrashConfig |
| 18 | 22 |
| 19 | 23 |
| 20 # TODO(http://crbug.com/659346): since most of our unit tests are | 24 # TODO(http://crbug.com/659346): since most of our unit tests are |
| 21 # FinditForFracas-specific, wrengr moved them to findit_for_chromecrash_test.py. | 25 # FinditForFracas-specific, wrengr moved them to findit_for_chromecrash_test.py. |
| 22 # However, now we're missing coverage for most of this file (due to the | 26 # However, now we're missing coverage for most of this file (due to the |
| 23 # buggy way coverage is computed). Need to add a bunch of new unittests | 27 # buggy way coverage is computed). Need to add a bunch of new unittests |
| 24 # to get coverage back up. | 28 # to get coverage back up. |
| 25 | 29 |
| 26 # TODO: this class depends on ndb stuff, and should therefore move to | 30 # TODO: this class depends on ndb stuff, and should therefore move to |
| 27 # cr-culprit-finder/service/predator as part of the big reorganization. | 31 # cr-culprit-finder/service/predator as part of the big reorganization. |
| 28 # This class should be renamed to avoid confustion between Findit and Predator. | 32 # This class should be renamed to avoid confustion between Findit and Predator. |
| 29 # Think of a good name (e.g.'PredatorApp') for this class. | 33 # Think of a good name (e.g.'PredatorApp') for this class. |
| 30 class Findit(object): | 34 class Findit(object): |
| 31 | 35 |
| 32 def __init__(self, get_repository): | 36 def __init__(self, get_repository, config): |
| 33 """ | 37 """ |
| 34 Args: | 38 Args: |
| 35 get_repository (callable): a function from DEP urls to ``Repository`` | 39 get_repository (callable): a function from DEP urls to ``Repository`` |
| 36 objects, so we can get changelogs and blame for each dep. Notably, | 40 objects, so we can get changelogs and blame for each dep. Notably, |
| 37 to keep the code here generic, we make no assumptions about | 41 to keep the code here generic, we make no assumptions about |
| 38 which subclass of ``Repository`` this function returns. Thus, | 42 which subclass of ``Repository`` this function returns. Thus, |
| 39 it is up to the caller to decide what class to return and handle | 43 it is up to the caller to decide what class to return and handle |
| 40 any other arguments that class may require (e.g., an http client | 44 any other arguments that class may require (e.g., an http client |
| 41 for ``GitilesRepository``). | 45 for ``GitilesRepository``). |
| 46 config (ndb.CrashConfig): Config for clients and project and component |
| 47 classifiers. |
| 42 """ | 48 """ |
| 43 self._get_repository = get_repository | 49 self._get_repository = get_repository |
| 44 self._dep_fetcher = ChromeDependencyFetcher(get_repository) | 50 self._dep_fetcher = ChromeDependencyFetcher(get_repository) |
| 51 |
| 52 # The top_n is the number of frames we want to check to get project |
| 53 # classifications. |
| 54 projects = [Project(name, path_regexs, function_regexs, host_directories) |
| 55 for name, path_regexs, function_regexs, host_directories |
| 56 in config.project_classifier['project_path_function_hosts']] |
| 57 self._project_classifier = ProjectClassifier( |
| 58 projects, config.project_classifier['top_n'], |
| 59 config.project_classifier['non_chromium_project_rank_priority']) |
| 60 |
| 61 components = [Component(component_name, path_regex, function_regex) |
| 62 for path_regex, function_regex, component_name |
| 63 in config.component_classifier['path_function_component']] |
| 64 self._component_classifier = ComponentClassifier( |
| 65 components, config.component_classifier['top_n']) |
| 66 |
| 67 self._config = config |
| 68 |
| 45 # TODO(http://crbug.com/659354): because self.client is volatile, | 69 # TODO(http://crbug.com/659354): because self.client is volatile, |
| 46 # we need some way of updating the Predator instance whenever the | 70 # we need some way of updating the Predator instance whenever the |
| 47 # config changes. How to do that cleanly? | 71 # config changes. How to do that cleanly? |
| 48 self._predator = None | 72 self._predator = None |
| 49 self._stacktrace_parser = None | 73 self._stacktrace_parser = None |
| 50 | 74 |
| 51 # This is a class method because it should be the same for all | 75 # This is a class method because it should be the same for all |
| 52 # instances of this class. We can in fact call class methods on | 76 # instances of this class. We can in fact call class methods on |
| 53 # instances (not just on the class itself), so we could in principle | 77 # instances (not just on the class itself), so we could in principle |
| 54 # get by with just this method. However, a @classmethod is treated | 78 # get by with just this method. However, a @classmethod is treated |
| (...skipping 25 matching lines...) Expand all Loading... |
| 80 | 104 |
| 81 N.B., this property is static and should not be overridden.""" | 105 N.B., this property is static and should not be overridden.""" |
| 82 return self._ClientID() | 106 return self._ClientID() |
| 83 | 107 |
| 84 # TODO(http://crbug.com/659354): can we remove the dependency on | 108 # TODO(http://crbug.com/659354): can we remove the dependency on |
| 85 # CrashConfig entirely? It'd be better to receive method calls | 109 # CrashConfig entirely? It'd be better to receive method calls |
| 86 # whenever things change, so that we know the change happened (and | 110 # whenever things change, so that we know the change happened (and |
| 87 # what in particular changed) so that we can update our internal state | 111 # what in particular changed) so that we can update our internal state |
| 88 # as appropriate. | 112 # as appropriate. |
| 89 @property | 113 @property |
| 90 def config(self): | 114 def client_config(self): |
| 91 """Get the current value of the client config for the class of this object. | 115 """Get the current value of the client config for the class of this object. |
| 92 | 116 |
| 93 N.B., this property is volatile and may change asynchronously. | 117 N.B., this property is volatile and may change asynchronously. |
| 94 | 118 |
| 95 If the event of an error this method will return ``None``. That we do | 119 If the event of an error this method will return ``None``. That we do |
| 96 not return the empty dict is intentional. All of the circumstances | 120 not return the empty dict is intentional. All of the circumstances |
| 97 which would lead to this method returning ``None`` indicate | 121 which would lead to this method returning ``None`` indicate |
| 98 underlying bugs in code elsewhere. (E.g., creating instances of | 122 underlying bugs in code elsewhere. (E.g., creating instances of |
| 99 abstract classes, implementing a subclass which is not suppported by | 123 abstract classes, implementing a subclass which is not suppported by |
| 100 ``CrashConfig``, etc.) Because we return ``None``, any subsequent | 124 ``CrashConfig``, etc.) Because we return ``None``, any subsequent |
| 101 calls to ``__getitem__`` or ``get`` will raise method-missing errors; | 125 calls to ``__getitem__`` or ``get`` will raise method-missing errors; |
| 102 which serve to highlight the underlying bug. Whereas, if we silently | 126 which serve to highlight the underlying bug. Whereas, if we silently |
| 103 returned the empty dict, then calls to ``get`` would "work" just | 127 returned the empty dict, then calls to ``get`` would "work" just |
| 104 fine; thereby masking the underlying bug! | 128 fine; thereby masking the underlying bug! |
| 105 """ | 129 """ |
| 106 return CrashConfig.Get().GetClientConfig(self.client_id) | 130 return self._config.GetClientConfig(self.client_id) |
| 107 | 131 |
| 108 # TODO(http://crbug.com/644476): rename to CanonicalizePlatform or | 132 # TODO(http://crbug.com/644476): rename to CanonicalizePlatform or |
| 109 # something like that. | 133 # something like that. |
| 110 def RenamePlatform(self, platform): | 134 def RenamePlatform(self, platform): |
| 111 """Remap the platform to a different one, based on the config.""" | 135 """Remap the platform to a different one, based on the config.""" |
| 112 # TODO(katesonia): Remove the default value after adding validity check to | 136 # TODO(katesonia): Remove the default value after adding validity check to |
| 113 # config. | 137 # config. |
| 114 return self.config.get('platform_rename', {}).get(platform, platform) | 138 return self.client_config.get('platform_rename', {}).get(platform, platform) |
| 115 | 139 |
| 116 def CheckPolicy(self, crash_data): # pylint: disable=W0613 | 140 def CheckPolicy(self, crash_data): # pylint: disable=W0613 |
| 117 """Check whether this client supports analyzing the given report. | 141 """Check whether this client supports analyzing the given report. |
| 118 | 142 |
| 119 Some clients only support analysis for crashes on certain platforms | 143 Some clients only support analysis for crashes on certain platforms |
| 120 or channels, etc. This method checks to see whether this client can | 144 or channels, etc. This method checks to see whether this client can |
| 121 analyze the given crash. The default implementation on the Findit | 145 analyze the given crash. The default implementation on the Findit |
| 122 base class returns None for everything, so that unsupported clients | 146 base class returns None for everything, so that unsupported clients |
| 123 reject everything, as expected. | 147 reject everything, as expected. |
| 124 | 148 |
| (...skipping 82 matching lines...) Expand 10 before | Expand all | Expand 10 after Loading... |
| 207 def ParseStacktrace(self, model): # pragma: no cover | 231 def ParseStacktrace(self, model): # pragma: no cover |
| 208 """Parse a CrashAnalysis's ``stack_trace`` string into a Stacktrace object. | 232 """Parse a CrashAnalysis's ``stack_trace`` string into a Stacktrace object. |
| 209 | 233 |
| 210 Args: | 234 Args: |
| 211 model (CrashAnalysis): The model containing the stack_trace string | 235 model (CrashAnalysis): The model containing the stack_trace string |
| 212 to be parsed. | 236 to be parsed. |
| 213 | 237 |
| 214 Returns: | 238 Returns: |
| 215 On success, returns a Stacktrace object; on failure, returns None. | 239 On success, returns a Stacktrace object; on failure, returns None. |
| 216 """ | 240 """ |
| 217 # Use up-to-date ``top_n`` in self.config to filter top n frames. | 241 # Use up-to-date ``top_n`` in self.client_config to filter top n frames. |
| 218 stacktrace = self._stacktrace_parser.Parse( | 242 stacktrace = self._stacktrace_parser.Parse( |
| 219 model.stack_trace, | 243 model.stack_trace, |
| 220 self._dep_fetcher.GetDependency(model.crashed_version, model.platform), | 244 self._dep_fetcher.GetDependency(model.crashed_version, model.platform), |
| 221 model.signature, | 245 model.signature, |
| 222 self.config.get('top_n')) | 246 self.client_config.get('top_n')) |
| 223 if not stacktrace: | 247 if not stacktrace: |
| 224 logging.warning('Failed to parse the stacktrace %s', model.stack_trace) | 248 logging.warning('Failed to parse the stacktrace %s', model.stack_trace) |
| 225 return None | 249 return None |
| 226 | 250 |
| 227 return stacktrace | 251 return stacktrace |
| 228 | 252 |
| 229 def ProcessResultForPublishing(self, result, key): | 253 def ProcessResultForPublishing(self, result, key): |
| 230 """Client specific processing of result data for publishing.""" | 254 """Client specific processing of result data for publishing.""" |
| 231 raise NotImplementedError() | 255 raise NotImplementedError() |
| 232 | 256 |
| (...skipping 39 matching lines...) Expand 10 before | Expand all | Expand 10 after Loading... |
| 272 stacktrace = self.ParseStacktrace(model) | 296 stacktrace = self.ParseStacktrace(model) |
| 273 if stacktrace is None: | 297 if stacktrace is None: |
| 274 return None | 298 return None |
| 275 | 299 |
| 276 return self._predator.FindCulprit(CrashReport( | 300 return self._predator.FindCulprit(CrashReport( |
| 277 crashed_version = model.crashed_version, | 301 crashed_version = model.crashed_version, |
| 278 signature = model.signature, | 302 signature = model.signature, |
| 279 platform = model.platform, | 303 platform = model.platform, |
| 280 stacktrace = stacktrace, | 304 stacktrace = stacktrace, |
| 281 regression_range = model.regression_range)) | 305 regression_range = model.regression_range)) |
| OLD | NEW |