Closed Bug 1163505 Opened 7 years ago Closed 7 years ago
Vector should disallow copies to prevent premature&double deletions
PtrVector holds a vector of pointers, and deletes all pointers on destruction. It also implicitly allows copies, which could result in two PtrVector's both pointing to the same objects, causing a double deletion; Or if one copy is deleted, users of the second copy will now see deleted objects. This does happens in a debug version of JsepSessionImpl::CreateAnswerMSection(), which calls JsepSessionImpl::GetCommonCodecs() to retrieve a PtrVector containing pointers to clones of codecs. However as the 'return' from GetCommonCodecs does not use tail-return-optimization in debug mode, it creates a copy of the PtrVector and destroys one; the program crashes a bit later when trying to use a deleted codec object. Copies should explicitly be disabled, and moves implemented to transfer ownership of the pointers as appropriate.
Deleted copy constructor&assignment, implemented move constructor&assignment.
Apologies, previous patch had wrong wrong wrong implementation of assignment! (But it's not used, that's why the compiler didn't complain) Deleted copy constructor&assignment, implemented move constructor&assignment.
Comment on attachment 8603975 [details] [diff] [review] 1163505-ptrvector-prevent-copy.patch Review of attachment 8603975 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine.
Attachment #8603975 - Flags: review?(docfaraday) → review+
Found this note in https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code: """rvalue references: Implicit move method generation cannot be used.""" Could this be why PtrVector was not implicitly moved when returned from JsepSessionImpl::GetCommonCodecs()?
Hey gerald - does bug 1161589 look like a dupe of this?
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5) > Hey gerald - does bug 1161589 look like a dupe of this? No. However, 'my' other recent bug 1163456 is a definite dupe of bug 1161589! Will link them...
Please make sure you have hg configured to include your author information in new patches as well.
Backed out for bustage. https://treeherder.mozilla.org/logviewer.html#?job_id=9918426&repo=mozilla-inbound
Patch with correct mozilla::Move instead of std::move.
Sorry for the snafu, forgot to upload the correct patch after successful try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=13b86813992c
You need to log in before you can comment on or make changes to this bug.