Closed Bug 981167 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mccr8, Assigned: shu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lsan][MemShrink])

Attachments

(3 files)

Attached file 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.
I introduced this leak when optimizing stack walking times for Nick's tracer.
I'm filled with much shame.
Attachment #8387975 - Flags: review?(jimb)
Assignee: nobody → shu
Status: NEW → ASSIGNED
Whiteboard: [lsan][MemShrink]
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.)
(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!
Try run looks clean.
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... :( )
Attachment #8387975 - Flags: review?(jimb) → review+
(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
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)
Attachment #8388440 - Flags: review?(jimb)
Note to self: need to disable eval-in-frame for remat frames.
Attachment #8388440 - Flags: review?(jimb) → review+
Depends on: 983558
You need to log in before you can comment on or make changes to this bug.