Closed
Bug 502775
Opened 15 years ago
Closed 9 years ago
prevent temporary instantiation of annotated classes
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 1201190
People
(Reporter: dwitte, Unassigned)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
taras.mozilla
:
review+
benjamin
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•15 years ago
|
||
here's the analysis pass - also adds annotation to nsCOMPtr.
Attachment #387275 -
Flags: review?(tglek)
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).
Attachment #387276 -
Flags: superreview?
Attachment #387276 -
Flags: review?(bsmedberg)
Reporter | ||
Updated•15 years ago
|
Attachment #387276 -
Flags: superreview? → superreview?(dbaron)
Reporter | ||
Updated•15 years ago
|
Attachment #387276 -
Flags: review?(bsmedberg) → review?(benjamin)
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
Attachment #387275 -
Flags: review?(tglek) → review-
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
Attachment #387276 -
Flags: review?(benjamin) → review-
Reporter | ||
Updated•15 years ago
|
Attachment #387276 -
Flags: superreview?(dbaron)
Reporter | ||
Comment 5•15 years ago
|
||
now with test!
Attachment #387275 -
Attachment is obsolete: true
Attachment #387342 -
Flags: review?(tglek)
Updated•15 years ago
|
Attachment #387342 -
Flags: review?(tglek) → review+
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.
Reporter | ||
Updated•15 years ago
|
Attachment #387342 -
Flags: superreview?(benjamin)
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...
Reporter | ||
Comment 8•15 years ago
|
||
now with .forget(), and some cases the old script missed.
Attachment #387276 -
Attachment is obsolete: true
Attachment #387364 -
Flags: review?(benjamin)
Updated•15 years ago
|
Attachment #387364 -
Flags: review?(benjamin) → review+
Reporter | ||
Updated•15 years ago
|
Attachment #387364 -
Flags: superreview?(dbaron)
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.
Attachment #387342 -
Flags: superreview?(benjamin) → superreview+
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.
Updated•15 years ago
|
Attachment #387364 -
Flags: superreview?(dbaron) → superreview+
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)
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>.
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.
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.
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?
Reporter | ||
Comment 16•15 years ago
|
||
Yep, I'll get back to this when I get a break in the ctypes stuff.
Reporter | ||
Comment 17•14 years ago
|
||
Reassigning to nobody. If anyone wants to work on this, feel free!
Assignee: dwitte → nobody
Status: ASSIGNED → NEW
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Updated•7 years ago
|
Product: Core → Firefox Build System
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•