Last Comment Bug 713068 - IonMonkey: Invalidation leaks IonScripts
: IonMonkey: Invalidation leaks IonScripts
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: IonOSI
  Show dependency treegraph
Reported: 2011-12-22 12:30 PST by David Anderson [:dvander]
Modified: 2011-12-27 17:57 PST (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix (8.96 KB, patch)
2011-12-22 18:07 PST, David Anderson [:dvander]
cdleary: review+
Details | Diff | Splinter Review

Description User image David Anderson [:dvander] 2011-12-22 12:30:57 PST
IonScripts are not gcthings, so when we invalidate, and detach them from JSScripts, they don't get freed later. Simple solution is to just refcount them.
Comment 1 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-22 14:05:27 PST
Is this instead of the HandleException walk-and-free approach? I'm just curious because performing decref operations appropriately doesn't seem inherently easier than performing a free operation appropriately.
Comment 2 User image David Anderson [:dvander] 2011-12-22 14:13:58 PST
Nah they're orthogonal. We'd incref the IonScript when we create an InvalidationRecord for it, and decref the IonScript when we destroy the record. The HandleException approach is fine we just have to destroy the IonScripts too.
Comment 3 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-22 14:58:58 PST
Oh right. At least that's nice and limited in scope -- don't need to worry about keeping a refcount until it actually becomes invalidated.
Comment 4 User image David Anderson [:dvander] 2011-12-22 18:07:51 PST
Created attachment 583982 [details] [diff] [review]
Comment 5 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-22 22:03:56 PST
Comment on attachment 583982 [details] [diff] [review]

Review of attachment 583982 [details] [diff] [review]:

::: js/src/ion/IonFrames.h
@@ +166,5 @@
>      void *calleeToken;
>      uint8 *returnAddress;
>      IonScript *ionScript;
>      InvalidationRecord(void *calleeToken, uint8 *returnAddress);

Should we privatize this stuff now?
Comment 6 User image David Anderson [:dvander] 2011-12-27 17:57:59 PST
I tried :( unfortunately new_ requires public visibility.

Note You need to log in before you can comment on or make changes to this bug.