To be precise, when we go to init the scope for our very first window, we end up GCing from under WrapNative; during this GC we have bogus XPCNativeInterfaces around. These come about because XPCNativeSet::GetNewOrUsed(XPCCallContext& ccx, nsIClassInfo* classInfo) allocates an array of XPCNativeInterfaces but doesn't auto-mark them during this process; it also doesn't auto-mark them when we go to create the actual set. Just auto-marking the objects makes things work.

Oh, you have to apply the patch in bug 305884 to get this far.

I wish I could do this without the extra heap allocation, but I don't see how... :(

Comment on attachment 195108 [details] [diff] [review] Proposed patch > if(root) > { // scoped lock >+ printf("Created nsXPCWrappedJS %p, JSObject is %p\n", >+ (void*)wrapper, (void*)aJSObj); > XPCAutoLock lock(rt->GetMapLock()); > map->Add(root); > } Did you mean to show this printf and the next two in the patch? >+ interfaceMarkerArray = >+ NS_STATIC_CAST(AutoMarkingNativeInterfacePtr*, >+ malloc(sizeof(AutoMarkingNativeInterfacePtr)*iidCount)); >+ if(!interfaceMarkerArray) >+ goto out; No way to use JSAutoLocalRootScope? If no, how about alloca for the storage? Should be portable (knock on wood). >+ if(interfaceMarkerArray) { Prevailing style puts left brace on its own line. /be

> Did you mean to show this printf and the next two in the patch? Er, no. Ignore that part. ;) > No way to use JSAutoLocalRootScope? Nope. The problem is not JS GC per se, since the XPCNativeInterfaces are not JS gcthings. The problem is that the GC callback ends up doing XPConnect's own marking and reaping and that's what we need to protect against... I suppose I could use alloca if this is really performance sensitive. > Prevailing style puts left brace on its own line. Will do.

Obviously my bad 4 years ago. I don't think the extra allocation here is that big a deal. XPCNativeSet creation can't be happenning all that often - they are shared and cached. Do you have numbers? I'd suggest using new[]/delete[] rather than malloc/free to be consistent with other code here; e.g. the interfaceArray management. If you want to get fancy, you could fairly easily make an AutoMarkingNativeInterfacePtrArray class to use for the interfaceArray that would mark whatever XPCNativeInterface ptrs had yet been added rather than having the primary array *and* an AutoMarking array. But, that might be overkill :)

> XPCNativeSet creation can't be happenning all that often - they are > shared and cached. Do you have numbers? No numbers, no. I agree that it should be pretty rare, though, based on reading the code... > I'd suggest using new[]/delete[] rather than malloc/free to be consistent with > other code here; I need an array of objects that have no zero-argument constructors; I wasn't sure I could do that with new[]/delete[]... Using a separate class that marks the array we already have might not be a bad idea, on the other hand... If reviewers would prefer that, I'll do it.

(In reply to comment #6) > Using a separate class that marks the array we already have might not be a bad > idea, on the other hand... If reviewers would prefer that, I'll do it. I'm for it. /be

Comment on attachment 195322 [details] [diff] [review] Per the comments Looks good to me, just a few nits: >- for(PRUint32 i = 0; i < iidCount; i++) >+ PRUint32 i; >+ for(i = 0; i < iidCount; ++i) >+ { >+ interfaceArray[i] = nsnull; >+ } Use memset, compilers may optimize, and it's the right thing. >+ if(*(mPtr + i)) Here and below, I prefer mPtr[i] -- no ugly * and parens. >+ // Scope for our auto-marker; it just needs to keep tempGlobal >+ // alive long enough for WrapNative to do its work >+ AUTO_MARK_JSVAL(ccx, OBJECT_TO_JSVAL(tempGlobal)); How about an extra blank line here? >+ if(NS_FAILED(InitClasses(aJSContext, tempGlobal))) >+ return UnexpectedFailure(NS_ERROR_FAILURE); >+ >+ if(NS_FAILED(WrapNative(aJSContext, tempGlobal, aCOMObj, aIID, >+ getter_AddRefs(holder))) || !holder) >+ return UnexpectedFailure(NS_ERROR_FAILURE); >+ } I'll mark when dbradley or a delegate (you may want to mail him to get past bugzilla filtering) r+'s. /be

Comment on attachment 195322 [details] [diff] [review] Per the comments A fly by review comment: + for(PRUint32 i = 0; i < mCount; ++i) \ + { \ + if(*(mPtr + i)) \ + { \ + (*(mPtr+i))->MarkBeforeJSFinalize(cx); \ + (*(mPtr+i))->AutoMark(cx); \ How about a local for *(mPtr + i) here (or mPtr[i] rather) to make this a bit more CPU register friendly...

I agree with previous comments on the last patch. I'll add... - AutoMarkingNativeInterfaceArrayPtr is technically: AutoMarkingNativeInterfacePtrArrayPtr - I don't see why you have 'interfaceArray' *and* 'marker'. Why not just declare interfaceArray as an AutoMarkingNativeInterfacePtrArrayPtr and get rid of marker? You'd just need to add a ctor that defaults the non-ccx params and an operator=() - just like the DEFINE_AUTO_MARKING_PTR_TYPE code that you cloned this from. - You could add a defaulted param to the ctor of the class defined by DEFINE_AUTO_MARKING_ARRAY_PTR_TYPE that optionally does a memset of the passed in array of pointers when that object is constructed. This seems reasonable to me since it is that class and not the calling code that is worried about the intermediate state of those pointers.

> - I don't see why you have 'interfaceArray' *and* 'marker'. Because I didn't really want to null-check mPtr in marker. I could do that, I suppose (and ignore mLength if mPtr is null). > and an operator=() I'd need a SetLength() too. Or something... And that would involve making decisions about whether the marker should delete the array in its destructor or what. > You could add a defaulted param to the ctor Good idea. I'll do that.

>> - I don't see why you have 'interfaceArray' *and* 'marker'. >Because I didn't really want to null-check mPtr in marker. I could do that, I >suppose (and ignore mLength if mPtr is null). Yes, please. I really think you hsould make this act like the autoMarkingPtr stuff that is already there. >> and an operator=() >I'd need a SetLength() too. Or something... And that would involve making >decisions about whether the marker should delete the array in its destructor or >what. No. This is not an owning pointer. The user still needs to use delete[]. Look at the operator=() this is in the existing class. It just copies over the pointer (and now length too).

I decided to just move the array ownership completely into the marker class. If we ever need something more flexible, it should be pretty easy to do with a constructor arg, I think. Still asking for r=dbradley, but if jband wants to r=, that'd be great too. ;)

jband, if you think I should make this thing no-owning instead and null-check mPtr instead, just let me know, ok?

[mid-air collision - 'letting you know'] You're not going to get my approval :) As stated above, I think this should work like the other auto marking classes in xpconnect: - it should just be a pointer, not an allocator/destroyer. - it should have a simple ctor so that the thing can be created as it goes into scope but, perhaps, before you are ready to fully populate it. - it should have an operator= so that you can assign the contents of one to another. This way you can create it at the appropriate scope and then populate it using a temporary at the point in the code approriate that that. - users should be free to have any number od such auto marking pointers wrapping the same data without fear that one of them will whack the data. So, the 'clear' param to the ctor should default to false. At least, that's what I think.

Comment on attachment 195341 [details] [diff] [review] With the comments addressed I agree with jband, it'd be better to make this match existing "pointer" helpers, which are primitives -- they don't mix in ownership. If we need that in general, we can derive/mix-in. /be

Not sure whether the delete[] interfaceArray is still OK or whether I should add a .get()...

Comment on attachment 195352 [details] [diff] [review] So like so? r=dbradley Looks ok to me. Minor nit, your comment in the last chunk states it's keeping tempGlobal alive long enough for WrapNative, but you're actually keeping it across InitClasses and WrapNative. Maybe sometime these macros could turn into templates.

Comment on attachment 195352 [details] [diff] [review] So like so? Sorry, I wasn't working my review request queue properly! /be

Fixed trunk and branch.