dwitte@gmail.com Reporter
	Description • 15 years ago

There are certain classes (nsCOMPtr and company, string classes, etc) that we generally don't want to be instantiated - by the compiler or the user - as temporaries (that is, without a named variable). In the case of nsCOMPtrs, this results in extra AddRef/Release operations; in the case of string classes, memory allocations. In many cases it also highlights poor coding practice.

This was spun off from bug 172937, which contains some discussion on this. I have a patch to add an analysis pass, and fix up the instances throughout the tree, which I'll post soon.

	dwitte@gmail.com Reporter
	Comment 1 • 15 years ago

here's the analysis pass - also adds annotation to nsCOMPtr.

	dwitte@gmail.com Reporter
	Comment 2 • 15 years ago

Fixes 61 warnings throughout the tree, which gets us to zero. Once this lands, I'll flip the analysis pass to error instead (assuming the tinderboxen don't warn).

	(dormant account)
	Comment 3 • 15 years ago

Comment on attachment 387275 [details] [diff] [review]
temporary.js analysis pass, v1

Instead of reimplementing recursion, i'd rather have an iterator doing recursion.

ie a better bug 496024

	Benjamin Smedberg
	Comment 4 • 15 years ago

Comment on attachment 387276 [details] [diff] [review]
tree patch, v1

>diff --git a/editor/libeditor/base/nsEditor.cpp b/editor/libeditor/base/nsEditor.cpp

>-nsCOMPtr<nsIDOMNode>
>+already_AddRefed<nsIDOMNode>


>+  nsIDOMNode *result = resultNode;
>+  NS_IF_ADDREF(result);
>+  return result;

This should be simply

  return resultNode.forget();


>+already_AddRefed<nsIDOMNode>
> nsEditor::GetLeftmostChild(nsIDOMNode *aCurrentNode,
>                            PRBool bNoBlockCrossing)

ditto

>diff --git a/editor/libeditor/html/nsHTMLDataTransfer.cpp b/editor/libeditor/html/nsHTMLDataTransfer.cpp

>-static nsCOMPtr<nsIDOMNode> GetListParent(nsIDOMNode* aNode)
>+static already_AddRefed<nsIDOMNode> GetListParent(nsIDOMNode* aNode)

ditto

>-static nsCOMPtr<nsIDOMNode> GetTableParent(nsIDOMNode* aNode)
>+static already_AddRefed<nsIDOMNode> GetTableParent(nsIDOMNode* aNode)

ditto

>diff --git a/editor/libeditor/html/nsHTMLEditRules.cpp b/editor/libeditor/html/nsHTMLEditRules.cpp

>-nsCOMPtr<nsIDOMNode> 
>+already_AddRefed<nsIDOMNode> 
> nsHTMLEditRules::GetHighestInlineParent(nsIDOMNode* aNode)

ditto
         
>-nsCOMPtr<nsIDOMNode> 
>+already_AddRefed<nsIDOMNode> 
> nsHTMLEditRules::IsInListItem(nsIDOMNode *aNode)

ditto

	dwitte@gmail.com Reporter
	Comment 5 • 15 years ago

now with test!

	(dormant account)
	Comment 6 • 15 years ago

Comment on attachment 387342 [details] [diff] [review]
temporary.js analysis pass, v2

as discussed on irc

>+function process_function(decl, statements)
>+{
>+  this._function = decl;

I don't think _function is used for anything

>+  for each (let v in iterate_vars(statements)) {
>+    // check if |v| is a constructor being assigned to an unnamed variable
>+    if (v.isConstructor && (!v.fieldOf || !v.fieldOf.name) && hasAttribute(v.memberOf, 'NS_nontemporary')) {
>+      warning("constructor '" + v.name + "' called to instantiate class '" + v.memberOf.name + "' as an unnamed temporary variable", v._loc);

Pretty sure v._loc is undefined., don't pass it
>+    }
>+  }
>+}

r+ with those fixed.

	dwitte@gmail.com Reporter
	Comment 7 • 15 years ago

Comment on attachment 387342 [details] [diff] [review]
temporary.js analysis pass, v2

bsmedberg, this touches nsCOMPtr - can i get your sr? new patch for the tree upcoming...

	dwitte@gmail.com Reporter
	Comment 8 • 15 years ago

now with .forget(), and some cases the old script missed.

	Benjamin Smedberg
	Comment 9 • 15 years ago

Comment on attachment 387342 [details] [diff] [review]
temporary.js analysis pass, v2

I'll mutter under my breath about the verboseness of NS_NONTEMPORARY_CLASS, but I can't think of a good alternative at this point.

	David Baron :dbaron:
	Comment 10 • 15 years ago

Comment on attachment 387364 [details] [diff] [review]
tree patch, v2

I think the thing that's bad in these cases is that the nsCOMPtrs are *implicit*, not that they're *temporary*.

I've intentionally used nsCOMPtr<nsIFoo>(do_QueryInterface(bar)) as the explicit argument to a function call (i.e., a temporary), and I think that's ok if it fits.


>diff --git a/content/base/src/nsLoadListenerProxy.cpp b/content/base/src/nsLoadListenerProxy.cpp

>-nsLoadListenerProxy::nsLoadListenerProxy(nsWeakPtr aParent) : mParent(aParent)
>+nsLoadListenerProxy::nsLoadListenerProxy(const nsWeakPtr& aParent) : mParent(aParent)

>diff --git a/content/base/src/nsLoadListenerProxy.h b/content/base/src/nsLoadListenerProxy.h

>-  nsLoadListenerProxy(nsWeakPtr aParent);
>+  nsLoadListenerProxy(const nsWeakPtr& aParent);

I think this should just be |nsIWeakReference* aParent|.

>diff --git a/editor/libeditor/base/IMETextTxn.cpp b/editor/libeditor/base/IMETextTxn.cpp

> NS_IMETHODIMP IMETextTxn::Init(nsIDOMCharacterData     *aElement,
>                                PRUint32                 aOffset,
>                                PRUint32                 aReplaceLength,
>                                nsIPrivateTextRangeList *aTextRangeList,
>                                const nsAString         &aStringToInsert,
>-                               nsWeakPtr                aSelConWeak)
>+                               const nsWeakPtr         &aSelConWeak)

>diff --git a/editor/libeditor/base/IMETextTxn.h b/editor/libeditor/base/IMETextTxn.h

>   NS_IMETHOD Init(nsIDOMCharacterData *aElement,
>                   PRUint32 aOffset,
>                   PRUint32 aReplaceLength,
>                   nsIPrivateTextRangeList* aTextRangeList,
>                   const nsAString& aString,
>-                  nsWeakPtr aSelCon);
>+                  const nsWeakPtr& aSelCon);

Same here: |nsIWeakReference* aSelCon|.

>-nsCOMPtr<nsIDOMNode> 
>+already_AddRefed<nsIDOMNode> 
> nsHTMLEditRules::IsInListItem(nsIDOMNode *aNode)
> {
>   if (!aNode) return nsnull;  
>-  if (nsHTMLEditUtils::IsListItem(aNode)) return aNode;
>+  if (nsHTMLEditUtils::IsListItem(aNode))
>+  {
>+    NS_ADDREF(aNode);
>+    return aNode;
>+  }

We might want to think about having a helper for this (not necessarily in this patch, though).  I think we may have a bug on the idea.

>@@ -848,21 +848,23 @@ nsHTMLEditor::GetBlockSectionsForRange(n
>-                blockParentOfLastStartNode = do_QueryInterface(GetBlockNodeParent(lastStartNode));
>+                nsCOMPtr<nsIDOMNode> node = GetBlockNodeParent(lastStartNode);
>+                blockParentOfLastStartNode = do_QueryInterface(node);

See, I think places like this are a fine place for explicit temporaries:

blockParentOfLastStartNode = do_QueryInterface(
  nsCOMPtr<nsIDOMNode>(GetBlockNodeParent(lastStartNode)));


>                 if (blockParentOfLastStartNode)
>                 {
>                   nsCOMPtr<nsIDOMElement> blockParentOfLeftNode;
>-                  blockParentOfLeftNode = do_QueryInterface(GetBlockNodeParent(leftNode));
>+                  node = GetBlockNodeParent(leftNode);
>+                  blockParentOfLeftNode = do_QueryInterface(node);

etc.

>-nsCOMPtr<nsIDOMNode> nsHTMLEditor::FindUserSelectAllNode(nsIDOMNode *aNode)
>-{
>-  nsCOMPtr<nsIDOMNode> resultNode;  // starts out empty
>+already_AddRefed<nsIDOMNode> nsHTMLEditor::FindUserSelectAllNode(nsIDOMNode *aNode)
>+{
>+  nsCOMPtr<nsIDOMNode> resultNode = nsnull;  // starts out empty

No need to add the " = nsnull".

>@@ -3798,32 +3799,32 @@ nsCOMPtr<nsIDOMNode> nsHTMLEditor::FindU
>     if (node != root)
>     {
>       nsCOMPtr<nsIDOMNode> tmp;
>       node->GetParentNode(getter_AddRefs(tmp));
>       node = tmp;
>     }
>     else
>     {
>-      node = nsnull;
>+      return nsnull;

The old code was returning resultNode, which need not have been null at this point.  I think it's ok to change this to |break;| or leave it as is, though.

>     }
>   } 
> 
>-  return resultNode;
>+  return resultNode.forget();
> }

>diff --git a/extensions/spellcheck/src/mozInlineSpellWordUtil.cpp b/extensions/spellcheck/src/mozInlineSpellWordUtil.cpp

>-mozInlineSpellWordUtil::Init(nsWeakPtr aWeakEditor)
>+mozInlineSpellWordUtil::Init(const nsWeakPtr& aWeakEditor)

>diff --git a/extensions/spellcheck/src/mozInlineSpellWordUtil.h b/extensions/spellcheck/src/mozInlineSpellWordUtil.h

>-  nsresult Init(nsWeakPtr aWeakEditor);
>+  nsresult Init(const nsWeakPtr& aWeakEditor);

nsIWeakReference* aWeakEditor

>diff --git a/layout/base/nsDocumentViewer.cpp b/layout/base/nsDocumentViewer.cpp

I'm up to this point in the patch.

	David Baron :dbaron:
	Comment 11 • 15 years ago

Comment on attachment 387364 [details] [diff] [review]
tree patch, v2

>diff --git a/layout/base/nsDocumentViewer.cpp b/layout/base/nsDocumentViewer.cpp

I think you don't need any changes in this file...

>diff --git a/layout/base/nsIPresShell.h b/layout/base/nsIPresShell.h

>-  void SetForwardingContainer(nsWeakPtr aContainer)
>+  void SetForwardingContainer(const nsWeakPtr& aContainer)

Given that you make this |nsIWeakReference* aContainer|.

And then you don't need this either:
>+
>+  void ResetForwardingContainer()
>+  {
>+    mForwardingContainer = nsnull;
>+  }

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp

I don't think you need any changes in this file (this was another perfectly good use of explicit temporaries).  Probably the best case for them, given that to avoid them you turned a two-parameter template into a three-parameter template.

(This temporary nsCOMPtr will be going away very shortly anyway with bug 479220, so I'd rather you not touch it to avoid the merge conflicts as well.)

>diff --git a/parser/htmlparser/src/nsExpatDriver.cpp b/parser/htmlparser/src/nsExpatDriver.cpp

>-  NS_ASSERTION(mSink == nsCOMPtr<nsIExpatSink>(do_QueryInterface(mOriginalSink)),
>+#ifdef DEBUG
>+  nsCOMPtr<nsIExpatSink> sink(do_QueryInterface(mOriginalSink));
>+  NS_ASSERTION(mSink == sink,
>                "In nsExpatDriver::OpenInputStreamFromExternalDTD: "
>                "mOriginalSink not the same object as mSink?");
>+#endif

And assertions are another good use case for explicit temporary nsCOMPtrs.  (But if you somehow convince me to go along with this change, you should at least brace and indent inside the #ifdef so that there's a block scope to keep the ifdef-ed variables from leaking out.)

>diff --git a/toolkit/components/places/src/nsMaybeWeakPtr.h b/toolkit/components/places/src/nsMaybeWeakPtr.h

This file makes my head hurt, but I think your changes are ok.

>diff --git a/xpfe/appshell/src/nsWebShellWindow.cpp b/xpfe/appshell/src/nsWebShellWindow.cpp

>+  nsIDOMDocument* domDoc = nsnull;
>+  CallQueryInterface(doc, &domDoc);
>   return domDoc;

It might be nice to have a helper for this at some point too (though probably not in this patch).


sr=dbaron with those changes (comment 10 through here)

	David Baron :dbaron:
	Comment 12 • 15 years ago

Another possible followup:  I think the name |nsWeakPtr| is confusing (I think you were confused by it when writing this patch) so we should get rid of it and just type out nsCOMPtr<nsIWeakReference>.

	dwitte@gmail.com Reporter
	Comment 13 • 15 years ago

(In reply to comment #10)
> I think the thing that's bad in these cases is that the nsCOMPtrs are
> *implicit*, not that they're *temporary*.

Okay - that makes sense. Perhaps what the analysis should be looking for, instead, are function arguments or return types where the class is passed by value. This would still catch all cases of "abuse" that I patched, while allowing temporaries.

It would also work well for the string classes, where temporaries are fine (I believe) but passing by value isn't.

If you think this is a better idea, I'll update the analysis pass to do that. The annotation could be something like NS_PASS_BY_REFERENCE (though that name erroneously suggests passing by pointer isn't okay). I can't immediately think of a use for the nontemporary one as well.

	David Baron :dbaron:
	Comment 14 • 15 years ago

Note that there are classes we do want to prevent being instantiated as temporaries; see bug 519171 for examples.  However, I don't think nsCOMPtr fits in that set; we want to prevent it from being instantiated implicitly.

So I think the analysis here is useful for other classes.

	Benjamin Smedberg
	Comment 15 • 15 years ago

Hrm, I really think nsCOMPtr should be disallowed, even if it prevents a few cases that are otherwise ok. But in any case, can we get the analysis part of this landed?

	dwitte@gmail.com Reporter
	Comment 16 • 15 years ago

Yep, I'll get back to this when I get a break in the ctypes stuff.

	dwitte@gmail.com Reporter
	Comment 17 • 14 years ago

Reassigning to nobody. If anyone wants to work on this, feel free!