981167 - js::ScriptFrameIter::Data leaks in Mochitest-browser-chrome

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

Attachment: leaking stacks

Looks like we leak about 300-400KB of these during the course of m-bc.

This was detected with LSAN.  To run LSAN, make an ASAN build, then run with ASAN_OPTIONS="detect_leaks=1" and MOZ_CC_RUN_DURING_SHUTDOWN=1.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Fix Debugger.Frame leaking ScriptFrameIter::Data on frame cache hit.

I introduced this leak when optimizing stack walking times for Nick's tracer.
I'm filled with much shame.

Comment 2

[Andrew McCreight [:mccr8]]

Here's an LSAN try run:
   https://tbpl.mozilla.org/?tree=Try&rev=a9a80c84b8b5
You should be able to go into the full log for M-bc when it finishes and see if there are still any FrameIter things in the leak stacks. (Don't be alarmed when it is entirely orange, that's normal.)

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Here's an LSAN try run:
>    https://tbpl.mozilla.org/?tree=Try&rev=a9a80c84b8b5
> You should be able to go into the full log for M-bc when it finishes and see
> if there are still any FrameIter things in the leak stacks. (Don't be
> alarmed when it is entirely orange, that's normal.)

Thanks!

Comment 4

[Shu-yu Guo [:shu]]

Try run looks clean.

Comment 5

[Jim Blandy :jimb]

Comment on attachment 8387975 [details] [diff] [review]
Fix Debugger.Frame leaking ScriptFrameIter::Data on frame cache hit.

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

Looks good to me!

(Then again, I was the reviewer for the buggy code, too... :( )

Comment 6

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7de39a071ca6

Comment 7

[Carsten Book [:Tomcat]]

(In reply to Shu-yu Guo [:shu] from comment #6)
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7de39a071ca6

sorry had to backout this cset for causing test bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=35873363&tree=Mozilla-Inbound

Comment 8

[Shu-yu Guo [:shu]]

Attachment: Followup: fix props/methods that need pc updated

The current Debugger spec expects the |frame.environment| to pick out the
*current* scope of the frame, i.e. at the currently executing PC. The failing
test is calling out to the onDebugger handler in nested block (let) scopes.

This patch fixed the leak behavior, which consequently fixed the cache behavior
as well, so we were caching stale pcs on the saved ScriptFrameIter::Data inside
the Debugger.Frame objects.

I audited the properties and methods on D.F to see which ones needed their pcs
updated:

- type: no
- older: no
- depth: no
- live: no
- script: no
- offset: yes (already calls updatePcQuadratic)
- environment: yes (fixed)
- callee: no
- generator: no
- constructing: no
- arguments: no
- eval: yes (fixed)
- evalWithBindings: yes (fixed)

Comment 9

[Shu-yu Guo [:shu]]

Note to self: need to disable eval-in-frame for remat frames.

Comment 10

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dfc89871248c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7265b36257e5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cafc246cc62c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dfc89871248c
https://hg.mozilla.org/mozilla-central/rev/7265b36257e5
https://hg.mozilla.org/mozilla-central/rev/cafc246cc62c