307313 - [FIX]Starting up with WAY_TOO_MUCH_GC dies in XPConnect

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

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

Comment 3

[Brendan Eich [:brendan]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 5

[John Bandhauer]

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 :)

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 7

[Brendan Eich [:brendan]]

(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 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Per the comments

Comment 9

[Brendan Eich [:brendan]]

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 10

[Johnny Stenback (:jst)]

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...

Comment 11

[John Bandhauer]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> - 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.

Comment 13

[John Bandhauer]

>> - 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). 

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: With the comments addressed

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.  ;)

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[John Bandhauer]

[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 17

[Brendan Eich [:brendan]]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: So like so?

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

Comment 19

[David Bradley]

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 20

[Brendan Eich [:brendan]]

Comment on attachment 195352 [details] [diff] [review]
So like so?

Sorry, I wasn't working my review request queue properly!

/be

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

Comment 22

[Boris Zbarsky [:bzbarsky]]

Fixed trunk and branch.