1584024 - IPDL promise resolver should be moved, not copied.

Status: NEW

(Core :: IPC, defect, P2)

Description

[Jean-Yves Avenard [:jya]]

I noticed a few misuse of IPDL MozPromise resolver like:
https://searchfox.org/mozilla-central/source/dom/media/gmp/ChromiumCDMChild.cpp#394

or:
https://searchfox.org/mozilla-central/source/dom/media/systemservices/MediaParent.cpp#459

In both case, the std::function resolver is copied rather than moved.

The issue normally arise when you need to do something like:

  void RecvPromiseMethods(IPDLType&& aArg, PromiseMethodsResolver&& aResolver) {
    MozPromiseMethod()->Then(thread, __func__,
        [aResolver](MozPromise::ResolveType&&) {
      // Resolved
      aResolver(...);
    }, [aResolver](MozPromise::RejectType&&) {
      // Rejected
      aResolver(...);
    });
  }

Here the aResolver would be captured by value, and as such copied instead of moved. And we need it for both resolve/reject cases.

A way to get around the issue is to use the MozPromise::ResolveOrRejectValue paradigm, the code above becomes:

  void RecvPromiseMethods(IPDLType&& aArg, PromiseMethodsResolver&& aResolver) {
    MozPromiseMethod()->Then(thread, __func__, [resolver = std::move(aResolver)]
        (MozPromise::ResolveOrRejectValue&& aValue) {
      if (aValue.IsResolve()) {
        aResolver(...);
      } else {
        aResolver(...);
      }
    });
  }

We move rather than copied the std::function object.

MediaParent.cpp the code goes half way correct, but still copy the lambda rather than move it.

It would be good to have a static analyser that reject such code.

Comment 1

[Jean-Yves Avenard [:jya]]

Ehsan, how difficult would it be to have our static analyser catch such use of std::function?

Comment 2

[(no longer active)]

(In reply to Jean-Yves Avenard [:jya] from comment #1)

Ehsan, how difficult would it be to have our static analyser catch such use of std::function?

Thinking about how that would work, we would need to somehow tell the compiler that this std::function is special, probably using an __attribute__. This is GetPrincipalKeyResolver which is auto-generated from this code. Now this would mean that we would need to modify the IPDL compiler to allow annotating the IPDL code in a way that would make it possible to lower that annotation into a special __attribute__ of some kind for the static analyzer to work off of.

But if we're going to go all that way, there is a much more straightforward option available, that is, modifying the IPDL compiler to allow a syntax here that would express the exact thing that we want, and generate the necessary C++ code directly, without needing static analysis at all. Here is how that would work IMO:

We could have:

  async GetPrincipalKey(...) returns moveonly(nsCString key); // moveonly is an existing IPDL keyword

And this would generate code like this inside PMediaParent.h:

  struct GetPrincipalKeyResolver : std::function<void(const nsCString&)> {
    GetPrincipalKeyResolver() {...}
    GetPrincipalKeyResolver(const GetPrincipalKeyResolver&) = delete;
    GetPrincipalKeyResolver(GetPrincipalKeyResolver&& rhs) {...}
    ...
  };

Then the first code snippet in comment 0 would not compile in the first place.

Wouldn't that be a better option?

Comment 3

[Jean-Yves Avenard [:jya]]

This sounds like a great alternative.

Will probably be sufficient for the IPDL MozPromise ; we may want to prevent all use copy of std::function in the future, but I'm no fuss about it.

Will you do it or should I?