Closed
Bug 1193762
Opened 9 years ago
Closed 8 years ago
Disallow implicit conversion to T* from nsCOMPtr<T>&&
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: ayg, Assigned: ayg)
References
(Depends on 2 open bugs)
Details
Attachments
(11 files, 8 obsolete files)
6.78 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.82 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.75 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
4.00 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.35 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
16.89 KB,
patch
|
froydnj
:
review+
bwc
:
review+
roc
:
review-
|
Details | Diff | Splinter Review |
11.65 KB,
patch
|
froydnj
:
review+
froydnj
:
feedback+
|
Details | Diff | Splinter Review |
2.74 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
8.43 KB,
patch
|
Details | Diff | Splinter Review | |
10.61 KB,
patch
|
Details | Diff | Splinter Review | |
5.85 KB,
patch
|
Details | Diff | Splinter Review |
This requires significantly more changes than the preceding two bugs, particularly in editor. Due to the editor changes needed, it also depends on editor cleanup.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8647052 -
Flags: review?(ehsan)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8647053 -
Flags: review?(ehsan)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8647055 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8647056 -
Flags: review?(nfroyd)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8647059 -
Flags: review?(nfroyd)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8647060 -
Flags: review?(nfroyd)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8647061 -
Flags: review?(nfroyd)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8647062 -
Flags: review?(nfroyd)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8647063 -
Flags: review?(nfroyd)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8647064 -
Flags: review?(nfroyd)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8647065 -
Flags: review?(nfroyd)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8647066 -
Flags: review?(ehsan)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8647067 -
Flags: review?(nfroyd)
Comment 14•9 years ago
|
||
Comment on attachment 8647060 [details] [diff] [review] Part 6 -- Convert DecodePool::threads to nsTArray Review of attachment 8647060 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/DecodePool.cpp @@ +389,5 @@ > > { > MutexAutoLock lock(mMutex); > threads.AppendElements(mThreads); > mThreads.Clear(); I think we can make this: threads.SwapElements(mThreads); instead. @@ +395,5 @@ > } > > mImpl->RequestShutdown(); > > + for (int32_t i = 0 ; i < threads.Length() ; ++i) { Pretty sure you want to make this |uint32_t i| or you will get signed/unsigned comparison warnings.
Attachment #8647060 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8647064 [details] [diff] [review] Part 10 -- Give RefParam operator T*() These three patches are now included in bug 1194195 and bug 1194215.
Attachment #8647064 -
Attachment is obsolete: true
Attachment #8647064 -
Flags: review?(nfroyd)
Assignee | ||
Updated•9 years ago
|
Attachment #8647065 -
Attachment is obsolete: true
Attachment #8647065 -
Flags: review?(nfroyd)
Assignee | ||
Updated•9 years ago
|
Attachment #8647066 -
Attachment is obsolete: true
Attachment #8647066 -
Flags: review?(ehsan)
Assignee | ||
Updated•9 years ago
|
Attachment #8647063 -
Attachment is obsolete: true
Attachment #8647063 -
Flags: review?(nfroyd)
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8647066 [details] [diff] [review] Part 12 -- Use (Ref|Raw)Param in editor Oops, obsoleted the wrong patch. You might want to hold off on reviewing this if RefParam/RawParam get significantly reworked in the aforesaid bugs, though.
Attachment #8647066 -
Attachment is obsolete: false
Attachment #8647066 -
Flags: review?(ehsan)
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(nfroyd)
Updated•9 years ago
|
Attachment #8647056 -
Flags: review?(nfroyd) → review+
Updated•9 years ago
|
Attachment #8647062 -
Flags: review?(nfroyd) → review+
Updated•9 years ago
|
Attachment #8647061 -
Flags: review?(nfroyd) → review+
Comment 17•9 years ago
|
||
Comment on attachment 8647059 [details] [diff] [review] Part 5 -- Use more RefParam/RawParam Review of attachment 8647059 [details] [diff] [review]: ----------------------------------------------------------------- I still think bug 1179451 comment 48 is valid here. More comments elsewhere.
Attachment #8647059 -
Flags: review?(nfroyd)
Comment 18•9 years ago
|
||
Comment on attachment 8647067 [details] [diff] [review] Part 13 -- Delete nsCOMPtr<T>::operator T*()&& Review of attachment 8647067 [details] [diff] [review]: ----------------------------------------------------------------- I approve of this idea in principle, but I'm not sure of the right way to get to the point where we can check this patch in.
Attachment #8647067 -
Flags: review?(nfroyd) → review+
Updated•9 years ago
|
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 19•9 years ago
|
||
This actually works very nicely without RefParam/RawParam. But nsCOMPtr return types are now a no-no unless you want lots of .get()s in function calls. As currently organized, this patch set still depends on the editing patches. In theory I could try to break this dependency, but it doesn't seem worth it at this point. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f66c874b9cac
Attachment #8647059 -
Attachment is obsolete: true
Attachment #8647066 -
Attachment is obsolete: true
Attachment #8647066 -
Flags: review?(ehsan)
Attachment #8672395 -
Flags: review?(nfroyd)
Comment 20•9 years ago
|
||
Comment on attachment 8672395 [details] [diff] [review] Patch to replace RefParam/RawParam Review of attachment 8672395 [details] [diff] [review]: ----------------------------------------------------------------- I feel OK reviewing everything here except the media/mtransport/ changes and the editor/ changes; please have appropriate people look at those changes. (They seem OK to me, but maybe there are hidden invariants I'm not aware of.)
Attachment #8672395 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8672395 [details] [diff] [review] Patch to replace RefParam/RawParam Requesting review from Byron for media/mtransport and roc for editor.
Attachment #8672395 -
Flags: review?(roc)
Attachment #8672395 -
Flags: review?(docfaraday)
Updated•9 years ago
|
Attachment #8672395 -
Flags: review?(docfaraday) → review+
Comment on attachment 8672395 [details] [diff] [review] Patch to replace RefParam/RawParam Review of attachment 8672395 [details] [diff] [review]: ----------------------------------------------------------------- The rest looks fine. ::: dom/plugins/base/nsPluginStreamListenerPeer.cpp @@ +87,5 @@ > return r; > > nsCOMPtr<nsIChannel> base; > mp->GetBaseChannel(getter_AddRefs(base)); > + return base.get(); This is a little scary. In principle GetBaseChannel could return an object which does when we unref 'base', which would mean you're introducing a bug here. Why can't we continue to return an nsCOMPtr<nsIRequest> and fix callers instead?
Attachment #8672395 -
Flags: review?(roc) → review-
Updated•9 years ago
|
Attachment #8647052 -
Flags: review?(ehsan) → review+
Updated•9 years ago
|
Attachment #8647053 -
Flags: review?(ehsan) → review+
Updated•9 years ago
|
Attachment #8647055 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 23•8 years ago
|
||
Nathan, what do you think of comment 22? You gave r+ on that part of the patch, so is it okay to land, or do you concur with roc's comment that I should change it? (I don't understand it, and think something went missing -- which does what when we unref base?)
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 24•8 years ago
|
||
If the problem is that GetBaseChannel might return a new object with a refcount of 1, why not return already_AddRefed?
Comment 25•8 years ago
|
||
roc has the right of it; I believe your comment 24 understanding of it is correct. We should return some refcounted thing there and make necessary changes in the callers.
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 26•8 years ago
|
||
Note explanation in commit message about the TestCOMPtr.cpp changes.
Attachment #8739992 -
Flags: review?(nfroyd)
Assignee | ||
Comment 27•8 years ago
|
||
Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0fe904221dd
Assignee | ||
Comment 28•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fad1a48a4221
Attachment #8740054 -
Flags: review?(nfroyd)
Comment 29•8 years ago
|
||
Comment on attachment 8739992 [details] [diff] [review] More fixes to allow compilation on tip Review of attachment 8739992 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/util/ActiveElementManager.cpp @@ +208,5 @@ > mCanBePanSet = false; > } > > void > +ActiveElementManager::SetActiveTask(const nsCOMPtr<dom::Element>& aTarget) Why is this necessary? It seems like we don't want to introduce these sorts of arguments as long as we have implicit conversion into nsCOMPtr from T*, or we'll wind up with code that doesn't look like it refcounts when it actually does?
Attachment #8739992 -
Flags: review?(nfroyd) → feedback+
Comment 30•8 years ago
|
||
Comment on attachment 8740054 [details] [diff] [review] New GetBaseRequest fix Review of attachment 8740054 [details] [diff] [review]: ----------------------------------------------------------------- Please add local variables instead of using .get() so we can attempt to keep .get() around for when it's truly unsafe or absolutely necessary. r=me with that change.
Attachment #8740054 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 31•8 years ago
|
||
Comment on attachment 8739992 [details] [diff] [review] More fixes to allow compilation on tip (In reply to Nathan Froyd [:froydnj] from comment #29) > Why is this necessary? IIRC, NewRunnableMethod calls into some sort of generic template method that passes SetActiveTask a T&& (using an explicit mozilla::Move). With T == nsCOMPtr<Element>, it no longer compiles. (I don't remember the method where the error occurred.) > It seems like we don't want to introduce these sorts > of arguments as long as we have implicit conversion into nsCOMPtr from T*, > or we'll wind up with code that doesn't look like it refcounts when it > actually does? Meaning if you pass a T* to this function it will now addref? True, but I don't see any other way to get it to work, unless you want me to specialize whatever method caused the error to treat nsCOMPtr differently. Generally methods that are passed to NewRunnableMethod() are never called by anything else, so it doesn't seem misleading to allow the argument type in this case.
Attachment #8739992 -
Flags: review?(nfroyd)
Assignee | ||
Comment 32•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #30) > Please add local variables instead of using .get() so we can attempt to keep > .get() around for when it's truly unsafe or absolutely necessary. r=me with > that change. For the NS_ASSERTION case, I need to do DebugOnly<nsCOMPtr<nsIRequest>> baseRequest = GetBaseRequest(request); NS_ASSERTION(pslp->mRequests.IndexOfObject(nsCOMPtr<nsIRequest>(baseRequest)) != -1, because of no double implicit conversion. Do you still want this, or do you prefer .get() in this case?
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 33•8 years ago
|
||
Actually, that doesn't even work. I need this: DebugOnly<nsCOMPtr<nsIRequest>> baseRequest = GetBaseRequest(request); NS_ASSERTION(pslp->mRequests.IndexOfObject(static_cast<nsCOMPtr<nsIRequest>>(baseRequest)) != -1,
Assignee | ||
Comment 34•8 years ago
|
||
No, that doesn't work either. I had to resort to #ifdef DEBUG. Do you want that, or .get(), or do you have a better suggestion?
Comment 35•8 years ago
|
||
(In reply to :Aryeh Gregor (working until May 8) from comment #31) > Comment on attachment 8739992 [details] [diff] [review] > More fixes to allow compilation on tip > > (In reply to Nathan Froyd [:froydnj] from comment #29) > > Why is this necessary? > > IIRC, NewRunnableMethod calls into some sort of generic template method that > passes SetActiveTask a T&& (using an explicit mozilla::Move). With T == > nsCOMPtr<Element>, it no longer compiles. (I don't remember the method > where the error occurred.) Oh, blah, this is for some silly Chromium event loop thing, rather than NS_NewRunnableMethod*, isn't it? If it was the latter, we could explicitly say: NS_NewRunnableMethodWithArgs<nsCOMPtr<...>>(...); but there's nothing like that for the Chromium side. I guess we'll just have to live with |const nsCOMPtr&| arguments, then. (In reply to :Aryeh Gregor (working until May 8) from comment #34) > No, that doesn't work either. I had to resort to #ifdef DEBUG. Do you want > that, or .get(), or do you have a better suggestion? Ugh. #ifdef DEBUG, please.
Flags: needinfo?(nfroyd)
Comment 36•8 years ago
|
||
Comment on attachment 8739992 [details] [diff] [review] More fixes to allow compilation on tip Review of attachment 8739992 [details] [diff] [review]: ----------------------------------------------------------------- r+ on the ActiveElementManager thing, as discussed.
Attachment #8739992 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 37•8 years ago
|
||
This is now blocked on bug 1156062 comment 61. Once that's resolved, and those bugs are landed -- which will need to be done on a weekend with one push per patch to enable easy mozregression testing -- I'll do a new try run and land this.
Assignee | ||
Comment 38•8 years ago
|
||
Attachment #8647052 -
Attachment is obsolete: true
Assignee | ||
Comment 39•8 years ago
|
||
Attachment #8647053 -
Attachment is obsolete: true
Assignee | ||
Comment 40•8 years ago
|
||
Attachment #8647055 -
Attachment is obsolete: true
Assignee | ||
Comment 41•8 years ago
|
||
Checkin instructions at bug 1191356 comment 20 (first three parts only).
Keywords: checkin-needed,
leave-open
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 45•8 years ago
|
||
Backed these out because the dependency bug 1191356 had to be partially backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/bc3b9c99ab80b7c2557e432c568552fcb3c0a8a8 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ed89d5763095feee7595200672c553765f9ee64 https://hg.mozilla.org/integration/mozilla-inbound/rev/bf63766949b2c626ae62d56fd9137cdbc093ef56
Flags: needinfo?(ayg)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(ayg)
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 49•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8c79c89d06 https://hg.mozilla.org/integration/mozilla-inbound/rev/3d3cc3ecdca6 https://hg.mozilla.org/integration/mozilla-inbound/rev/eb70b194e002 https://hg.mozilla.org/integration/mozilla-inbound/rev/5c6e39260feb https://hg.mozilla.org/integration/mozilla-inbound/rev/c01fbb04e9a8 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0daadf6943c
Assignee | ||
Comment 50•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cce6d9628b9
Comment 51•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/05992224611e https://hg.mozilla.org/mozilla-central/rev/1d8d94e519f9 https://hg.mozilla.org/mozilla-central/rev/b742a1fd1d69 https://hg.mozilla.org/mozilla-central/rev/fb8c79c89d06 https://hg.mozilla.org/mozilla-central/rev/3d3cc3ecdca6 https://hg.mozilla.org/mozilla-central/rev/eb70b194e002 https://hg.mozilla.org/mozilla-central/rev/5c6e39260feb https://hg.mozilla.org/mozilla-central/rev/c01fbb04e9a8 https://hg.mozilla.org/mozilla-central/rev/a0daadf6943c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 52•8 years ago
|
||
\o/
You need to log in
before you can comment on or make changes to this bug.
Description
•