Andrew McCreight [:mccr8] Reporter
	Description • 7 years ago

Once XPCOM leak checking is enabled for web-platform-tests, and AddressSanitizer builds are running them, too, we should make sure the LeakSanitizer is enabled. It catches some kind of low level buffer leaks that the XPCOM leak checker doesn't find.

	James Graham [:jgraham] Assignee
	Comment 20 • 6 years ago

I just pushed a review for this; I'm not absoutely sure it all works, as I just discovered I forgot to make treeherder/mozharness notice the lsan_* log actions. But particularly given that Maja's going to be away and the code at least nearly works, I think it's ready for review.

The background here is that when we import new web-platform-tests we need a way to ensure that we can make the tree green even though there could be new problems (in this case LSAN) failures. So the approach is to add LSAN to structured logging (via lsan_leak and lsan_summary) actions, and to redirect all the things that can cause failures through there rather than logging strings directly. Then I added the possibility to provide an additional list of stackframes that should cause the leak to be marked as "allowed" (or expected) and added some code in web-platform-tests to update the list of allowed leaks based on the results of a try run. I don't know if there's some better hueristic for picking the best frame from a stack to use as the ignore frame other than just the top one that isn't excluded by the existing exclusions of low-level frames in the LSAN handler.

mccr8 - If this works, how important is the XPCOM leak checking? That works, but probably to enable it I need to repeat the same process of allowing a dynamically updated list of allowed leaks (which is not that hard to do, but of course it's easier to do nothing if LSAN is a pure superset).

	Andrew McCreight [:mccr8] Reporter
	Comment 21 • 6 years ago

Thanks for working on this!

I was going to say that white listing things at the harness level wasn't ideal, but it looks like there are only a handful of failures, and the way that you have them marked looks more finegrained than is available in LSan itself.

Would you like me to file bugs on these leaks? I'm surprised this DNS one shows up in a number of places in WPT, but it isn't a problem elsewhere. It looks like there's only that leak, and some kind of bad worker leak in the file reader directory.

What happens if a leak goes away? Does the list get automatically updated? Also, is there any notification if a new leak shows up?

(In reply to James Graham [:jgraham] from comment #20)
> mccr8 - If this works, how important is the XPCOM leak checking? That works,
> but probably to enable it I need to repeat the same process of allowing a
> dynamically updated list of allowed leaks (which is not that hard to do, but
> of course it's easier to do nothing if LSAN is a pure superset).

We still need XPCOM leak checking because LSan is only run on Linux64. Well, maybe it is run on Windows now, too. LSan will also not report leaks via things held in static variables, while the XPCOM leak checker will.

	Andrew McCreight [:mccr8] Reporter
	Comment 22 • 6 years ago

Also, I'd be interested in a link to a try run, so I could look at what the actual LSan output is for these leaks. Thanks.

	James Graham [:jgraham] Assignee
	Comment 23 • 6 years ago

> What happens if a leak goes away? Does the list get automatically updated? Also, is there any notification if a new leak shows up?

If a leak goes away we don't update the list because of intermittents. That's not ideal, but I don't know what a good solution looks like.

I'll add notification for this in the wpt sync bugs; likely we will extend that to file bugs for specific failures in the second half of this year.

> Also, I'd be interested in a link to a try run, so I could look at what the actual LSan output is for these leaks.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=182fdd5c694a3ad68a9865b2e5a3ab7123761ca6

Is the one I just pushed after fixing the make-the-tree-turn-orange stuff (so hopefully it's orange)

> We still need XPCOM leak checking because LSan is only run on Linux64. Well, maybe it is run on Windows now, too. LSan will also not report leaks via things held in static variables, while the XPCOM leak checker will.

OK. I tried turning that on and didn't see anything, so either I was holding it wrong or I'm going to need a hint on how to add a leak to a custom build so I can write the metadata update parts for that case.

	Andreas Tolfsen ❲:ato❳
	Comment 24 • 6 years ago
mozreview-review

Comment on attachment 8984482 [details]
Bug 1354232 - Support skipping output lines in handlers,

https://reviewboard.mozilla.org/r/250312/#review256606

	Andreas Tolfsen ❲:ato❳
	Comment 25 • 6 years ago
mozreview-review

Comment on attachment 8984483 [details]
Bug 1354232 - Add group_metadata to test group metadata and pass it through to the browser,

https://reviewboard.mozilla.org/r/250314/#review256608

	Andreas Tolfsen ❲:ato❳
	Comment 26 • 6 years ago
mozreview-review

Comment on attachment 8984481 [details]
Bug 1354232 - Log run-by-dir setting,

https://reviewboard.mozilla.org/r/250310/#review256612

	Andreas Tolfsen ❲:ato❳
	Comment 27 • 6 years ago
mozreview-review

Comment on attachment 8984485 [details]
Bug 1354232 - Always try to process the leak log whenever we stop firefox,

https://reviewboard.mozilla.org/r/250318/#review256614

::: commit-message-00d0e:5
(Diff revision 1)
> +Bug 1354232 - Always try to process the leak log whenever we stop firefox, r=ato
> +
> +The `stop` method is always called to shutdown firefox, but the
> +cleanup method is only called at the end of a test run. Therefore we
> +need all the leak processing stuff ot happen in stop().

to

	Andrew McCreight [:mccr8] Reporter
	Comment 28 • 6 years ago

(In reply to James Graham [:jgraham] from comment #23)
> If a leak goes away we don't update the list because of intermittents.
> That's not ideal, but I don't know what a good solution looks like.

That's fine. People will just have to manually edit the lists, which is the same as any other harness.

> https://treeherder.mozilla.org/#/
Thanks.

> OK. I tried turning that on and didn't see anything, so either I was holding
> it wrong or I'm going to need a hint on how to add a leak to a custom build
> so I can write the metadata update parts for that case.

You can delete this line from ipc/chromium/src/chrome/common/ipc_message.cc
  MOZ_COUNT_DTOR(IPC::Message);
and it should register every IPC message as leaked.

	Andrew McCreight [:mccr8] Reporter
	Comment 29 • 6 years ago

Oh, I guess there are more leaks in the final patch besides the worker one and the DNS one.

I also see another failure or two in the logs I'm looking at that don't seem to be captured by the existing white lists. Maybe they are intermittent.

	Andrew McCreight [:mccr8] Reporter
	Comment 30 • 6 years ago
mozreview-review

Comment on attachment 8984477 [details]
Bug 1354232 - Add support for LSAN to mozlog,

https://reviewboard.mozilla.org/r/250302/#review256714

I only looked at the lsan.py changes, but those look reasonable to me.

	Andrew McCreight [:mccr8] Reporter
	Comment 31 • 6 years ago
mozreview-review

Comment on attachment 8984479 [details]
Bug 1354232 - Enable LSAN Leak detection in wpt,

https://reviewboard.mozilla.org/r/250306/#review256728

	Andrew McCreight [:mccr8] Reporter
	Comment 32 • 6 years ago

I just noticed this in one of the logs you linked to:
INFO | LeakSanitizer | SUMMARY: (expected) AddressSanitizer: %d byte(s) leaked in %d allocation(s).
The %d's should be numbers.

	Andrew McCreight [:mccr8] Reporter
	Comment 33 • 6 years ago

"mozilla::BackgroundHangThread::ReportHang"

Leaks like this show up in other LSan tests, so I actually have a patch up for review in bug 1467549 to disable BHR in ASan builds to "fix" this leak.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 34 • 6 years ago
mozreview-review

Comment on attachment 8984480 [details]
Bug 1354232 - Allow wpt manifest files to specify LSAN errors to ignore,

https://reviewboard.mozilla.org/r/250308/#review257132

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 35 • 6 years ago
mozreview-review

Comment on attachment 8984481 [details]
Bug 1354232 - Log run-by-dir setting,

https://reviewboard.mozilla.org/r/250310/#review257150

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 36 • 6 years ago
mozreview-review

Comment on attachment 8984484 [details]
Bug 1354232 - Add support for updating LSAN data in wpt-update

https://reviewboard.mozilla.org/r/250316/#review257152

::: commit-message-9a8af:14
(Diff revision 1)
> +
> +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 humand fixing the issues

humans

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 37 • 6 years ago
mozreview-review

Comment on attachment 8984486 [details]
Bug 1354232 - Support creating ConditionalValue objects containing a list,

https://reviewboard.mozilla.org/r/250320/#review257156

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 38 • 6 years ago
mozreview-review

Comment on attachment 8984487 [details]
Bug 1354232 - Support a __dir__.ini file in the metadata root,

https://reviewboard.mozilla.org/r/250322/#review257158

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 39 • 6 years ago
mozreview-review

Comment on attachment 8984492 [details]
Bug 1354232 - Run web-platform-tests on linux64-asan,

https://reviewboard.mozilla.org/r/250332/#review257160

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 40 • 6 years ago
mozreview-review

Comment on attachment 8984494 [details]
Bug 1354232 - Update metadata for lsan failures,

https://reviewboard.mozilla.org/r/250336/#review257162

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 41 • 6 years ago
mozreview-review

Comment on attachment 8984493 [details]
Bug 1354232 - Chunk by dir more for wpt on asan,

https://reviewboard.mozilla.org/r/250334/#review257164

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 42 • 6 years ago
mozreview-review

Comment on attachment 8984491 [details]
Bug 1354232 - Don't store the result of update_expected,

https://reviewboard.mozilla.org/r/250330/#review257166

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 43 • 6 years ago
mozreview-review

Comment on attachment 8984488 [details]
Bug 1354232 - Fix updating assert count when there's an exising value,

https://reviewboard.mozilla.org/r/250324/#review257168

::: testing/web-platform/tests/tools/wptrunner/wptrunner/manifestupdate.py:475
(Diff revision 1)
>          if old_value is not None:
>              old_value = self.value_type(old_value)
> -        if old_value and new_value < old_value:
> +        if old_value is not None and new_value < old_value:
>              return 0
>          if old_value is None:
>              # If we are getting some asserts for the first time, set the minimum to 0

This comment is incorrect now?

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 44 • 6 years ago
mozreview-review

Comment on attachment 8984489 [details]
Bug 1354232 - Refactor data storage in metadata.py,

https://reviewboard.mozilla.org/r/250326/#review257240

::: testing/web-platform/tests/tools/wptrunner/wptrunner/metadata.py:65
(Diff revision 1)
> -            for tree in expected_map.itervalues():
> -                for test in tree.iterchildren():
> +            for tree in expected:
> +                if not tree.modified:
> +                    continue
> +                for test in expected.iterchildren():
>                      for subtest in test.iterchildren():
>                          if subtest.new_disabled:

I'm not sure about the relationship between `tree`, `test` and `expected`. Seems like the changes result in visiting each test many times, once per modified tree, whereas previously each test was visited only once.

::: testing/web-platform/tests/tools/wptrunner/wptrunner/metadata.py:112
(Diff revision 1)
>      for item in changes:
>          rv[item[1]] = status_keys[item[0]]
>      return rv
>  
>  
>  def unexpected_changes(manifests, change_data, files_changed):

Looks like you can remove this function now.

::: testing/web-platform/tests/tools/wptrunner/wptrunner/metadata.py:331
(Diff revision 1)
>  def create_test_tree(metadata_path, test_manifest, property_order=None,
>                       boolean_properties=None):
> -    expected_map = {}
> +    """Create a map of expectation manifests for all tests in test_manifest,
> +    reading existing manifests under manifest_path
> +
> +    :returns: A map of test_id to (manifest, test, expectation_data)

Misleading comment? It seems to actually be test_id to (manifest, expectation_data)... and you get retrieve the test from expectation_data.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 45 • 6 years ago
mozreview-review

Comment on attachment 8984490 [details]
Bug 1354232 - Fix, enable, and add to, the manifest update tests,

https://reviewboard.mozilla.org/r/250328/#review257296

	Andrew Halberstadt [:ahal]
	Comment 46 • 6 years ago
mozreview-review

Comment on attachment 8984476 [details]
Bug 1354232 - Copy LSANLeaks to mozleak,

https://reviewboard.mozilla.org/r/250300/#review257300

	Andrew Halberstadt [:ahal]
	Comment 47 • 6 years ago
mozreview-review

Comment on attachment 8984477 [details]
Bug 1354232 - Add support for LSAN to mozlog,

https://reviewboard.mozilla.org/r/250302/#review257302

::: testing/mozbase/mozlog/mozlog/formatters/machformatter.py:228
(Diff revision 1)
>          return rv
>  
> +    def lsan_leak(self, data):
> +        allowed = data.get("allowed_match")
> +        if allowed:
> +            prefix = self.term.yellow("FAIL")

Should this be more obvious that the failure is allowed? Not really sure what `allowed_match` signifies.

::: testing/mozbase/mozlog/mozlog/formatters/tbplformatter.py:282
(Diff revision 1)
>  
> +    def lsan_leak(self, data):
> +        frames = data.get("frames")
> +        allowed_match = data.get("allowed_match")
> +        frame_list = ", ".join(frames)
> +        prefix = "TEST-UNEXPECTED-FAIL" if not allowed_match else "TEST-FAIL"

Ditto. Should this be TEST-KNOWN-FAIL or similar?

	Andrew Halberstadt [:ahal]
	Comment 48 • 6 years ago
mozreview-review

Comment on attachment 8984478 [details]
Bug 1354232 - Ensure lsan failures cause treeherder jobs to fail,

https://reviewboard.mozilla.org/r/250304/#review257306

	Andrew Halberstadt [:ahal]
	Comment 49 • 6 years ago
mozreview-review

Comment on attachment 8984479 [details]
Bug 1354232 - Enable LSAN Leak detection in wpt,

https://reviewboard.mozilla.org/r/250306/#review257308

	James Graham [:jgraham] Assignee
	Comment 50 • 6 years ago

(In reply to Andrew Halberstadt [:ahal] from comment #47)
> Comment on attachment 8984477 [details]
> Bug 1354232 - Add support for LSAN to mozlog,
> 
> https://reviewboard.mozilla.org/r/250302/#review257302
> 
> ::: testing/mozbase/mozlog/mozlog/formatters/machformatter.py:228
> (Diff revision 1)
> >          return rv
> >  
> > +    def lsan_leak(self, data):
> > +        allowed = data.get("allowed_match")
> > +        if allowed:
> > +            prefix = self.term.yellow("FAIL")
> 
> Should this be more obvious that the failure is allowed? Not really sure
> what `allowed_match` signifies.

I wasn't sure what wording to use. The content here is the topmost frame in the stack that matched a rule indicating that the leak is expected. So I'm very happy to hear alternative suggestions, but this was the best I had.

> 
> ::: testing/mozbase/mozlog/mozlog/formatters/tbplformatter.py:282
> (Diff revision 1)
> >  
> > +    def lsan_leak(self, data):
> > +        frames = data.get("frames")
> > +        allowed_match = data.get("allowed_match")
> > +        frame_list = ", ".join(frames)
> > +        prefix = "TEST-UNEXPECTED-FAIL" if not allowed_match else "TEST-FAIL"
> 
> Ditto. Should this be TEST-KNOWN-FAIL or similar?

We don't use that in other places (except in assertion checks, for some reason). So I guess it's not harmful, but it isn't what I would consider the precedent set by the existing behaviour.

	Pulsebot
	Comment 51 • 6 years ago

Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05441f0a29b3
Copy LSANLeaks to mozleak, r=ahal
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eda80370f59
Add support for LSAN to mozlog, r=ahal, mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d28db898070
Ensure lsan failures cause treeherder jobs to fail, r=ahal
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d106c2112d
Enable LSAN Leak detection in wpt, r=ahal,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8da0ef2695d
Allow wpt manifest files to specify LSAN errors to ignore, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b9ba77b7316
Log run-by-dir setting, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c976baa7194
Support skipping output lines in handlers, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/07049ac67119
Add group_metadata to test group metadata and pass it through to the browser, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e8a66385f66
Add support for updating LSAN data in wpt-update r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc292a22975
Always try to process the leak log whenever we stop firefox, r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ebcf01626a
Support creating ConditionalValue objects containing a list, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e161374b583
Support a __dir__.ini file in the metadata root, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2a7154454c
Fix updating assert count when there's an exising value, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf4a20193b8
Refactor data storage in metadata.py, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/5028820efd9e
Fix, enable, and add to, the manifest update tests, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/004b079b76d0
Don't store the result of update_expected, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d5ed234247c
Run web-platform-tests on linux64-asan, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7dec081ead2
Chunk by dir more for wpt on asan, r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5720fa6da9
Update metadata for lsan failures, r=maja_zf

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 52 • 6 years ago

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12073 for changes under testing/web-platform/tests

	Pulsebot
	Comment 53 • 6 years ago

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9193e72be19
Annotate html/infrastructure/urls/resolving-urls/query-encoding/navigation.sub.html as passing on Linux asan. r=RyanVM on IRC

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 54 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/05441f0a29b3
https://hg.mozilla.org/mozilla-central/rev/6eda80370f59
https://hg.mozilla.org/mozilla-central/rev/6d28db898070
https://hg.mozilla.org/mozilla-central/rev/e0d106c2112d
https://hg.mozilla.org/mozilla-central/rev/b8da0ef2695d
https://hg.mozilla.org/mozilla-central/rev/5b9ba77b7316
https://hg.mozilla.org/mozilla-central/rev/7c976baa7194
https://hg.mozilla.org/mozilla-central/rev/07049ac67119
https://hg.mozilla.org/mozilla-central/rev/9e8a66385f66
https://hg.mozilla.org/mozilla-central/rev/8cc292a22975
https://hg.mozilla.org/mozilla-central/rev/c1ebcf01626a
https://hg.mozilla.org/mozilla-central/rev/0e161374b583
https://hg.mozilla.org/mozilla-central/rev/0f2a7154454c
https://hg.mozilla.org/mozilla-central/rev/6bf4a20193b8
https://hg.mozilla.org/mozilla-central/rev/5028820efd9e
https://hg.mozilla.org/mozilla-central/rev/004b079b76d0
https://hg.mozilla.org/mozilla-central/rev/9d5ed234247c
https://hg.mozilla.org/mozilla-central/rev/a7dec081ead2
https://hg.mozilla.org/mozilla-central/rev/7a5720fa6da9

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 55 • 6 years ago

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/12073
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/405927601?utm_source=github_status&utm_medium=notification)

	Tiberius Oros[:tiberius_oros]
	Comment 56 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b9193e72be19

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 57 • 6 years ago

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/12073
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/407114828?utm_source=github_status&utm_medium=notification)

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 58 • 6 years ago

Upstream PR merged

	Pulsebot
	Comment 59 • 6 years ago

Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f271db900a41
[wpt PR 12073] - [Gecko Bug 1354232] Enable LSAN Leak detection in wpt, a=testonly

	Andreea Pavel [:apavel]
	Comment 60 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f271db900a41