The default bug view has changed. See this FAQ.

[FIX]Starting up with WAY_TOO_MUCH_GC dies in XPConnect

RESOLVED FIXED in mozilla1.8beta5

Status

()

Core
XPConnect
P1
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({fixed1.8})

Trunk
mozilla1.8beta5
fixed1.8
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
Blocks: 307312
Oh, you have to apply the patch in bug 305884 to get this far.
Created attachment 195108 [details] [diff] [review]
Proposed patch

I wish I could do this without the extra heap allocation, but I don't see
how... :(
Attachment #195108 - Flags: superreview?(brendan)
Attachment #195108 - Flags: review?(dbradley)
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.

Comment 5

12 years ago
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
Attachment #195108 - Flags: superreview?(brendan)
Attachment #195108 - Flags: review?(dbradley)
Created attachment 195322 [details] [diff] [review]
Per the comments
Attachment #195108 - Attachment is obsolete: true
Attachment #195322 - Flags: superreview?(brendan)
Attachment #195322 - Flags: review?(dbradley)
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...

Comment 11

12 years ago
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.

Comment 13

12 years ago
>> - 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). 
Created attachment 195341 [details] [diff] [review]
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.  ;)
Attachment #195322 - Attachment is obsolete: true
Attachment #195341 - Flags: superreview?(brendan)
Attachment #195341 - Flags: review?(dbradley)
jband, if you think I should make this thing no-owning instead and null-check
mPtr instead, just let me know, ok?

Comment 16

12 years ago
[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
Created attachment 195352 [details] [diff] [review]
So like so?

Not sure whether the delete[] interfaceArray is still OK or whether I should
add a .get()...
Attachment #195352 - Flags: superreview?(brendan)
Attachment #195352 - Flags: review?(dbradley)
Attachment #195322 - Flags: superreview?(brendan)
Attachment #195322 - Flags: review?(dbradley)
Attachment #195341 - Attachment is obsolete: true
Attachment #195341 - Flags: superreview?(brendan)
Attachment #195341 - Flags: review?(dbradley)

Comment 19

12 years ago
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.
Attachment #195352 - Flags: review?(dbradley) → review+
Assignee: dbradley → bzbarsky
Priority: -- → P1
Summary: Starting up with WAY_TOO_MUCH_GC dies in XPConnect → [FIX]Starting up with WAY_TOO_MUCH_GC dies in XPConnect
Target Milestone: --- → mozilla1.8beta5
Comment on attachment 195352 [details] [diff] [review]
So like so?

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

/be
Attachment #195352 - Flags: superreview?(brendan)
Attachment #195352 - Flags: superreview+
Attachment #195352 - Flags: approval1.8b5+

Updated

12 years ago
Flags: blocking1.8b5+
Created attachment 196173 [details] [diff] [review]
Updated to review comments
Fixed trunk and branch.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.