Closed Bug 502775 Opened 10 years ago Closed 4 years ago

prevent temporary instantiation of annotated classes

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1201190

People

(Reporter: dwitte, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch temporary.js analysis pass, v1 (obsolete) — Splinter Review
here's the analysis pass - also adds annotation to nsCOMPtr.
Attachment #387275 - Flags: review?(tglek)
Attached patch tree patch, v1 (obsolete) — Splinter Review
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).
Attachment #387276 - Flags: superreview?
Attachment #387276 - Flags: review?(bsmedberg)
Attachment #387276 - Flags: superreview? → superreview?(dbaron)
Attachment #387276 - Flags: review?(bsmedberg) → review?(benjamin)
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
Attachment #387275 - Flags: review?(tglek) → review-
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
Attachment #387276 - Flags: review?(benjamin) → review-
Attachment #387276 - Flags: superreview?(dbaron)
Depends on: 502970
now with test!
Attachment #387275 - Attachment is obsolete: true
Attachment #387342 - Flags: review?(tglek)
Attachment #387342 - Flags: review?(tglek) → review+
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.
Attachment #387342 - Flags: superreview?(benjamin)
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...
Attached patch tree patch, v2Splinter Review
now with .forget(), and some cases the old script missed.
Attachment #387276 - Attachment is obsolete: true
Attachment #387364 - Flags: review?(benjamin)
Attachment #387364 - Flags: review?(benjamin) → review+
Attachment #387364 - Flags: superreview?(dbaron)
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.
Attachment #387342 - Flags: superreview?(benjamin) → superreview+
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.
Attachment #387364 - Flags: superreview?(dbaron) → superreview+
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)
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>.
(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.
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.
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?
Yep, I'll get back to this when I get a break in the ctypes stuff.
Reassigning to nobody. If anyone wants to work on this, feel free!
Assignee: dwitte → nobody
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1201190
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.