Last Comment Bug 736978 - Remove JS_FinalizeStub
: Remove JS_FinalizeStub
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
-- normal (vote)
: mozilla14
Assigned To: Igor Bukanov
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-03-19 04:11 PDT by Igor Bukanov
Modified: 2012-03-20 03:55 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (46.06 KB, patch)
2012-03-19 07:34 PDT, Igor Bukanov
wmccloskey: review+
Details | Diff | Splinter Review

Description User image Igor Bukanov 2012-03-19 04:11:02 PDT
Currently the GC finalizes on the background thread only objects with null JSClass::finalize. However, this implies that any object that uses JS_FinalizeStub for the finalizer would be prevented from the background finalization.

To fix this I suggest to remove JS_FinalizeStub so the code would use NULL if the class has no custom finalizer.
Comment 1 User image :Ms2ger (⌚ UTC+1/+2) 2012-03-19 04:38:08 PDT
Looks like DOM workers use it
Comment 2 User image Igor Bukanov 2012-03-19 07:34:30 PDT
Created attachment 607158 [details] [diff] [review]

The patch removes JS_FinalizeStub and replaces it with NULL in the static class definitions or just drops the field if this is the last non-null field. For style consistency the patch also removes JSCLASS_NO_OPTIONAL_MEMBERS in all static class declarations. Currently some places use it and some do not.
Comment 3 User image Bill McCloskey (:billm) 2012-03-19 10:52:26 PDT
Comment on attachment 607158 [details] [diff] [review]

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

Was there ever any reason for JSCLASS_NO_OPTIONAL_MEMBERS to exist?
Comment 4 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-19 11:00:12 PDT
Some compilers let you make brace-initialization of a struct without specifying all members a warning (and, usually, an error with a warnings-as-errors configuration).  It's possible some embedders build with such a configuration, or that some compilers have a problem with it anyway.  (It also might be the case that brace-initialization of non-static variables doesn't zero out trailing members, in which case omitting trailing members could be Bad.)  But I think if we wanted to support that use case, we should turn on the warning for ourselves so that we have something enforcing the correctness of things like JSCLASS_NO_OPTIONAL_MEMBERS.
Comment 5 User image Bill McCloskey (:billm) 2012-03-19 11:03:35 PDT
It looks like we have classes in the JS engine that already don't use JSCLASS_NO_OPTIONAL_MEMBERS. Nobody has complained, so I guess it's probably okay.
Comment 6 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-19 11:08:45 PDT
Yeah, I agree.  It's an attractive nuisance at best now, and if we're not verifying it works by using it ourselves, we should just remove it.  Embedder update cycles are long enough that it probably isn't too much burden on most of them to reverse-engineer something from time to time if necessary.
Comment 8 User image Mounir Lamouri (:mounir) 2012-03-20 03:55:09 PDT

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