[This is a spin-off of bug 341821 comment 12 and comment 21.] Currently close hooks are exposed as public API through JSExtendedClass.close. It allows for any class to define them. Unfortunately since close hooks can run arbitrary code, to prevent denial-of-service and potentially other problems an embedding may want skip them. But with native hooks this would inevitably leads to leaks and other hazards unless one very carefully define them. Thus the idea is to remove JSExtendedClass.close and support close hooks required by generator implementation through an internal API at least until all details of close hook protocol are ironed out.

Comment on attachment 231256 [details] [diff] [review] Implementation v1 >- JSCloseOp close; >- jsword reserved0; >+ void (*reserved0)(); > jsword reserved1; > jsword reserved2; > jsword reserved3; >+ jsword reserved4; Use jsword reserved0 to match -- we do require a jsword to be as big as a function pointer on all supported architectures, and it's more consistent with the rest of the API. > #define JSCLASS_HAS_PRIVATE (1<<0) /* objects have private slot */ > #define JSCLASS_NEW_ENUMERATE (1<<1) /* has JSNewEnumerateOp hook */ > #define JSCLASS_NEW_RESOLVE (1<<2) /* has JSNewResolveOp hook */ > #define JSCLASS_PRIVATE_IS_NSISUPPORTS (1<<3) /* private is (nsISupports *) */ > #define JSCLASS_SHARE_ALL_PROPERTIES (1<<4) /* all properties are SHARED */ > #define JSCLASS_NEW_RESOLVE_GETS_START (1<<5) /* JSNewResolveOp gets starting Where is the change to JSCLASS_NO_RESERVED_MEMBERS, and inline equivalent change(s) to xpconnect/src/xpcwrappednativejsops.cpp and possibly other places (liveconnect)? >+ /* From this point quit-on-failures code must go through label bad. */ >+ Typical style does not put a blank line here. > #if JS_HAS_GENERATORS > /* Expose Iterator and initialize the generator internals if configured. */ >- proto = JS_InitClass(cx, obj, NULL, &js_IteratorClass, Iterator, 2, >- NULL, iterator_methods, NULL, NULL); >+ proto = JS_InitClass(cx, obj, NULL, &js_IteratorClass, Iterator, 2, NULL, >+ iterator_methods, NULL, NULL); Please don't make unnecessary changes, esp. here where the wrapping intentionally grouped methods/props/static-methods/static-props actual args on the second line. /be

(In reply to comment #2) > (From update of attachment 231256 [details] [diff] [review] [edit]) > >- JSCloseOp close; > >- jsword reserved0; > >+ void (*reserved0)(); > > jsword reserved1; > > jsword reserved2; > > jsword reserved3; > >+ jsword reserved4; > > Use jsword reserved0 to match -- we do require a jsword to be as big as a > function pointer on all supported architectures, and it's more consistent with > the rest of the API. On Amd-64 there is an option to use 32 bits for function pointers and 64 bits for data pointers. This becomes popular since GCC started to support this. Using jsword would waster memory. But I guess I should then replace all reserved members to use function pointers. > > > #define JSCLASS_HAS_PRIVATE (1<<0) /* objects have private slot */ > > #define JSCLASS_NEW_ENUMERATE (1<<1) /* has JSNewEnumerateOp hook */ > > #define JSCLASS_NEW_RESOLVE (1<<2) /* has JSNewResolveOp hook */ > > #define JSCLASS_PRIVATE_IS_NSISUPPORTS (1<<3) /* private is (nsISupports *) */ > > #define JSCLASS_SHARE_ALL_PROPERTIES (1<<4) /* all properties are SHARED */ > > #define JSCLASS_NEW_RESOLVE_GETS_START (1<<5) /* JSNewResolveOp gets starting > > Where is the change to JSCLASS_NO_RESERVED_MEMBERS, and inline equivalent > change(s) to xpconnect/src/xpcwrappednativejsops.cpp and possibly other places > (liveconnect)? I wanted to make the patch as small as possible. Since the current code already uses NULL for JSExtendedClass.close, changing JSCLASS_NO_RESERVED_MEMBERS would require to patch few other users of JSExtendedClass indeed.

(In reply to comment #3) > On Amd-64 there is an option to use 32 bits for function pointers and 64 bits > for data pointers. This becomes popular since GCC started to support this. > Using jsword would waster memory. But I guess I should then replace all > reserved members to use function pointers. Ok, do that then -- it'll help avoid changing JSCLASS_NO_RESERVED_MEMBERS and those fooconnect files. Thanks, /be

Update to resolve issues from comment 2. The patch continue not to touch JSCLASS_NO_RESERVED_MEMBERS. Is it how should I interpret comment 4?

(In reply to comment #4) > Ok, do that then -- it'll help avoid changing JSCLASS_NO_RESERVED_MEMBERS and > those fooconnect files. Thanks, But what is value of JSCLASS_NO_RESERVED_MEMBERS? If one of the reserved fields in future would be taken, then all users of the macro should be updated as well. It seems just writing NULL for reserved fields would provide best source and binary compatibility.

Please ignore the prev patch: I forgot to run the diff there.

The only benefit to JSCLASS_NO_*_MEMBERS macros is if the number of optional trailing (null-initialized), or even most-frequently-unused-optional trailing null-initialized members, is fixed. If that's not likely to ever be the case, then both macros lose. It may be that JSCLASS_NO_OPTIONAL_MEMBERS took root after JSClass had stabilized -- I didn't invent it, and it did come in years after, so this might be a useful macro just to avoid typing lots of comma-separated NULLs. If we are not ready to reap similar minor benefit from JSCLASS_NO_RESERVED_MEMBERS, then I agree it's going to be a pain. But I don't think we can withdraw it now -- jsapi.h is mostly frozen and never shrinks over time. Will review in a second. /be

Comment on attachment 231428 [details] [diff] [review] Implementation v2 for real Looks good, we can leave JSCLASS_NO_RESERVED_MEMBERS alone and see if anyone in the wider world of SpiderMonkey embedding complains. /be

(In reply to comment #9) > But I don't think we can withdraw it now -- jsapi.h is > mostly frozen and never shrinks over time. For extra compatibility I suggest to revert JSCLASS_NO_RESERVED_MEMBERS to the form it has on 1.8.0 branch, where it was defined to be 0,0,0,0,0 covering all 5 reserved members and fix all places (just 2 files AFAICS) in the browser tree to use explicit nulls.

This version of the patch reverts JSCLASS_NO_RESERVED_MEMBERS to the form "0,0,0,0,0" it has on 1.8.0 branch and changes 4 places in the browser tree that use the macro to use explit nulls.

Comment on attachment 231433 [details] [diff] [review] Implementation v3 Still looks good to me. Thanks, /be

Yet another patch update. First, CVS commits required an update of the patch. Second, the previous version of the patch assumed that the patch from bug 341821 was already committed. But changing the order simplifies both patches since patch from 341821 changed many places that this patch would simply remove.

Comment on attachment 231561 [details] [diff] [review] Implementation v4 >+ /* >+ * After the following 3 lines quit-on-failures code must go through label >+ * "bad". >+ */ >+ obj = js_NewObject(cx, &js_GeneratorClass, NULL, NULL); > if (!obj) > return NULL; How about "After this return, failing control flow must goto bad." and fit it on one line? >+ >+ if (!JS_SetPrivate(cx, obj, gen)) { >+ JS_free(cx, gen); >+ goto bad; >+ } Did you have to separate this just to avoid the possibility of a partly initialized private data struct being scanned by generator_mark? In any case JS_SetPrivate is infallible, and we've been paying failure test taxes on it for too long. I would be ok with just calling it with a lead-in "/* JS_SetPrivate is infallible. */" comment. >+ if (!js_RegisterGeneratorObject(cx, obj)) { >+ /* >+ * We must not free gen here as after successful JS_SetPrivate it is >+ * the job for the finalizer. Nit: "job of the finalizer". Also, comma after "here". Could shorten by avoiding passive voice: "Do not free gen here, as the finalizer will do that since we called JS_SetPrivate." Looks good, glad to see this all simplifying in code and data size. /be

(In reply to comment #14) > > Did you have to separate this just to avoid the possibility of a partly > initialized private data struct being scanned by generator_mark? Yes, plus there is a hidden agenda here: I want to minimize the patch from 341821 were JSGenerator is used to store close list link fields. In any case > JS_SetPrivate is infallible, and we've been paying failure test taxes on it for > too long. I think this is a job for a separated bug that would replaces all such cases in one go and either fix JS_SetPrivate signature or add new infallable API.

(In reply to comment #15) > I think this is a job for a separated bug that would replaces all such cases in > one go and either fix JS_SetPrivate signature or add new infallable API. I don't think fixing the signature is really viable (backwards compatibility and all that), but perhaps just documenting that JS_SetPrivate never returns anything other than JS_TRUE, and cannot fail would avoid having to make a new API?

This is a patch to commit with comment nits addresses

I filed bug 346914 about SetPrivate cleanup.

I committed the patch from comment 17 to the trunk.

Comment on attachment 231645 [details] [diff] [review] Implementation v4b The patch effectively removes a premature public API that was introduced during JS1.7 development.

Comment on attachment 231645 [details] [diff] [review] Implementation v4b This is needed for js1.7, not just for API hygiene but for further fixing. /be

Comment on attachment 231645 [details] [diff] [review] Implementation v4b a=dbaron on behalf of drivers. Please check in to MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have done so.

Patch for to 1.8.1 branch. The only difference compared with the trunk version is that this patch does notchange js/src/js.c. On the branch the file does not use JSExtendedClass so there is no need to change anything there. Still given it is not the same patch as for the trunk I ask for approval again.

The previous version of the patch for 1.8.1 branch caused a warning that js_ReportUncaughtException was undeclared. It turned out that 1.8.1 version of js/src/jsgc.c did not include "jsexn.c". The new version fixes this missed include.

Comment on attachment 232419 [details] [diff] [review] Implementation v4b - 1.8.1 version (b) a=schrep for drivers.

I committed the patch from comment 24 to MOZILLA_1_8_BRANCH.

I think we should sync js.c from trunk to 1.8 branch -- it's not part of the product builds, but used by the test suite. It can be checked in without seeking approval. I'll leave it to mrbkap and igor to do. /be