240480 - nsCategoryManager::DeleteCategoryEntry misinterprets its aPersist argument

Status: NEW

(Core :: XPCOM, defect)

Description

[Darin Fisher]

nsCategoryManager::DeleteCategoryEntry misinterprets its aPersist argument.  The
documentation for nsICategoryManager claims that DeleteCategoryEntry takes an
"aPersist" with the comment:

  "Delete persistent data from registry, if present?"

I think that means that if I pass true, then the deletion will be remembered
across application sessions.  However, nsCategoryManager::DeleteCategoryEntry
calls the variable aDontPersist.  It passes this to CategoryNode::DeleteLeaf,
which also calls the parameter aDontPersist.  The code for DeleteLeaf appears to
interpret aDontPersist literally, which means that we are not implementing
nsICategoryManager::DeleteCategoryEntry correctly.

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #0)
> nsCategoryManager::DeleteCategoryEntry misinterprets its aPersist argument. 
> The
> documentation for nsICategoryManager claims that DeleteCategoryEntry takes an
> "aPersist" with the comment:
> 
>   "Delete persistent data from registry, if present?"
> 
> I think that means that if I pass true, then the deletion will be remembered
> across application sessions.
That's how I interpreted it too.

But I think the code in CategoryNode::DeleteLeaf is correct.

CategoryNode::WritePersistentEntries writes the CategoryLeaf to the registry iff its ::pValue is != nsnull.

If you call nsICategoryManager::DeleteCateogoryEntry with persist=PR_TRUE [the variable name from the .idl] , the leaf is removed from the category node's mTable and therefore will not be enumerated by WritePersistentEntries, and so not persisted to the registry. IOW, the entry is deleted from the registry too; the deletion is persistent.

If you call DeleteCategoryEntry with persist=PR_FALSE, only the leaf's ::nonpValue is set to nsnull; the pValue is kept and the leaf is kept in the node's mTable. So in this case, WritePersistenEntries enumerates this leaf, and writes its ::pValue to the registry. IOW, the entry is present in the registry; the deletion was not persistent.

So the only thing that seems wrong is that the param is called aDontPersist in nsCategoryManager.cpp.