NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(IMETextTxn) incorrectly used NS_INTERFACE_MAP_END_INHERITING(EditTxn)

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
4 months ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
x86
Mac OS X
coverity
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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 */
}
(Assignee)

Comment 1

9 years ago
Created attachment 388477 [details] [diff] [review]
add the missing else
Attachment #388477 - Flags: review?(neil)

Updated

9 years ago
Attachment #388477 - Flags: review?(neil) → review+
(Assignee)

Comment 2

9 years ago
Created attachment 388483 [details] [diff] [review]
and the same for EditTxn
Attachment #388477 - Attachment is obsolete: true
Attachment #388483 - Flags: review?(neil)

Updated

9 years ago
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.
(Assignee)

Comment 6

9 years ago
http://hg.mozilla.org/mozilla-central/rev/40bc10840e6d
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Blocks: 1230156
You need to log in before you can comment on or make changes to this bug.