Closed Bug 1193762 Opened 5 years ago Closed 4 years ago

Disallow implicit conversion to T* from nsCOMPtr<T>&&

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

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.
Attachment #8647059 - Flags: review?(nfroyd)
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+
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)
Attachment #8647065 - Attachment is obsolete: true
Attachment #8647065 - Flags: review?(nfroyd)
Attachment #8647066 - Attachment is obsolete: true
Attachment #8647066 - Flags: review?(ehsan)
Attachment #8647063 - Attachment is obsolete: true
Attachment #8647063 - Flags: review?(nfroyd)
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)
Flags: needinfo?(nfroyd)
Attachment #8647056 - Flags: review?(nfroyd) → review+
Attachment #8647062 - Flags: review?(nfroyd) → review+
Attachment #8647061 - Flags: review?(nfroyd) → review+
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 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+
Flags: needinfo?(nfroyd)
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 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+
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)
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-
Attachment #8647052 - Flags: review?(ehsan) → review+
Attachment #8647053 - Flags: review?(ehsan) → review+
Attachment #8647055 - Flags: review?(ehsan) → review+
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)
If the problem is that GetBaseChannel might return a new object with a refcount of 1, why not return already_AddRefed?
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)
Note explanation in commit message about the TestCOMPtr.cpp changes.
Attachment #8739992 - Flags: review?(nfroyd)
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 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+
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)
(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)
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,
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?
(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 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+
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.
Attached patch Part 1 updatedSplinter Review
Attachment #8647052 - Attachment is obsolete: true
Attached patch Part 2 updatedSplinter Review
Attachment #8647053 - Attachment is obsolete: true
Attached patch Part 3 updatedSplinter Review
Attachment #8647055 - Attachment is obsolete: true
Checkin instructions at bug 1191356 comment 20 (first three parts only).
Flags: needinfo?(ayg)
Depends on: 1269316
You need to log in before you can comment on or make changes to this bug.