Last Comment Bug 660441 - removal of cx parameter from IsAboutToBeFinalized
: removal of cx parameter from IsAboutToBeFinalized
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-28 04:07 PDT by Igor Bukanov
Modified: 2012-02-08 09:01 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (23.85 KB, patch)
2011-05-28 10:21 PDT, Igor Bukanov
no flags Details | Diff | Review
v2 (277.28 KB, patch)
2012-02-07 14:08 PST, Igor Bukanov
no flags Details | Diff | Review
v3 (40.83 KB, patch)
2012-02-07 14:34 PST, Igor Bukanov
anygregor: review+
Details | Diff | Review

Description Igor Bukanov 2011-05-28 04:07:57 PDT
IsAboutToBeFinalized takes cx to access the runtime and get the associated gcCurrentCompartment. Howerever, the runtime can be accessed from the cpmpartment for the thing making the cx parameter unnecessary.
Comment 1 Gregor Wagner [:gwagner] 2011-05-28 10:20:23 PDT
Do you also want to change the JS_IsAboutToBeFinalized function? Whats the procedure to change an API function?
Comment 2 Igor Bukanov 2011-05-28 10:21:27 PDT
Created attachment 535871 [details] [diff] [review]
v1

Here is the patch that removes the unneeded cx parameter.
Comment 3 Igor Bukanov 2011-05-28 13:10:13 PDT
(In reply to comment #1)
> Do you also want to change the JS_IsAboutToBeFinalized function?

Thanks for remainding about it! I will update the patch to always call IsAboutToBeFinalized from xpconnect. After that we would not have JS_IsAboutToBeFinalized calls in the browser and we can wait with any changes to JS_IsAboutToBeFinalized until we have a generational GC or something like that that disrupts the GC logic significantly. 

> Whats the
> procedure to change an API function?

Some discussion on internal and public mail lists is in due.
Comment 4 Igor Bukanov 2012-02-07 14:08:42 PST
Created attachment 595170 [details] [diff] [review]
v2

The patch removes the cx from all internal and public API - per resent discussion on the mailing list this is OK and desirable.
Comment 5 Igor Bukanov 2012-02-07 14:34:57 PST
Created attachment 595186 [details] [diff] [review]
v3

Here is the right patch!
Comment 6 Gregor Wagner [:gwagner] 2012-02-07 16:24:30 PST
Comment on attachment 595186 [details] [diff] [review]
v3


>-{
>-    JS_ASSERT(thing);
>-    JS_ASSERT(!cx->runtime->gcIncrementalTracer);
>-    return IsAboutToBeFinalized(cx, (gc::Cell *)thing);
>+JS_IsAboutToBeFinalized(void *thing)
>+{
>+    gc::Cell * t = static_cast<gc::Cell *>(thing);

Space around *?

nice!
Comment 8 Ed Morley [:emorley] 2012-02-08 09:01:43 PST
https://hg.mozilla.org/mozilla-central/rev/d6f1d1fbe13a

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