From 795ce832d0a9e74c199345a6b4d6ffb737600e6c Mon Sep 17 00:00:00 2001 From: James Graham Date: Tue, 29 May 2018 18:18:40 +0100 Subject: [PATCH] Bug 1354232 - Add support for updating LSAN data in wpt-update r=maja_zf LSAN data differs from existing expectation data because the data is only generated when the browser exits, so the problems reported can happen at any point in the current session. We use the `scope` property in the log message to determine the path to a __dir__.ini file that covers all the tests run in the session, and add the LSAN exclusion rules to there. The rules themselves are generated by taking the topmost frame of any stack that's reported as unexpectedly leaking, and adding that to the list of permitted frames in the lsan-allowed property. We never remove entries from this list since intermittents may be present which won't appear on a specific run. Instead we rely on humans fixing the issues to also clean up the expectation files. MozReview-Commit-ID: Kxm0hFXlGE3 --- .../wptrunner/wptrunner/browsers/firefox.py | 20 +++-- .../wptrunner/wptrunner/manifestupdate.py | 54 ++++++++++++ .../tools/wptrunner/wptrunner/metadata.py | 86 +++++++++++++++---- .../tools/wptrunner/wptrunner/testloader.py | 1 + 4 files changed, 137 insertions(+), 24 deletions(-) diff --git a/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py b/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py index 8b3d32b41098..094fdeb56945 100644 --- a/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py +++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py @@ -180,24 +180,32 @@ class FirefoxBrowser(Browser): self.init_timeout = self.init_timeout * timeout_multiplier self.asan = asan + self.lsan_allowed = None self.leak_check = leak_check self.leak_report_file = None self.lsan_handler = None - if self.asan: - self.lsan_handler = mozleak.LSANLeaks(logger) self.stylo_threads = stylo_threads self.chaos_mode_flags = chaos_mode_flags def settings(self, test): - if self.asan: - self.lsan_handler.set_allowed(test.lsan_allowed) - return {"check_leaks": self.leak_check and not test.leaks} + self.lsan_allowed = test.lsan_allowed + return {"check_leaks": self.leak_check and not test.leaks, + "lsan_allowed": test.lsan_allowed} + + def start(self, group_metadata=None, **kwargs): + if group_metadata is None: + group_metadata = {} - def start(self, **kwargs): if self.marionette_port is None: self.marionette_port = get_free_port(2828, exclude=self.used_ports) self.used_ports.add(self.marionette_port) + if self.asan: + print "Setting up LSAN" + self.lsan_handler = mozleak.LSANLeaks(self.logger, + scope=group_metadata.get("scope", "/"), + allowed=self.lsan_allowed) + env = test_environment(xrePath=os.path.dirname(self.binary), debugger=self.debug_info is not None, log=self.logger, diff --git a/testing/web-platform/tests/tools/wptrunner/wptrunner/manifestupdate.py b/testing/web-platform/tests/tools/wptrunner/wptrunner/manifestupdate.py index 35d0c070201e..99aa3cfba434 100644 --- a/testing/web-platform/tests/tools/wptrunner/wptrunner/manifestupdate.py +++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/manifestupdate.py @@ -75,6 +75,9 @@ class ExpectedManifest(ManifestItem): self.modified = False self.boolean_properties = boolean_properties self.property_order = property_order + self.update_properties = { + "lsan": LsanUpdate(self), + } def append(self, child): ManifestItem.append(self, child) @@ -106,6 +109,19 @@ class ExpectedManifest(ManifestItem): return urlparse.urljoin(self.url_base, "/".join(self.test_path.split(os.path.sep))) + def set_lsan(self, run_info, result): + """Set the result of the test in a particular run + + :param run_info: Dictionary of run_info parameters corresponding + to this run + :param result: Lsan violations detected""" + + self.update_properties["lsan"].set(run_info, result) + + def coalesce_properties(self, stability): + for prop_update in self.update_properties.itervalues(): + prop_update.coalesce(stability) + class TestNode(ManifestItem): def __init__(self, node): @@ -475,6 +491,44 @@ class MinAssertsUpdate(PropertyUpdate): return True, new_value +class LsanUpdate(PropertyUpdate): + property_name = "lsan-allowed" + cls_default_value = None + + def get_value(self, result): + # IF we have an allowed_match that matched, return None + # This value is ignored later (because it matches the default) + # We do that because then if we allow a failure in foo/__dir__.ini + # we don't want to update foo/bar/__dir__.ini with the same rule + if result[1]: + return None + # Otherwise return the topmost stack frame + # TODO: there is probably some improvement to be made by looking for a "better" stack frame + return result[0][0] + + def update_value(self, old_value, new_value): + if isinstance(new_value, (str, unicode)): + new_value = {new_value} + else: + new_value = set(new_value) + if old_value is None: + old_value = set() + old_value = set(old_value) + return sorted((old_value | new_value) - {None}) + + def update_default(self): + current_default = None + if self.property_name in self.node._data: + current_default = [item for item in + self.node._data[self.property_name] + if item.condition_node is None] + if current_default: + current_default = current_default[0].value + new_values = [item.value for item in self.new] + new_value = self.update_value(current_default, new_values) + return True, new_value if new_value else None + + def group_conditionals(values, property_order=None, boolean_properties=None): """Given a list of Value objects, return a list of (conditional_node, status) pairs representing the conditional diff --git a/testing/web-platform/tests/tools/wptrunner/wptrunner/metadata.py b/testing/web-platform/tests/tools/wptrunner/wptrunner/metadata.py index 4ebb2242d779..602b71cef371 100644 --- a/testing/web-platform/tests/tools/wptrunner/wptrunner/metadata.py +++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/metadata.py @@ -3,7 +3,6 @@ import shutil import tempfile import uuid -from mozlog import reader from mozlog import structuredlog import expected @@ -18,11 +17,9 @@ manifestitem = None logger = structuredlog.StructuredLogger("web-platform-tests") try: - import ujson + import ujson as json except ImportError: - pass -else: - reader.json = ujson + import json def load_test_manifests(serve_root, test_paths): @@ -167,6 +164,7 @@ def update_from_logs(manifests, *log_filenames, **kwargs): for manifest_expected in expected_map.itervalues(): for tree in manifest_expected.itervalues(): + tree.coalesce_properties(stability=stability) for test in tree.iterchildren(): for subtest in test.iterchildren(): subtest.coalesce_properties(stability=stability) @@ -238,7 +236,8 @@ class ExpectedUpdater(object): "test_start": self.test_start, "test_status": self.test_status, "test_end": self.test_end, - "assertion_count": self.assertion_count} + "assertion_count": self.assertion_count, + "lsan_leak": self.lsan_leak} self.tests_visited = {} self.test_cache = {} @@ -251,8 +250,12 @@ class ExpectedUpdater(object): def update_from_log(self, log_file): self.run_info = None - log_reader = reader.read(log_file) - reader.each_log(log_reader, self.action_map) + action_map = self.action_map + for line in log_file: + data = json.loads(line) + action = data["action"] + if action in action_map: + action_map[action](data) def suite_start(self, data): self.run_info = data["run_info"] @@ -314,6 +317,16 @@ class ExpectedUpdater(object): test.set_asserts(self.run_info, data["count"]) + def lsan_leak(self, data): + dir_path = data.get("scope", "/") + dir_id = os.path.join(dir_path, "__dir__").replace(os.path.sep, "/") + if dir_id.startswith("/"): + dir_id = dir_id[1:] + test_manifest, obj = self.id_path_map[dir_id] + expected_node = self.expected_tree[test_manifest][obj] + + expected_node.set_lsan(self.run_info, (data["frames"], data.get("allowed_match"))) + def create_test_tree(metadata_path, test_manifest, property_order=None, boolean_properties=None): @@ -326,23 +339,60 @@ def create_test_tree(metadata_path, test_manifest, property_order=None, item.item_type is not None] include_types = set(all_types) - exclude_types for _, test_path, tests in test_manifest.itertypes(*include_types): - expected_data = load_expected(test_manifest, metadata_path, test_path, tests, - property_order=property_order, - boolean_properties=boolean_properties) - if expected_data is None: - expected_data = create_expected(test_manifest, - test_path, - tests, - property_order=property_order, - boolean_properties=boolean_properties) - + expected_data = load_or_create_expected(test_manifest, metadata_path, test_path, tests, + property_order, boolean_properties) for test in tests: id_test_map[test.id] = (test_manifest, test) expected_map[test] = expected_data + dir_path = os.path.split(test_path)[0].replace(os.path.sep, "/") + while True: + if dir_path: + dir_id = dir_path + "/__dir__" + else: + dir_id = "__dir__" + dir_id = (test_manifest.url_base + dir_id).lstrip("/") + if dir_id not in id_test_map: + dir_object = DirObject(dir_id, dir_path) + expected_data = load_or_create_expected(test_manifest, + metadata_path, + dir_id, + [], + property_order, + boolean_properties) + + id_test_map[dir_id] = (test_manifest, dir_object) + expected_map[dir_object] = expected_data + if not dir_path: + break + dir_path = dir_path.rsplit("/", 1)[0] if "/" in dir_path else "" + return expected_map, id_test_map +class DirObject(object): + def __init__(self, id, path): + self.id = id + self.path = path + + def __hash__(self): + return hash(self.id) + + +def load_or_create_expected(test_manifest, metadata_path, test_path, tests, property_order=None, + boolean_properties=None): + expected_data = load_expected(test_manifest, metadata_path, test_path, tests, + property_order=property_order, + boolean_properties=boolean_properties) + if expected_data is None: + expected_data = create_expected(test_manifest, + test_path, + tests, + property_order=property_order, + boolean_properties=boolean_properties) + return expected_data + + def create_expected(test_manifest, test_path, tests, property_order=None, boolean_properties=None): expected = manifestupdate.ExpectedManifest(None, test_path, test_manifest.url_base, diff --git a/testing/web-platform/tests/tools/wptrunner/wptrunner/testloader.py b/testing/web-platform/tests/tools/wptrunner/wptrunner/testloader.py index e54053a164f5..b9e2d53469a4 100644 --- a/testing/web-platform/tests/tools/wptrunner/wptrunner/testloader.py +++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/testloader.py @@ -455,6 +455,7 @@ def iterfilter(filters, iter): for item in iter: yield item + class TestLoader(object): def __init__(self, test_manifests,