Closed Bug 1504643 Opened 2 years ago Closed 2 years ago

MozPromiseHolder::Resolve/Reject bypasses |explicit| keyword somehow

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(1 file)

Similaire to bug 1331137 and observed in bug 1504531.

We have a MediaDataDecoder::DecodePromise which is defined as:

  typedef nsTArray<RefPtr<MediaData>> DecodedData;
  typedef MozPromise<DecodedData, MediaResult, /* IsExclusive = */ true>
    DecodePromise;

In various places of the code we do:
https://searchfox.org/mozilla-central/rev/efc0d9172cb6a5849c6c4fc0f19d7fd5a2da9643/dom/media/platforms/agnostic/eme/EMEDecoderModule.cpp#357

MozPromiseHolder<DecodePromise>::Resolve(aResults) where aResults is a const DecodedData&
e.g.
const nsTArray<RefPtr<MediaDadata>>&

In bug 15404531 we attempted to use forward references throughout the MozPromise code to simplify things.

However, adding static_assert to ensure the Resolve/Reject are called with the right arguments as requested by :gerald suddenly enforced the explicit nsTArray copy constructor and caused many compilation errors wherever DecodePromise were used.

So at present, a lot of nsTArray copies are made implicitly when the nsTArray request that such copy be made explicitly.

So let's fix this, either by allowing implicit copy, or explicitly change all the callers so that the nsTArray copy is now explicit
Assignee: nobody → jyavenard
I think that templated classes like MozPromise should in general honor their template-parameter classes restrictions.
So I would personally recommend going for the IsConvertible test.

This means having to change lots of callers to do an explicit copy or conversion at the call site -- Which I think is a good thing as it forces the developer to think about that case, and it indicates to reviewers and maintainers that this copy/conversion was deemed necessary and acceptable.

An alternative (considered in bug 1331137) would be to provide a different function (e.g., `template<typename... Ts> void EmplaceResolve(const char*, Ts&&...)`) which would both make the copy implicit, and allow in-place construction from a variadic parameter pack.
In this case, "bad" call sites would only need to change which function is called instead of having to rework the whole argument temporary copy.
Make thorough use of move semantic for MozPromise using nsTArray
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/157be32d85fe
Enforce template-parameter classes restrictions in MozPromise. r=gerald
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f55b073ab55
Backed out changeset 157be32d85fe for conflicts while backing out bug 1471535
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23849b2015ce
Enforce template-parameter classes restrictions in MozPromise. r=gerald
https://hg.mozilla.org/mozilla-central/rev/23849b2015ce
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
See Also: → 1510265
You need to log in before you can comment on or make changes to this bug.