Closed Bug 394692 Opened 16 years ago Closed 16 years ago

ASSERTION: read cursor is bad: 'mReadCursor != mWriteCursor', file xpcom/io/nsPipe3.cpp, line 539


(Core :: XPCOM, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)



(Keywords: assertion)


(2 files, 1 obsolete file)

This seems to be the only assertion remaining on "bm-xserve11 Depend Debug + Leak Test", preventing us from making assertions fatal there.  I'm going to try to reproduce this locally and fix the bug.
Attached patch Patch (obsolete) — Splinter Review
The assertion is bogus with a particular pattern of segment allocations and pipe reads; see comments in the patch for details.
Attachment #279414 - Flags: review?(benjamin)
Comment on attachment 279414 [details] [diff] [review]

>+                      mBuffer.GetSegmentSize() == bytesWritten &&

On further thought this isn't valid (writing 5 bytes, then 1019, for a 1024-byte segment size could still trigger), so assume it's not there.  I added this after the other two conditions in an attempt to strengthen the assertion, but on further thought I doubt it's possible to do so.
Attachment #279414 - Flags: review?(benjamin) → review?(cbiesinger)
Comment on attachment 279414 [details] [diff] [review]

I think you shouldn't mention a "current segment", because there are two of them. I suggest "current write segment" or something

With that and the change from comment 2 r=biesi

a unit test would be nice :)
Attachment #279414 - Flags: review?(cbiesinger) → review+
Be assertive and demand a test!  If reviewers simply didn't grant review without tests in the patch, we'd have a lot more tests.  You can be sure I try to do that when I review things (problem being I don't review enough stuff yet).

I considered reviving TestPipes.cpp, but it asserts when it runs, the code's crufty and #if 0'd and worse, and I don't really want to put the time into fixing it in this bug (and I'm sure you wouldn't want to review it!).

TestHarness.h is meant to be reusable by any XPCOM-using test; where's a good location to put it?  The current location works for this patch, but it doesn't work long-term.  Let's make this a followup so we don't hold off on fixing the assertion (and making asserts fatal on that Mac box!) for too long.
Attachment #279414 - Attachment is obsolete: true
Attachment #281153 - Flags: review?(cbiesinger)
bsmedberg, sayrer: thoughts on a useful location for TestHarness.h, and on the Makefile changes if you feel like it?
Comment on attachment 281153 [details] [diff] [review]
With unit test and fixups

>Index: xpcom/tests/TestPipe.cpp

>+    ~BackwardsAllocator() { }

I need a |delete [] mMemory;| here, of course.  :-)
Comment on attachment 281153 [details] [diff] [review]
With unit test and fixups

>Index: xpcom/tests/TestPipe.cpp

>+  rv = NS_NewPipe2(getter_AddRefs(input),
>+                   getter_AddRefs(output),
>+                   PR_FALSE,
>+                   PR_FALSE,
>+                   SEGMENT_SIZE, SEGMENT_COUNT, allocator); 

Er, and with this:

  if (NS_FAILED(rv))
    printf("FAIL NS_NewPipe2 failed: %x\n", rv);
    return rv;

C++ and XPCOM are hard, let's go shopping.
Comment on attachment 281153 [details] [diff] [review]
With unit test and fixups

+  if (size != mSize)
+    return NULL;

add a NS_WARNING here? that shouldn't be reached really

+  return NULL;

here too

(or NS_ERROR even?)

+void* BackwardsAllocator::Alloc(size_t size)

need NS_IMETHODIMP / NS_IMETHODIMP_(void*) for your implementations
Attachment #281153 - Flags: review?(cbiesinger) → review+
Attached patch Final patchSplinter Review
This fixes the sole assertion (last I heard, I haven't checked in a couple weeks) preventing assertions from being fatal on the Mac debug tinderbox, which is useful in ensuring assertions don't occur in basic Firefox operation.  Once this gets in I'm going to be doing some pestering to make those assertions fatal ASAP; fatal assertions have been way too long in coming.
Attachment #281408 - Flags: review+
Attachment #281408 - Flags: approval1.9?
Comment on attachment 281408 [details] [diff] [review]
Final patch

Attachment #281408 - Flags: approval1.9? → approval1.9+
Fix committed, after much pain noticing 1) that NS_NewPipe2 isn't exported, so it had to be reimplemented in the test file, 2) that the default args I preserved in the reimplementation broke some stupid compilers which want default args only in the declaration, not the definition, and 3) that I changed the calling convention on BackwardsAllocator::Init in the final patch posted here when I shouldn't have done so, breaking Windows.
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.