Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2353093002:
Assert that dictionary can own the objects it is given. (Closed)
DescriptionAssert that dictionary can own the objects it is given.
Upon indirect object holder destruction, all indirect
objects are destroyed -- currently by order of increasing
object number -- but ideally without ordering constraints.
So currently, we can get away with a dictionary pointing
directly at an indirect object with a higher number. It
gets destroyed first, invoking Release() on its subordinates,
which skips destroying them if they are indirect objects. But
we don't want to rely on this artifact of destruction
order. Should it happen to be reversed, the dictionary
would invoke Release() on freed memory.
Interestingly, CPDF_Array skirts the issue by replacing
any indirect objects it is given with references. Not
clear whether we should do the same thing for dictionaries,
or remove it from arrays. The technique certainly
complicates understanding ownership.
The one violation found is in the unittest that broke the
previous CL which tried to use unique_ptrs in indirect
object holder.
Committed: https://pdfium.googlesource.com/pdfium/+/393fe4943226846a9b99878406d0bf75f31bb643
Patch Set 1 #
Messages
Total messages: 18 (14 generated)
The CQ bit was checked by tsepez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Assert that dictionary can own the objects it is given. Fix exception in unittest. This broke the previous CL which tried to use unique_ptrs in indirect object holder. ========== to ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. But we don't want to rely on this artifact of destruction order. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ==========
tsepez@chromium.org changed reviewers: + dsinclair@chromium.org
Description was changed from ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. But we don't want to rely on this artifact of destruction order. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ========== to ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release on its subordinates, which are skipped if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ==========
Description was changed from ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release on its subordinates, which are skipped if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ========== to ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release on its subordinates, which are skipped if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ==========
Dan, for review.
Description was changed from ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release on its subordinates, which are skipped if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ========== to ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release on its subordinates, which are skipped if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. Interestingly, CPDF_Array skirts the issue by replacing any indirect objects it is given with references. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ==========
Description was changed from ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release on its subordinates, which are skipped if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. Interestingly, CPDF_Array skirts the issue by replacing any indirect objects it is given with references. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ========== to ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release on its subordinates, which are skipped if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. Interestingly, CPDF_Array skirts the issue by replacing any indirect objects it is given with references. Not clear whether we should do the same thing for dictionaries, or remove it from arrays. The technique certainly complicates understanding ownership. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ==========
lgtm
Description was changed from ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release on its subordinates, which are skipped if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. Interestingly, CPDF_Array skirts the issue by replacing any indirect objects it is given with references. Not clear whether we should do the same thing for dictionaries, or remove it from arrays. The technique certainly complicates understanding ownership. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ========== to ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release on its subordinates, which skips destroying them if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. Interestingly, CPDF_Array skirts the issue by replacing any indirect objects it is given with references. Not clear whether we should do the same thing for dictionaries, or remove it from arrays. The technique certainly complicates understanding ownership. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ==========
Description was changed from ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release on its subordinates, which skips destroying them if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. Interestingly, CPDF_Array skirts the issue by replacing any indirect objects it is given with references. Not clear whether we should do the same thing for dictionaries, or remove it from arrays. The technique certainly complicates understanding ownership. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ========== to ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release() on its subordinates, which skips destroying them if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. Interestingly, CPDF_Array skirts the issue by replacing any indirect objects it is given with references. Not clear whether we should do the same thing for dictionaries, or remove it from arrays. The technique certainly complicates understanding ownership. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tsepez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release() on its subordinates, which skips destroying them if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. Interestingly, CPDF_Array skirts the issue by replacing any indirect objects it is given with references. Not clear whether we should do the same thing for dictionaries, or remove it from arrays. The technique certainly complicates understanding ownership. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. ========== to ========== Assert that dictionary can own the objects it is given. Upon indirect object holder destruction, all indirect objects are destroyed -- currently by order of increasing object number -- but ideally without ordering constraints. So currently, we can get away with a dictionary pointing directly at an indirect object with a higher number. It gets destroyed first, invoking Release() on its subordinates, which skips destroying them if they are indirect objects. But we don't want to rely on this artifact of destruction order. Should it happen to be reversed, the dictionary would invoke Release() on freed memory. Interestingly, CPDF_Array skirts the issue by replacing any indirect objects it is given with references. Not clear whether we should do the same thing for dictionaries, or remove it from arrays. The technique certainly complicates understanding ownership. The one violation found is in the unittest that broke the previous CL which tried to use unique_ptrs in indirect object holder. Committed: https://pdfium.googlesource.com/pdfium/+/393fe4943226846a9b99878406d0bf75f31b... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://pdfium.googlesource.com/pdfium/+/393fe4943226846a9b99878406d0bf75f31b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
