Closed Bug 387584 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Workaround (obsolete) — Splinter Review
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.
Attachment #271713 - Flags: review?(benjamin)
Oh, right, we only see this on debug builds. Go figure.
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 on attachment 271713 [details] [diff] [review]
Workaround

But this is a poor solution because it makes sEmptyHdr rather worthless.
Attachment #271713 - Flags: review?(benjamin) → review-
Attached patch Patch, v2 (obsolete) — Splinter Review
Ok, this un-inlines the constructor.
Assignee: nobody → bent.mozilla
Attachment #271713 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #277304 - Flags: review?
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.
Attachment #277304 - Flags: review? → review?(dougt)
dbaron would be another good reviewer for this.  you might want to try him.
Comment on attachment 277304 [details] [diff] [review]
Patch, v2

dbaron, what do you think?
Attachment #277304 - Flags: review?(dougt) → review?(dbaron)
Attachment #277304 - Flags: review?(dbaron) → review?(benjamin)
Comment on attachment 277304 [details] [diff] [review]
Patch, v2

I think this is ok. Please check the perf numbers carefully after you land this.
Attachment #277304 - Flags: review?(benjamin)
Attachment #277304 - Flags: review+
Attachment #277304 - Flags: approval1.9+
>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.
Duplicate of this bug: 394214
(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.
(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.
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.
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.
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.
Attachment #278940 - Flags: review+
Attachment #278940 - Flags: approval1.9+
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.
Attachment #277304 - Attachment is obsolete: true
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.