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

RESOLVED FIXED in Firefox 65

Status

()

enhancement
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: jya, Assigned: jya)

Tracking

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

7 months ago
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

Updated

7 months ago
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.
Assignee

Comment 2

7 months ago
Make thorough use of move semantic for MozPromise using nsTArray

Comment 3

7 months ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/157be32d85fe
Enforce template-parameter classes restrictions in MozPromise. r=gerald

Comment 4

7 months ago
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f55b073ab55
Backed out changeset 157be32d85fe for conflicts while backing out bug 1471535

Comment 5

7 months ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23849b2015ce
Enforce template-parameter classes restrictions in MozPromise. r=gerald

Comment 6

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/23849b2015ce
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Assignee

Updated

7 months ago
See Also: → 1510265
You need to log in before you can comment on or make changes to this bug.