Closed Bug 1313497 Opened 8 years ago Closed 8 years ago

MozPromise's InvokeAsync should allow more controlled argument storage (including references)

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

Details

Attachments

(13 files)

Bug 1313497 - Storages can be provided to InvokeAsync -
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1313497 - InvokeAsync taking a lambda -
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1313497 - Use InvokeAsync with lambda to replace MediaSourceDemuxer::AttemptInit -
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1313497 - Use InvokeAsync with Storages in MediaSourceDemuxer -
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1313497 - Pass TimeUnit by const& in MediaDataDemuxer -
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1313497 - Use InvokeAsync with Storages in MediaDecoderReaderWrapper -
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1313497 - Use InvokeAsync with Storages in MediaDecoderStateMachine -
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1313497 - Use InvokeAsync with Storages in TrackBuffersManager -
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1313497 - AppendBufferTask can take SourceBufferAttributes by const& -
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1313497 - Use InvokeAsync with Storages in GMPParent -
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1313497 - Use InvokeAsync with Storages in GMPServiceParent -
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1313497 - Expose Is/RemoveSmartPointer -
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1313497 - Use InvokeAsync with Storages in OmxDataDecoder -
58 bytes, text/x-review-board-request
jya
: review+
Details 
This is a follow-up to bug 1300476, where we disallowed storing references in the InvokeAsync runnable, in order to avoid potential UAF issues.

But this is potentially limiting, as there could be genuine cases where passing a reference is fine.
So we could adopt one of the existing ways to deal with this, from e.g.: NewRunnableMethod or WrapRunnable.
I'll add more details in the following comments.
(Benjamin Smedberg [:bsmedberg] from bug 1300476 comment #1)
> Auto-deducing argument types for async methods is intrinsically dangerous.
> That's why we require explicit template typing for NewRunnableMethod. I
> think we should require the same here.

(Nathan Froyd [:froydnj] from bug 1300476 comment #4)
> Comment on attachment 8804996 [details]
> https://reviewboard.mozilla.org/r/88804/#review88178
> 
> At first I thought, "yes, of course, r+" and then I thought, "wait, no,
> let's think about this" and then I thought "well, that'd be a lot of work." 
> So I'm on the fence, but tilting towards r+; mozreview doesn't give me
> anyway to say "r+0.5", so I'm setting r+, but I'd like to have a small
> discussion about this first.
> 
> ::: xpcom/threads/MozPromise.h:996
> (Diff revision 1)
> > +  static_assert(!detail::Any(IsReference<ArgTypes>::value...),
> > +                "Cannot pass reference types through InvokeAsync");
> 
> This seems like an expedient but limiting solution.  It's not technically
> incorrect for the the method itself to take references, but it would be
> incorrect for the runnable to have reference-valued member variables.  So I
> think this solution only works because `MethodCallType` doesn't care about
> `ActualArgTypes`, but only `ArgTypes`, and since we've verified that
> `ArgTypes` is reference-free, things effectively get copied and/or moved
> instead of `MethodCall` storing references?  Is that correct?
> 
> There are two ways of doing what we want here.  The first is to duplicate
> what `NewRunnableMethod` and friends do, and require explicit template types
> for `ActualArgTypes`, which would then get passed through to `MethodCall`. 
> The second is to adopt the `WrapRunnable` approach, but to be smarter about
> it: pass `ActualArgTypes` through to `MethodCall`, but to define `mArgs`
> thusly:
> 
> ```
>   Tuple<typename Decay<ActualArgTypes>::Type...> mArgs;
> ```
> 
> and thus effectively capture any arguments we need by copying them.
> 
> I know some people truly loathe having to provide explicit template argument
> types.  They do, however, provide some measure of security that implicit
> argument types can't provide.  OTOH, I think all the stuff
> `NewRunnableMethod` does under the hood with the template types you supply
> could be applied to implicit template argument types, so we could get the
> best of both worlds: safety and terseness.
> 
> On the third hand, I'm not keen on requiring you to rewrite a whole bunch of
> stuff just so we can make this check slightly more correct; reference
> arguments tend to be rare in our codebase anyway...

Another suggestion from Jean-Yves: Only require explicit argument types *if* one of them is a reference.

Note: I've added a mention of this bug in the static_assert message from bug 1300476, so interested developers may come here begging, or they could (please?!) work on it as needed.
Working on it.
I'll keep the old InvokeAsync, good for simple cases where passing by-value is good enough (including pointers), and obviously less code to change.
I'll add one with the same template parameters as NewRunnableMethod.
I'll also add one that takes a lambda, good when a separate method is not really necessary.
Assignee: nobody → gsquelart
Attachment #8810231 - Flags: review?(jyavenard) → review?(nfroyd)
Attachment #8810232 - Flags: review?(jyavenard) → review?(nfroyd)
Comment on attachment 8810233 [details]
Bug 1313497 - Use InvokeAsync with lambda to replace MediaSourceDemuxer::AttemptInit -

https://reviewboard.mozilla.org/r/92602/#review92596

::: dom/media/mediasource/MediaSourceDemuxer.cpp:41
(Diff revision 1)
>  MediaSourceDemuxer::Init()
>  {
> -  return InvokeAsync(GetTaskQueue(), this, __func__,
> -                     &MediaSourceDemuxer::AttemptInit);
> -}
> -
> +  RefPtr<MediaSourceDemuxer> self = this;
> +  return InvokeAsync(GetTaskQueue(), __func__,
> +    [self](){
> +      MOZ_ASSERT(self->OnTaskQueue());

I don't think this assert is required now...

there's no other way due to the arguments passed to InvokeAsync.
There's no way to call this code outside of the taskqueue
Attachment #8810233 - Flags: review?(jyavenard) → review+
Comment on attachment 8810235 [details]
Bug 1313497 - Pass TimeUnit by const& in MediaDataDemuxer -

https://reviewboard.mozilla.org/r/92606/#review92598

::: dom/media/MediaDataDemuxer.h:179
(Diff revision 1)
>    // Skip frames until the next Random Access Point located after
>    // aTimeThreshold.
>    // The first frame returned by the next call to GetSamples() will be the
>    // first random access point found after aTimeThreshold.
>    // Upon success, returns the number of frames skipped.
> -  virtual RefPtr<SkipAccessPointPromise> SkipToNextRandomAccessPoint(media::TimeUnit aTimeThreshold) = 0;
> +  virtual RefPtr<SkipAccessPointPromise> SkipToNextRandomAccessPoint(const media::TimeUnit& aTimeThreshold) = 0;

80 columns formatting.

wmouhaaahahahaha (evil laugh)

::: dom/media/fmp4/MP4Demuxer.cpp:52
(Diff revision 1)
>  
>    void Reset() override;
>  
>    nsresult GetNextRandomAccessPoint(media::TimeUnit* aTime) override;
>  
> -  RefPtr<SkipAccessPointPromise> SkipToNextRandomAccessPoint(media::TimeUnit aTimeThreshold) override;
> +  RefPtr<SkipAccessPointPromise> SkipToNextRandomAccessPoint(const media::TimeUnit& aTimeThreshold) override;

80 columns...

(all the times you annoyed me with this are re-surfacing)
Attachment #8810235 - Flags: review?(jyavenard) → review+
Comment on attachment 8810239 [details]
Bug 1313497 - AppendBufferTask can take SourceBufferAttributes by const& -

https://reviewboard.mozilla.org/r/92614/#review92600
Attachment #8810239 - Flags: review?(jyavenard) → review+
Comment on attachment 8810236 [details]
Bug 1313497 - Use InvokeAsync with Storages in MediaDecoderReaderWrapper -

https://reviewboard.mozilla.org/r/92608/#review92602

::: dom/media/MediaDecoderReaderWrapper.h:74
(Diff revision 1)
>    bool IsRequestingAudioData() const;
>    bool IsRequestingVideoData() const;
>    bool IsWaitingAudioData() const;
>    bool IsWaitingVideoData() const;
>  
> -  RefPtr<SeekPromise> Seek(SeekTarget aTarget, media::TimeUnit aEndTime);
> +  RefPtr<SeekPromise> Seek(SeekTarget aTarget, const media::TimeUnit& aEndTime);

Can we make SeekTarget argument a const reference too?
no longer need to pass it by value.

Especially as you changed the type later on for the call to MediaDecoderReader::Seek
Attachment #8810236 - Flags: review?(jyavenard) → review+
Comment on attachment 8810237 [details]
Bug 1313497 - Use InvokeAsync with Storages in MediaDecoderStateMachine -

https://reviewboard.mozilla.org/r/92610/#review92604

::: dom/media/MediaDecoderStateMachine.cpp:2459
(Diff revision 1)
>  }
>  
>  RefPtr<MediaDecoder::SeekPromise>
> -MediaDecoderStateMachine::InvokeSeek(SeekTarget aTarget)
> +MediaDecoderStateMachine::InvokeSeek(const SeekTarget& aTarget)
>  {
> -  return InvokeAsync(OwnerThread(), this, __func__,
> +  return InvokeAsyncWithStorages<StoreCopyPassByConstLRef<SeekTarget>>(

this is the most awkward way to pass arguments I've ever seen :(
Attachment #8810237 - Flags: review?(jyavenard) → review+
Comment on attachment 8810238 [details]
Bug 1313497 - Use InvokeAsync with Storages in TrackBuffersManager -

https://reviewboard.mozilla.org/r/92612/#review92606
Attachment #8810238 - Flags: review?(jyavenard) → review+
Comment on attachment 8810240 [details]
Bug 1313497 - Use InvokeAsync with Storages in GMPParent -

https://reviewboard.mozilla.org/r/92616/#review92608
Attachment #8810240 - Flags: review?(jyavenard) → review+
Comment on attachment 8810241 [details]
Bug 1313497 - Use InvokeAsync with Storages in GMPServiceParent -

https://reviewboard.mozilla.org/r/92618/#review92610
Attachment #8810241 - Flags: review?(jyavenard) → review+
Comment on attachment 8810234 [details]
Bug 1313497 - Use InvokeAsync with Storages in MediaSourceDemuxer -

https://reviewboard.mozilla.org/r/92604/#review92612

I'd prefer to use lambda...
Attachment #8810234 - Flags: review?(jyavenard) → review+
Comment on attachment 8810234 [details]
Bug 1313497 - Use InvokeAsync with Storages in MediaSourceDemuxer -

https://reviewboard.mozilla.org/r/92604/#review92612

The 'Do...' target function are quite big, which I think would be ugly for lambdas.
But this can be changed later on if preferred.
(In reply to Jean-Yves Avenard [:jya] from comment #18)
> Comment on attachment 8810237 [details]
> Bug 1313497 - Use InvokeAsync with Storages in MediaDecoderStateMachine -
> 
> https://reviewboard.mozilla.org/r/92610/#review92604
> 
> ::: dom/media/MediaDecoderStateMachine.cpp:2459
> (Diff revision 1)
> >  }
> >  
> >  RefPtr<MediaDecoder::SeekPromise>
> > -MediaDecoderStateMachine::InvokeSeek(SeekTarget aTarget)
> > +MediaDecoderStateMachine::InvokeSeek(const SeekTarget& aTarget)
> >  {
> > -  return InvokeAsync(OwnerThread(), this, __func__,
> > +  return InvokeAsyncWithStorages<StoreCopyPassByConstLRef<SeekTarget>>(
> 
> this is the most awkward way to pass arguments I've ever seen :(

Yes...I'm wondering if we shouldn't make StoreCopyPassByConstLRef the default.  The default is currently StoreCopyPassByValue, which is probably not what you want, and I think StoreCopyPassByConstLRef would provide essentially the same functionality if the method *did* take its arguments by value.
Comment on attachment 8810232 [details]
Bug 1313497 - InvokeAsync taking a lambda -

https://reviewboard.mozilla.org/r/92600/#review93156

I think this is OK with the changes below.

::: xpcom/threads/MozPromise.h:1086
(Diff revision 4)
> +    return NS_OK;
> +  }
> +
> +private:
> +  RefPtr<typename PromiseType::Private> mProxyPromise;
> +  nsAutoPtr<FunctionStorage> mFunction;

Please use `UniquePtr` here; we're trying to not introduce new `nsAutoPtr` usages.

::: xpcom/threads/MozPromise.h:1091
(Diff revision 4)
> +// We prefer getting function objects by non-lvalue-ref (to avoid copying them
> +// and their captures). This struct is a tag that allows the use of objects
> +// through lvalue-refs where necessary.
> +struct AllowInvokeAsyncFunctionLVRef {};

The need for this mechanism ought to be fairly rare, right?  (I confess to not wanting to scan through the patches to see if it's used in the current patch series.)  I'm wondering if we could just put this struct and the associated `InvokeAsync` overload in `detail::`.

::: xpcom/threads/MozPromise.h:1101
(Diff revision 4)
> +InvokeAsync(AbstractThread* aTarget, const char* aCallerName,
> +            AllowInvokeAsyncFunctionLVRef, Function&& aFunction)

I have a slight preference for swapping the last two arguments here, but perhaps the unusual argument ordering will make people think twice about using it?

::: xpcom/threads/MozPromise.h:1105
(Diff revision 4)
> +  static_assert(detail::IsRefPtrMozPromise<decltype(aFunction())>::value,
> +                "Function object must return RefPtr<MozPromise>");
> +  typedef typename detail::IsRefPtrMozPromise<decltype(aFunction())>::PromiseType PromiseType;

Maybe:

```
// Not really sure what name to give this.
typedef detail::IsRefPtrMozPromise<decltype(aFunction())> Selector;
static_assert(Selector::value, ...);
typedef typename Selector::PromiseType PromiseType;
```

would be a little tidier?
Attachment #8810232 - Flags: review?(nfroyd) → review+
Comment on attachment 8810231 [details]
Bug 1313497 - Storages can be provided to InvokeAsync -

https://reviewboard.mozilla.org/r/92598/#review93172

I think this all looks reasonable.
Attachment #8810231 - Flags: review?(nfroyd) → review+
Comment on attachment 8810232 [details]
Bug 1313497 - InvokeAsync taking a lambda -

https://reviewboard.mozilla.org/r/92600/#review93156

Thank you for the review so far.

Before I go ahead with any changes, would you be happy to consider my answers below? Just to be sure we get this right, as it's a "New Kind of API" (if I could be so bold! ... Or just over-excited)

> Please use `UniquePtr` here; we're trying to not introduce new `nsAutoPtr` usages.

I thought about that, but as this is copying good old `ProxyRunnable` above, I kept `nsAutoPtr` for consistency.
Happy to change to `UniquePtr`, I prefer it as well.

Should I change the `nsAutoPtr` in `ProxyRunnable` too while I'm in the vicinity?

> The need for this mechanism ought to be fairly rare, right?  (I confess to not wanting to scan through the patches to see if it's used in the current patch series.)  I'm wondering if we could just put this struct and the associated `InvokeAsync` overload in `detail::`.

My goal was to provide the strongly-preferred API (taking temporaries) as the obvious choice, but leave the door open for copied functors for those who actually need it.
The tag is proof that the programmers really thought about the problem and accepts the cost of copies or thinks they're cheap.
"Make APIs hard to use incorrectly" kind of thinking.

Yes, this mechanism ought to be rare, but maybe not never-needed?
E.g., the function object could be stored in a `std::function` elsewhere; or the same lambda is needed multiple times within a function: `auto f = []{return...;}; for (...) { InvokeAsync(thread, __func__, AllowInvokeAsyncFunctionLVRef{}, f); }`. Of course that's a silly example, AFAIK we have not needed such monstrosity so far in related `MozPromise::Then()`.
But someone asked me about that recently, which is why I added this mechanism here -- in the end they could change their logic to only use one lambda, which I admit plays into your hands!

So if you think this optional API should not even be offered, I can just remove it completely, and change the static_assert message accordingly.
I just like to offer considered options to those who might one day need it...


Spoiler alert: I intended to use the same technique (in a later bug) with the lambda-taking `MozPromise::Then()`, to allow copied function objects, also with a tag to show intent. Should I kill it?

> I have a slight preference for swapping the last two arguments here, but perhaps the unusual argument ordering will make people think twice about using it?

There's that (making people think twice), and I think my ordering is more obvious, reading from left to right (and top-to-bottom when call gets split over multiple lines): "oh, this next function is copied, alright".
me : `InvokeAsync(thread, __func__, AllowInvokeAsyncFunctionLVRef{}, arrayOfStdFunctions[i]);`
you: `InvokeAsync(thread, __func__, arrayOfStdFunctions[i], AllowInvokeAsyncFunctionLVRef{});`
But happy to change, you have broader and wiser knowledge of the whole system.

> Maybe:
> 
> ```
> // Not really sure what name to give this.
> typedef detail::IsRefPtrMozPromise<decltype(aFunction())> Selector;
> static_assert(Selector::value, ...);
> typedef typename Selector::PromiseType PromiseType;
> ```
> 
> would be a little tidier?

Yes indeed, good idea.

While we're discussing, I was not sure "Is..." is a good class name, as it does both an 'is' check, *and* a type extraction. Any better name that would capture both actions?
Or I could have separate classes, e.g.: `IsMozPromise`, and more traditional `IsRefPtr` and `RemoveRefPtr` (these last two could be placed somewhere more public (RefPtr.h?), if you think that could be useful).
(In reply to Nathan Froyd [:froydnj] from comment #56)
> (In reply to Jean-Yves Avenard [:jya] from comment #18)
> > >  RefPtr<MediaDecoder::SeekPromise>
> > >  MediaDecoderStateMachine::InvokeSeek(const SeekTarget& aTarget)
> > >  {
> > >    return InvokeAsyncWithStorages<StoreCopyPassByConstLRef<SeekTarget>>(
> > 
> > this is the most awkward way to pass arguments I've ever seen :(

(Just for context, this was a previous version with InvokeAsyncWithStorages and StoreCopyPassByConstLRef, which I've later changed to InvokeAsync and StoreCopyPassByConstLRef, more about that last one below.)
I think Jean-Yves is complaining about the subjective ugliness of the whole statement. :-)

> Yes...I'm wondering if we shouldn't make StoreCopyPassByConstLRef the
> default.  The default is currently StoreCopyPassByValue, which is probably
> not what you want, and I think StoreCopyPassByConstLRef would provide
> essentially the same functionality if the method *did* take its arguments by
> value.

One thing that we know about InvokeAsync (and which we cannot know about NewRunnableMethod), is that the given method will be run exactly once, never more.
This means that it is safe to move() from our stored copies of the arguments (as they'll soon be destroyed anyway), which could be more efficient for some types.

SeekTarget is currently not inherently movable, so we'll just fallback to the old copy from a const-lref; but if SeekTarget ever evolved into a move-friendly type (e.g., one day it could gain a RefPtr to something), we'll get the optimization for free.

So I do believe that specifying StoreCopyPassByRRef is the best option in most uses of InvokeAsync. And despite being somewhat "ugly", it also has the benefit of being explicit about what we're doing.


Alternatively, instead of using the NewRunnable stuff as-is (which prefers passing by-value) in the first patch, I could somehow modify it to prefer passing by-rref, in which case we could just write a more beautiful `InvokeAsync<SeekTarget>`.
Thoughts?
Comment on attachment 8810232 [details]
Bug 1313497 - InvokeAsync taking a lambda -

https://reviewboard.mozilla.org/r/92600/#review93156

> I thought about that, but as this is copying good old `ProxyRunnable` above, I kept `nsAutoPtr` for consistency.
> Happy to change to `UniquePtr`, I prefer it as well.
> 
> Should I change the `nsAutoPtr` in `ProxyRunnable` too while I'm in the vicinity?

We're never going to convince people of the greatness of `UniquePtr` if we keep adding uses of `nsAutoPtr`. :)

Please change the one in `ProxyRunnable` in a separate bug unless you're already modifying the code declaring the member and the member's uses.

> My goal was to provide the strongly-preferred API (taking temporaries) as the obvious choice, but leave the door open for copied functors for those who actually need it.
> The tag is proof that the programmers really thought about the problem and accepts the cost of copies or thinks they're cheap.
> "Make APIs hard to use incorrectly" kind of thinking.
> 
> Yes, this mechanism ought to be rare, but maybe not never-needed?
> E.g., the function object could be stored in a `std::function` elsewhere; or the same lambda is needed multiple times within a function: `auto f = []{return...;}; for (...) { InvokeAsync(thread, __func__, AllowInvokeAsyncFunctionLVRef{}, f); }`. Of course that's a silly example, AFAIK we have not needed such monstrosity so far in related `MozPromise::Then()`.
> But someone asked me about that recently, which is why I added this mechanism here -- in the end they could change their logic to only use one lambda, which I admit plays into your hands!
> 
> So if you think this optional API should not even be offered, I can just remove it completely, and change the static_assert message accordingly.
> I just like to offer considered options to those who might one day need it...
> 
> 
> Spoiler alert: I intended to use the same technique (in a later bug) with the lambda-taking `MozPromise::Then()`, to allow copied function objects, also with a tag to show intent. Should I kill it?

My vote would be to put it in `mozilla::detail::`, and then if somebody comes along with a real use-case, we can just move it out of the `detail::` namespace.  I think the technique is fine, I would just like to restrict use of it until we come up with actual use cases.

> There's that (making people think twice), and I think my ordering is more obvious, reading from left to right (and top-to-bottom when call gets split over multiple lines): "oh, this next function is copied, alright".
> me : `InvokeAsync(thread, __func__, AllowInvokeAsyncFunctionLVRef{}, arrayOfStdFunctions[i]);`
> you: `InvokeAsync(thread, __func__, arrayOfStdFunctions[i], AllowInvokeAsyncFunctionLVRef{});`
> But happy to change, you have broader and wiser knowledge of the whole system.

The current ordering is fine.

> Yes indeed, good idea.
> 
> While we're discussing, I was not sure "Is..." is a good class name, as it does both an 'is' check, *and* a type extraction. Any better name that would capture both actions?
> Or I could have separate classes, e.g.: `IsMozPromise`, and more traditional `IsRefPtr` and `RemoveRefPtr` (these last two could be placed somewhere more public (RefPtr.h?), if you think that could be useful).

I don't have any better ideas for the name.  I agree that it would be cleaner to split this up into several traits...except that once you write out all those traits, that's quite a lot of code for something that's currently pretty simple.

Although...nsThreadUtils.h already has `IsRefcountedSmartPointer` and `StripSmartPointer` traits, which I wouldn't feel bad about re-using here.  They handle `nsCOMPtr` as well, but that shouldn't be too much of a problem, right?  So then you'd just need `IsMozPromise`, IIUC.
(In reply to Gerald Squelart [:gerald] from comment #60)
> (In reply to Nathan Froyd [:froydnj] from comment #56)
> > Yes...I'm wondering if we shouldn't make StoreCopyPassByConstLRef the
> > default.  The default is currently StoreCopyPassByValue, which is probably
> > not what you want, and I think StoreCopyPassByConstLRef would provide
> > essentially the same functionality if the method *did* take its arguments by
> > value.
> 
> One thing that we know about InvokeAsync (and which we cannot know about
> NewRunnableMethod), is that the given method will be run exactly once, never
> more.
> This means that it is safe to move() from our stored copies of the arguments
> (as they'll soon be destroyed anyway), which could be more efficient for
> some types.

Hm.  But this might not always work.  Consider something like:

void DoSomething(RefCountedType* p);

I don't think you can use that with InvokeAsync, because the runnable we create under the hood is storing RefPtr<RefCountedType>, and then for parameter passing, we try to pass RefPtr<RefCountedType>&&.  But we prohibit implicit conversion from RefPtr<T> to T* for temporary objects (to avoid memory safety issues), so won't the current scheme fail to compile in some cases?

> SeekTarget is currently not inherently movable, so we'll just fallback to
> the old copy from a const-lref; but if SeekTarget ever evolved into a
> move-friendly type (e.g., one day it could gain a RefPtr to something),
> we'll get the optimization for free.
> 
> So I do believe that specifying StoreCopyPassByRRef is the best option in
> most uses of InvokeAsync. And despite being somewhat "ugly", it also has the
> benefit of being explicit about what we're doing.

Modulo the caveat about, I think you are correct.  My comment about using StoreCopyPassByConstLRef was for the whole NewRunnable which InvokeAsync piggybacks off of:

http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h#700

or thereabouts (the SmartPointerStorageClass traits class).  I think if we did that, then your example becomes:

InvokeAsyncWithStorages<SeekTarget>(...)

or it can possibly forego the *WithStorages variant altogether?

Unsure if we are talking about the same things here.
(In reply to Nathan Froyd [:froydnj] from comment #62)
> (In reply to Gerald Squelart [:gerald] from comment #60)
> > (In reply to Nathan Froyd [:froydnj] from comment #56)
> > > Yes...I'm wondering if we shouldn't make StoreCopyPassByConstLRef the
> > > default.  The default is currently StoreCopyPassByValue, which is probably
> > > not what you want, and I think StoreCopyPassByConstLRef would provide
> > > essentially the same functionality if the method *did* take its arguments by
> > > value.
> > 
> > One thing that we know about InvokeAsync (and which we cannot know about
> > NewRunnableMethod), is that the given method will be run exactly once, never
> > more.
> > This means that it is safe to move() from our stored copies of the arguments
> > (as they'll soon be destroyed anyway), which could be more efficient for
> > some types.
> 
> Hm.  But this might not always work.  Consider something like:
> 
> void DoSomething(RefCountedType* p);
> 
> I don't think you can use that with InvokeAsync, because the runnable we
> create under the hood is storing RefPtr<RefCountedType>, and then for
> parameter passing, we try to pass RefPtr<RefCountedType>&&.  But we prohibit
> implicit conversion from RefPtr<T> to T* for temporary objects (to avoid
> memory safety issues), so won't the current scheme fail to compile in some
> cases?

Good thinking, we may not want to move from *all* types!
Luckily, in the case of ref-counted types, unless explicitly-specified we automagically use StorensRefPtrPassByPtr, i.e. we store into a RefPtr, but we then pass a raw pointer to the target function.

But I was thinking of the final non-special case handled by ParameterStorage, 'other T':
http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/xpcom/glue/nsThreadUtils.h#740
Currently we select StoreCopyPassByValue (store a copy, pass another copy to the target function).

My thinking was that we should replace that "default" choice with StoreCopyPassByRRef (store a copy, move from it into the target function), either explicitly at every InvokeAsync call (which is what I have done in most patches here), or implicitly by changing the ParameterStorage in that case (this could be done in another bug if we see value).

> > SeekTarget is currently not inherently movable, so we'll just fallback to
> > the old copy from a const-lref; but if SeekTarget ever evolved into a
> > move-friendly type (e.g., one day it could gain a RefPtr to something),
> > we'll get the optimization for free.
> > 
> > So I do believe that specifying StoreCopyPassByRRef is the best option in
> > most uses of InvokeAsync. And despite being somewhat "ugly", it also has the
> > benefit of being explicit about what we're doing.
> 
> Modulo the caveat about, I think you are correct.  My comment about using
> StoreCopyPassByConstLRef was for the whole NewRunnable which InvokeAsync
> piggybacks off of:
> 
> http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h#700
> 
> or thereabouts (the SmartPointerStorageClass traits class).  I think if we
> did that, then your example becomes:
> 
> InvokeAsyncWithStorages<SeekTarget>(...)
> 
> or it can possibly forego the *WithStorages variant altogether?
> 
> Unsure if we are talking about the same things here.

I now realise that you were talking about that default choice for NewRunnableMethod itself! I also wondered about that passing by-value, I'll just open a new bug so we can think about it and fix it if needed...

But regardless of that, as I said above I think InvokeAsync has the advantage of knowing the function will be called exactly once, so we can pass arguments by rvalue-references when not handling special types like RefPtr, which will still follow the same rules as NewRunnableMethod.

And there is no 'InvokeAsyncWithStorages' anymore, it's just 'InvokeAsync' now.
Comment on attachment 8811565 [details]
Bug 1313497 - Expose Is/RemoveSmartPointer -

https://reviewboard.mozilla.org/r/93638/#review94316

I just meant that the `InvokeAsync` code could pluck things out of the `detail::` namespace, but this works too!

::: xpcom/glue/nsThreadUtils.h:667
(Diff revision 2)
> +template<typename>
> +struct SFINAE1True : mozilla::TrueType
> +{};
> +
> +template<class T>
> +static auto HasRefCountMethodsTest(int)
> +    -> SFINAE1True<decltype(mozilla::DeclVal<T>().AddRef(),
> +                            mozilla::DeclVal<T>().Release())>;
> +template<class>
> +static auto HasRefCountMethodsTest(long) -> mozilla::FalseType;
> +
> +template<class T>
> +struct HasRefCountMethods : decltype(HasRefCountMethodsTest<T>(0))
> +{};

Would you mind moving all of this prior to the `NonnsISupportsPointerStorageClass`?  Just to keep the storage class traits classes together.  Thanks!
Attachment #8811565 - Flags: review?(nfroyd) → review+
(In reply to Gerald Squelart [:gerald] from comment #86)
> (In reply to Nathan Froyd [:froydnj] from comment #62)
> > (In reply to Gerald Squelart [:gerald] from comment #60)
> > > (In reply to Nathan Froyd [:froydnj] from comment #56)
> > > > Yes...I'm wondering if we shouldn't make StoreCopyPassByConstLRef the
> > > > default.  The default is currently StoreCopyPassByValue, which is probably
> > > > not what you want, and I think StoreCopyPassByConstLRef would provide
> > > > essentially the same functionality if the method *did* take its arguments by
> > > > value.
> > > 
> > > One thing that we know about InvokeAsync (and which we cannot know about
> > > NewRunnableMethod), is that the given method will be run exactly once, never
> > > more.
> > > This means that it is safe to move() from our stored copies of the arguments
> > > (as they'll soon be destroyed anyway), which could be more efficient for
> > > some types.
> > 
> > Hm.  But this might not always work.  Consider something like:
> > 
> > void DoSomething(RefCountedType* p);
> > 
> > I don't think you can use that with InvokeAsync, because the runnable we
> > create under the hood is storing RefPtr<RefCountedType>, and then for
> > parameter passing, we try to pass RefPtr<RefCountedType>&&.  But we prohibit
> > implicit conversion from RefPtr<T> to T* for temporary objects (to avoid
> > memory safety issues), so won't the current scheme fail to compile in some
> > cases?
> 
> Good thinking, we may not want to move from *all* types!
> Luckily, in the case of ref-counted types, unless explicitly-specified we
> automagically use StorensRefPtrPassByPtr, i.e. we store into a RefPtr, but
> we then pass a raw pointer to the target function.
> 
> But I was thinking of the final non-special case handled by
> ParameterStorage, 'other T':
> http://searchfox.org/mozilla-central/rev/
> efcb1644e49c36445e75d89b434fdf4c84832c84/xpcom/glue/nsThreadUtils.h#740
> Currently we select StoreCopyPassByValue (store a copy, pass another copy to
> the target function).
> 
> My thinking was that we should replace that "default" choice with
> StoreCopyPassByRRef (store a copy, move from it into the target function),
> either explicitly at every InvokeAsync call (which is what I have done in
> most patches here), or implicitly by changing the ParameterStorage in that
> case (this could be done in another bug if we see value).

Apologies for introducing extra lag into a conversation that already takes a day to turn things around due to timezone differences.  My comment was directed at InvokeAsync, which currently has up on MozReview:

  return detail::InvokeAsyncImpl<StoreCopyPassByRRef<ArgTypes>...>(
           aTarget, aThisVal, aCallerName, aMethod,

which is subject to the problem I described above, right?  And this also has issues if one does (as is common in Gecko code):

  void DoSomething(..., RefCountedType* aObject, ...)
  {
     ...InvokeAsync(..., aObject, ...)
  }

as we're now storing a pointer to the object, rather than a RefPtr<T> to the object.  Yuck. :(
(In reply to Nathan Froyd [:froydnj] from comment #100)
> Apologies for introducing extra lag into a conversation that already takes a
> day to turn things around due to timezone differences.

That's fine, we want to get this right, thank you for all the feedback.

> My comment was
> directed at InvokeAsync, which currently has up on MozReview:
>   return detail::InvokeAsyncImpl<StoreCopyPassByRRef<ArgTypes>...>(
>            aTarget, aThisVal, aCallerName, aMethod,
> which is subject to the problem I described above, right?  And this also has
> issues if one does (as is common in Gecko code):
>   void DoSomething(..., RefCountedType* aObject, ...)
>   {
>      ...InvokeAsync(..., aObject, ...)
>   }
> as we're now storing a pointer to the object, rather than a RefPtr<T> to the
> object.  Yuck. :(

Yes, that could be dangerous.

In the latest push, I'm now also refusing pointers in "naked" InvokeAsync (in the first patch). So only concrete objects (that the target method requests) will now get the default StoreCopyPassByRRef treatment.

Only one location was affected, I've added a patch to fix it.
Attachment #8812395 - Flags: review?(nfroyd) → review?(jyavenard)
Comment on attachment 8812395 [details]
Bug 1313497 - Use InvokeAsync with Storages in OmxDataDecoder -

https://reviewboard.mozilla.org/r/94162/#review95800
Attachment #8812395 - Flags: review?(jyavenard) → review+
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6d2c739857f
Storages can be provided to InvokeAsync - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/68eaaa3f96e3
Expose Is/RemoveSmartPointer - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/75ac87c67bee
InvokeAsync taking a lambda - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/a530742d6005
Use InvokeAsync with lambda to replace MediaSourceDemuxer::AttemptInit - r=jya
https://hg.mozilla.org/integration/autoland/rev/3583f85c09d4
Use InvokeAsync with Storages in MediaSourceDemuxer - r=jya
https://hg.mozilla.org/integration/autoland/rev/b1f632851ee5
Pass TimeUnit by const& in MediaDataDemuxer - r=jya
https://hg.mozilla.org/integration/autoland/rev/87a0350cf96f
Use InvokeAsync with Storages in MediaDecoderReaderWrapper - r=jya
https://hg.mozilla.org/integration/autoland/rev/23b59e8e7f71
Use InvokeAsync with Storages in MediaDecoderStateMachine - r=jya
https://hg.mozilla.org/integration/autoland/rev/2125fc90d9bc
Use InvokeAsync with Storages in TrackBuffersManager - r=jya
https://hg.mozilla.org/integration/autoland/rev/40e0fe1aa9d9
AppendBufferTask can take SourceBufferAttributes by const& - r=jya
https://hg.mozilla.org/integration/autoland/rev/30674252d7f5
Use InvokeAsync with Storages in GMPParent - r=jya
https://hg.mozilla.org/integration/autoland/rev/7b4b358976d4
Use InvokeAsync with Storages in GMPServiceParent - r=jya
https://hg.mozilla.org/integration/autoland/rev/4e38b63c7829
Use InvokeAsync with Storages in OmxDataDecoder - r=jya
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: