NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(IMETextTxn) incorrectly used NS_INTERFACE_MAP_END_INHERITING(EditTxn)

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
Last year

People

(Reporter: timeless, Assigned: timeless)

Tracking

(Blocks 1 bug, {coverity})

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

So, trying to understand Coverity reports can be painful.
In order to explain what's going on here, i've marked each macro expansion with its own letter, the b218..b219 is the definition and expansion of NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION, and the line before it, a67, is the line in the code which used that macro.

The problem is that:

d538 defines foundInterface.
i205 sets it
then the else if from i206+a68 is skipped because it's an else, which takes us to a72, the next line is k576 which nulls out foundInterface.

Coverity complains that upon reaching k578, the effort taken in i205 was wasted because it was unconditionally cleared by k576.

/*a67 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(IMETextTxn) */
/*b218 #define NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(_class) */
/*b219  NS_INTERFACE_MAP_BEGIN(_class) */
/*c615 #define NS_INTERFACE_MAP_BEGIN(_implClass) */
/*c     NS_IMPL_QUERY_HEAD(_implClass) */
/*d533 #define NS_IMPL_QUERY_HEAD(_class) */
/*d534 */
NS_IMETHODIMP _class::QueryInterface(REFNSIID aIID, void** aInstancePtr)
/*d535 */
{
/*d536 */
  NS_ASSERTION(aInstancePtr,
/*d537 */
               "QueryInterface requires a non-NULL destination!");
/*d538 */
  nsISupports* foundInterface;
/*b220     NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(_class) */
/*e214 #define NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(_class) */
/*e215   NS_INTERFACE_MAP_ENTRY_CYCLE_COLLECTION(_class) */
/*f208 #define NS_INTERFACE_MAP_ENTRY_CYCLE_COLLECTION(_class) */
/*f209   NS_IMPL_QUERY_CYCLE_COLLECTION(_class) */
/*g197 #define NS_IMPL_QUERY_CYCLE_COLLECTION(_class) */
/*g198 */
  if ( aIID.Equals(NS_GET_IID(nsXPCOMCycleCollectionParticipant)) ) {
/*g199 */
    *aInstancePtr = & NS_CYCLE_COLLECTION_NAME(_class);
/*g200 */
     return NS_OK;
/*g201 */
  } else
/*e216   NS_INTERFACE_MAP_ENTRY_CYCLE_COLLECTION_ISUPPORTS(_class) */
/*h211 #define NS_INTERFACE_MAP_ENTRY_CYCLE_COLLECTION_ISUPPORTS(_class) */
/*h212   NS_IMPL_QUERY_CYCLE_COLLECTION_ISUPPORTS(_class) */
/*i203 #define NS_IMPL_QUERY_CYCLE_COLLECTION_ISUPPORTS(_class) */
/*i204 */
  if ( aIID.Equals(NS_GET_IID(nsCycleCollectionISupports)) )
/*i205 */
    foundInterface = NS_CYCLE_COLLECTION_CLASSNAME(_class)::Upcast(this);
/*i206 */
  else
/*a68 */
  if (aIID.Equals(IMETextTxn::GetCID())) {
/*a69 */
    *aInstancePtr = (void*)(IMETextTxn*)this;
/*a70 */
    NS_ADDREF_THIS();
/*a71 */
    return NS_OK;
/*a72 */
  }
/*a73 NS_INTERFACE_MAP_END_INHERITING(EditTxn) */
/*j625 #define NS_INTERFACE_MAP_END_INHERITING(_baseClass) */
/*j626   NS_IMPL_QUERY_TAIL_INHERITING(_baseClass) */
/*k575 #define NS_IMPL_QUERY_TAIL_INHERITING(_baseclass) */
/*k576 */
    foundInterface = 0;
/*k577 */
  nsresult status;
/*k578 */
  if ( !foundInterface )
/*k579 */
    status = _baseclass::QueryInterface(aIID, (void**)&foundInterface);
/*k580 */
  else
/*k581 */
    {
/*k582 */
      NS_ADDREF(foundInterface);
/*k583 */
      status = NS_OK;
/*k584 */
    }
/*k585 */
  *aInstancePtr = foundInterface;
/*k586 */
  return status;
/*k587 */
}
Posted patch add the missing else (obsolete) — Splinter Review
Attachment #388477 - Flags: review?(neil)
Attachment #388477 - Flags: review?(neil) → review+
Attachment #388477 - Attachment is obsolete: true
Attachment #388483 - Flags: review?(neil)
Attachment #388483 - Flags: review?(neil) → review+
Did you want to fix EditAggregateTxn as well?
Er, never mind... I guess I removed that bit before I landed the original patch, since it was unused.
Er, actually, I removed it when I landed bug 489851, which was after I posted the patch to the bug but before I landed it; I think I remember having a little fun merging that.
http://hg.mozilla.org/mozilla-central/rev/40bc10840e6d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.