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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

The assertion is bogus with a particular pattern of segment allocations and pipe reads; see comments in the patch for details.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 279414 [details] [diff] [review]
Patch

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

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 279414 [details] [diff] [review]
Patch

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

Comment 4

[Jeff Walden [:Waldo]]

Attachment: With unit test and fixups

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.

Comment 5

[Jeff Walden [:Waldo]]

bsmedberg, sayrer: thoughts on a useful location for TestHarness.h, and on the Makefile changes if you feel like it?

Comment 6

[Jeff Walden [:Waldo]]

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 7

[Jeff Walden [:Waldo]]

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 8

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Final patch

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281408 [details] [diff] [review]
Final patch

a=bzbarsky

Comment 11

[Jeff Walden [:Waldo]]

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.