504087 - NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(IMETextTxn) incorrectly used NS_INTERFACE_MAP_END_INHERITING(EditTxn)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[timeless]

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 */
}

Comment 1

[timeless]

Attachment: add the missing else

Comment 2

[timeless]

Attachment: and the same for EditTxn

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you want to fix EditAggregateTxn as well?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, never mind... I guess I removed that bit before I landed the original patch, since it was unused.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 6

[timeless]

http://hg.mozilla.org/mozilla-central/rev/40bc10840e6d