Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2301613002:
Fix leaks due to created popup annotations (Closed)
DescriptionFix leaks due to created popup annotations
When we create popup annotations, we also create the dictionary
associated with it. For regular annotations, the dictionary
associated with an annotation is not owned by annotation,
and will be released separately. But our created dictionary is not
associated with any other data structure, it would be leaked if not
released by the associated annotation.
Add a boolean to indicate the ownership to the dictionary, and release
the owned dictionary during the destruction of an annotation.
BUG=pdfium:242
Committed: https://pdfium.googlesource.com/pdfium/+/7c5d090719a25f0c1b81fb6b46544b9394a7fdd2
Patch Set 1 #
Total comments: 5
Patch Set 2 : fix typo #
Messages
Total messages: 28 (18 generated)
The CQ bit was checked by weili@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
weili@chromium.org changed reviewers: + dsinclair@chromium.org, thestig@chromium.org
pls review, thanks
thestig@chromium.org changed reviewers: + jaepark@google.com
https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/cpdf_annotlist... File core/fpdfdoc/cpdf_annotlist.cpp (right): https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/cpdf_annotlist... core/fpdfdoc/cpdf_annotlist.cpp:77: pDict = pAnnots->GetDictAt(i); Could we clone the dictionary at this point and pass a std::unique_ptr into CPDF_Annot constructor so it's always owned? Do we have any idea how big this dictionary can be?
On 2016/08/31 20:28:31, dsinclair wrote: > https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/cpdf_annotlist... > File core/fpdfdoc/cpdf_annotlist.cpp (right): > > https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/cpdf_annotlist... > core/fpdfdoc/cpdf_annotlist.cpp:77: pDict = pAnnots->GetDictAt(i); > Could we clone the dictionary at this point and pass a std::unique_ptr into > CPDF_Annot constructor so it's always owned? > > Do we have any idea how big this dictionary can be? I don't have statistics on this. But it depends on some string-typed fields such as its name, contents, as well as appearance stream. I would avoid to copy them if at all possible.
https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/cpdf_annotlist... File core/fpdfdoc/cpdf_annotlist.cpp (right): https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/cpdf_annotlist... core/fpdfdoc/cpdf_annotlist.cpp:31: CPDF_Dictionary* pAnnotDict = new CPDF_Dictionary; Alternatively, we can have some other container to hold these objects, and tie the lifetime of the container to the lifetime of |pPopupAnnot|. That way we won't have to modify CPDF_Annot at all. https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/include/cpdf_a... File core/fpdfdoc/include/cpdf_annot.h (right): https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/include/cpdf_a... core/fpdfdoc/include/cpdf_annot.h:105: // our artificaially created popup annotations, |m_pAnnotDict| typo
The CQ bit was checked by weili@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/win/builds/1909)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by weili@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...
https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/cpdf_annotlist... File core/fpdfdoc/cpdf_annotlist.cpp (right): https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/cpdf_annotlist... core/fpdfdoc/cpdf_annotlist.cpp:31: CPDF_Dictionary* pAnnotDict = new CPDF_Dictionary; On 2016/09/01 03:53:12, Lei Zhang wrote: > Alternatively, we can have some other container to hold these objects, and tie > the lifetime of the container to the lifetime of |pPopupAnnot|. That way we > won't have to modify CPDF_Annot at all. I know the current solution is not ideal. But maintaining a separate container as well as the mapping between <popupAnnot, popupAnnotDict> is too much and confusing. I tried to use a vector in CPDF_AnnotList to store the dict list. But the use and release an individual Annotation is all over the place. :( https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/include/cpdf_a... File core/fpdfdoc/include/cpdf_annot.h (right): https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/include/cpdf_a... core/fpdfdoc/include/cpdf_annot.h:105: // our artificaially created popup annotations, |m_pAnnotDict| On 2016/09/01 03:53:12, Lei Zhang wrote: > typo Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/01 19:08:34, Wei Li wrote: > https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/cpdf_annotlist... > File core/fpdfdoc/cpdf_annotlist.cpp (right): > > https://codereview.chromium.org/2301613002/diff/1/core/fpdfdoc/cpdf_annotlist... > core/fpdfdoc/cpdf_annotlist.cpp:31: CPDF_Dictionary* pAnnotDict = new > CPDF_Dictionary; > On 2016/09/01 03:53:12, Lei Zhang wrote: > > Alternatively, we can have some other container to hold these objects, and tie > > the lifetime of the container to the lifetime of |pPopupAnnot|. That way we > > won't have to modify CPDF_Annot at all. > > I know the current solution is not ideal. But maintaining a separate container > as well as the mapping between <popupAnnot, popupAnnotDict> is too much and > confusing. I tried to use a vector in CPDF_AnnotList to store the dict list. But > the use and release an individual Annotation is all over the place. :( Sure. It was just a hand-wavy idea. I'm fine with this too.
The CQ bit was checked by weili@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 ========== Fix leaks due to created popup annotations When we create popup annotations, we also create the dictionary associated with it. For regular annotations, the dictionary associated with an annotation is not owned by annotation, and will be released separately. But our created dictionary is not associated with any other data structure, it would be leaked if not released by the associated annotation. Add a boolean to indicate the ownership to the dictionary, and release the owned dictionary during the destruction of an annotation. BUG=pdfium:242 ========== to ========== Fix leaks due to created popup annotations When we create popup annotations, we also create the dictionary associated with it. For regular annotations, the dictionary associated with an annotation is not owned by annotation, and will be released separately. But our created dictionary is not associated with any other data structure, it would be leaked if not released by the associated annotation. Add a boolean to indicate the ownership to the dictionary, and release the owned dictionary during the destruction of an annotation. BUG=pdfium:242 Committed: https://pdfium.googlesource.com/pdfium/+/7c5d090719a25f0c1b81fb6b46544b9394a7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://pdfium.googlesource.com/pdfium/+/7c5d090719a25f0c1b81fb6b46544b9394a7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
