Closed Bug 736978 Opened 8 years ago Closed 8 years ago

Remove JS_FinalizeStub


(Core :: JavaScript Engine, defect)

Not set





(Reporter: igor, Assigned: igor)



(1 file)

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.
Looks like DOM workers use it
Attached patch v1Splinter 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.
Assignee: general → igor
Attachment #607158 - Flags: review?(wmccloskey)
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?
Attachment #607158 - Flags: review?(wmccloskey) → review+
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.
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.
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.
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.