736978 - Remove JS_FinalizeStub

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looks like DOM workers use it

Comment 2

[Igor Bukanov]

Attachment: v1

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 607158 [details] [diff] [review]
v1

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

Was there ever any reason for JSCLASS_NO_OPTIONAL_MEMBERS to exist?

Comment 4

[Jeff Walden [:Waldo]]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

[Jeff Walden [:Waldo]]

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 7

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0782c1f3d5

Comment 8

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/ca0782c1f3d5