387584 - ASSERTION: should not have buffer of zero size [@nsTArray_base::EnsureCapacity]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Workaround

Somehow we're hitting this assertion:

  NS_ASSERTION(mHdr->mCapacity > 0, "should not have buffer of zero size");

It seems that mHdr is pointing to a valid Header with mCapacity=0, but the address of that header isn't the same as sEmptyHdr like it should be:

  if (mHdr == &sEmptyHdr) {
    // NS_Alloc new data
    Header *header = static_cast<Header*>
      (NS_Alloc(sizeof(Header) + capacity * elemSize));

Although gdb indicates that it *is* the same address. I think that there are multiple sEmptyHdr symbols getting linked together somehow, although adding NS_HIDDEN to its declaration did not resolve the issue.

Not sure what's really going on, but I'll throw this patch up in case someone needs it.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, right, we only see this on debug builds. Go figure.

Comment 2

[Benjamin Smedberg]

This is probably caused by the inline references to sEmptyHdr in nsTArray.h which can refer to a different symbol than those in nsTArray.cpp, if you pass a nsTArray across DLL boundaries (not recommended!)

The easy fix is probably to always un-inline the nsTArray constructor here: http://mxr.mozilla.org/mozilla/source/xpcom/glue/nsTArray.h#88

Comment 3

[Benjamin Smedberg]

Comment on attachment 271713 [details] [diff] [review]
Workaround

But this is a poor solution because it makes sEmptyHdr rather worthless.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

Ok, this un-inlines the constructor.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 277304 [details] [diff] [review]
Patch, v2

Doug, can you look at this since bsmedberg is off for a while? If not let me know and I'll retarget.

Comment 6

[Doug Turner (:dougt)]

dbaron would be another good reviewer for this.  you might want to try him.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 277304 [details] [diff] [review]
Patch, v2

dbaron, what do you think?

Comment 8

[Benjamin Smedberg]

Comment on attachment 277304 [details] [diff] [review]
Patch, v2

I think this is ok. Please check the perf numbers carefully after you land this.

Comment 9

[Sergey Yanovich]

>Index: xpcom/glue/nsTArray.h

>     nsTArray_base();
>+#ifdef NS_BUILD_REFCNT_LOGGING
>     ~nsTArray_base();  
>-#endif // NS_BUILD_REFCNT_LOGGING
>+#endif
> 

nsTArray.h is a glue header, and it is desirably be in sync with the glue binary libraries.

Leaving NS_BUILD_REFCNT_LOGGING macro here will leave a duplicate of this - bug 394214 open, except that the linker will complain about missing destructor. It may make sense to put MOZ_COUNT_* in in-line constructor/destructor.

A single empty in-line destructor call should not add overhead, since it should be optimized out by compiler.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #9)

The issue this bug is trying to fix is the inline constructor.

And anyway mixing debug components with optimized xpcom has crashed our product lots of times. You should probably avoid it in general.

Comment 12

[Sergey Yanovich]

(In reply to comment #11)
> And anyway mixing debug components with optimized xpcom has crashed our product
> lots of times. You should probably avoid it in general.

I just cannot buy this. Following this logic I cannot build a debug mozilla without debug libc, which is obviously wrong.

Each time you have a crash is bug which just needs to be fixed. And I believe that  this and bug 394214 are exactly the same issue. We are probably looking at it from different sides.

Comment 13

[timeless]

please file bugs about mixing crashes. but bug 392526 should be fixed first, so if you're interested in working on it, you could post a patch there.

Comment 14

[Sergey Yanovich]

Attachment: an alternative patch (all inline), v3

This patch achieves similar results as the previous one. In addition, it fixes bug 394214.

Performance implications in non-debug builds:
* one empty inline destructor call per instance. Taking into account compiler optimization, this should be nothing at all.

Comment 15

[Sergey Yanovich]

Attachment: an alternative patch (all outline), v3

This patch achieves similar results as the v2. In addition, it fixes bug 394214.

Performance implications in non-debug builds:
* 2 real function call per instance.
* each module that uses nsTArray<T> will grow a few byte.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 278940 [details] [diff] [review]
an alternative patch (all outline), v3

Yeah, I'm fine with this. I'll check it in.

FWIW I wasn't saying that debug components + optimized XPCOM *shouldn't* work, just that it doesn't always.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed on trunk.