240589 - hasMoreElements is off by one for simple enumerators returned by the component registrar

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Christopher Lenz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040415

The nsISimpleEnumerator instances returned by
nsIComponentRegistrar::enumeratorsContractIDs() and
nsIComponentRegistrar::enumerateCIDs() don't report when the end of the
enumeration has been reached. In particular
nsISimpleEnumerator::hasMoreElements() returns TRUE at the end of the
enumeration, but nsISimpleEnumerator::getNext() fails.

I have traced down the problem to the method
PLDHashTableEnumeratorImpl::HasMoreElements() in
xpcom/components/nsComponentManager.cpp line 591:

  NS_IMETHODIMP
  PLDHashTableEnumeratorImpl::HasMoreElements(PRBool *_retval)
  {
      if (!mCount || (mCurrent == mCount))
          *_retval = PR_FALSE;
      else
          *_retval = PR_TRUE;
  
      return NS_OK;
  }

The problem here is that this function implements the same logic as
PLDHashTableEnumeratorImpl::IsDone(), even though the semantics of the method
are different:

- IsDone() should return a failure if the current offset is *past* the last
index into the underlying array, meaning that after IsDone() failed, a call to
CurrentItem() will also fail. See also bug 106617.

- HasMoreElements() should return FALSE if the current offset is *at* the last
index into the underlying array, so that calling GetNext() after
HasMoreElements() returned FALSE will fail.

To fix this problem, the function implementation needs to be changed:

  NS_IMETHODIMP
  PLDHashTableEnumeratorImpl::HasMoreElements(PRBool *_retval)
  {
      if (!mCount || (mCurrent == mCount - 1))
          *_retval = PR_FALSE;
      else
          *_retval = PR_TRUE;
  
      return NS_OK;
  }

I will attach a minimalistic C++ test application that demonstrates the failure.

Even though it looks like this code isn't actually being used by a real
Mozilla-based product, I think this functionality is very basic to XPCOM and
would be important (and IMHO simple) to fix.

Reproducible: Always
Steps to Reproduce:
1. Compile and run the attached program
Actual Results:  
GetNext() returns a failure even though HasMoreElements() returned TRUE.
The test application prints the error message "Failed to get next contract ID
from enum!" to stdout.

Expected Results:  
HasMoreElements() returned FALSE. No output from the test application.

Comment 1

[Christopher Lenz]

Typo: Instead of bug 106617, see bug 106671

Comment 2

[Christopher Lenz]

Attachment: Simple command-line test-application to demonstrate the problem

This is a simple command-line application that shows the failure.

Comment 3

[Christopher Lenz]

Attachment: Patch that fixes the described problem

Comment 4

[Alec Flett]

Your logic sounds correct to me. Confirming the bug so I can review the patch.

Comment 5

[Alec Flett]

Comment on attachment 146176 [details] [diff] [review]
Patch that fixes the described problem

r/sr=alecf

Comment 6

[timeless]

do you need someone to check this in?

Comment 7

[timeless]

Comment on attachment 146176 [details] [diff] [review]
Patch that fixes the described problem

mozilla/xpcom/components/nsComponentManager.cpp 	1.259