Add report_objects=1 to LSan options

RESOLVED FIXED in Firefox 50

Status

()

Core
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jandem, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8649301 [details] [diff] [review]
Patch

I used this to track down bug 1194627. With this option enabled, LSan prints the following after the leak report:

13:19:50     INFO -  Objects leaked above:
13:19:50     INFO -  0x60600055a120 (64 bytes)

Knowing the address is extremely useful because you can then add logging to the allocation site to find out more.
Attachment #8649301 - Flags: review?(continuation)
(Assignee)

Comment 1

3 years ago
That's a nice feature I didn't know about.

It seems a little odd to print out the address all the time when it is not useful without additional logging. XPCOM leak logging can also report addresses, but does not do it by default.

Also, can you show a little more context for the output, or maybe link to a log? I'm curious what comes before and after it. I'm just worried if it will interfere with our LSan report parsing.
(Reporter)

Comment 2

3 years ago
(In reply to Andrew McCreight [:mccr8] from comment #1)
> It seems a little odd to print out the address all the time when it is not
> useful without additional logging. XPCOM leak logging can also report
> addresses, but does not do it by default.

Fair enough, but we're already reporting the stack trace at the allocation site, that's a lot more information than just the address right?

> Also, can you show a little more context for the output, or maybe link to a
> log? I'm curious what comes before and after it. I'm just worried if it will
> interfere with our LSan report parsing.

Sure, here's a report (search for `leaked`):

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jandemooij@gmail.com-0bf12f3b1c72/try-linux64-asan/try_ubuntu64-asan_vm_test-mochitest-e10s-browser-chrome-3-bm116-tests1-linux64-build165.txt.gz
(Reporter)

Comment 3

3 years ago
(In reply to Jan de Mooij [:jandem] from comment #2)
> (In reply to Andrew McCreight [:mccr8] from comment #1)
> > It seems a little odd to print out the address all the time when it is not
> > useful without additional logging. XPCOM leak logging can also report
> > addresses, but does not do it by default.
> 
> Fair enough, but we're already reporting the stack trace at the allocation
> site, that's a lot more information than just the address right?

Oh sorry I missed the "without additional logging" part. That's a good point, though most people won't know about report_objects=1. Maybe we can print a line "Use report_objects=1 to ..." or add it to some wiki page.
(Assignee)

Comment 4

3 years ago
Created attachment 8649363 [details] [diff] [review]
When LSan detects leaks, tell the user about the report_objects option.

That's a good idea. How does something like this look to you?
Attachment #8649363 - Flags: feedback?(jdemooij)
(Reporter)

Comment 5

3 years ago
Comment on attachment 8649363 [details] [diff] [review]
When LSan detects leaks, tell the user about the report_objects option.

Review of attachment 8649363 [details] [diff] [review]:
-----------------------------------------------------------------

Yep this seems fine, thanks.

It adds the same number of lines though and requires additional work when someone needs this, but I don't have a strong opinion on it :)
Attachment #8649363 - Flags: feedback?(jdemooij) → feedback+
(Assignee)

Comment 6

3 years ago
(In reply to Jan de Mooij [:jandem] from comment #5)
> It adds the same number of lines though and requires additional work when
> someone needs this, but I don't have a strong opinion on it :)

It adds less lines in the case where we are leaking many objects (sometimes we leak hundreds of them), though in that case there are going so many stacks it may not really matter. I'll pick this up from here and get it landed, and add something to our wiki about it. Thanks for suggesting this!
Assignee: jdemooij → continuation
(Assignee)

Comment 7

3 years ago
Comment on attachment 8649301 [details] [diff] [review]
Patch

I'll try my alternate approach here.
Attachment #8649301 - Attachment is obsolete: true
Attachment #8649301 - Flags: review?(continuation)
(Assignee)

Comment 8

2 years ago
Created attachment 8775302 [details] [diff] [review]
When LSan detects leaks, tell the user about the report_objects option.
Attachment #8775302 - Flags: review?(cmanchester)
Comment on attachment 8775302 [details] [diff] [review]
When LSan detects leaks, tell the user about the report_objects option.

Review of attachment 8775302 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/leaks.py
@@ +222,5 @@
>  
>      def process(self):
> +        if self.foundFrames:
> +            self.logger.info("TEST-INFO | LeakSanitizer | To show the addresses of leaked objects add report_objects=1 to LSAN_OPTIONS")
> +            self.logger.info("TEST-INFO | LeakSanitizer | This can be done in testing/mozbase/mozrunner/mozrunner/utils.py")

This message seems likely to bitrot, but I guess there's no harm done.
Attachment #8775302 - Flags: review?(cmanchester) → review+

Comment 10

2 years ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcc56a97afb3
When LSan detects leaks, tell the user about the report_objects option. r=chmanchester

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fcc56a97afb3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.