Closed
Bug 1504643
Opened 6 years ago
Closed 6 years ago
MozPromiseHolder::Resolve/Reject bypasses |explicit| keyword somehow
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
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 | ||
Updated•6 years 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•6 years ago
|
||
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
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/23849b2015ce
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in
before you can comment on or make changes to this bug.
Description
•