|
|
DescriptionFixup formfiller cleanup
The CFFL_InteractiveFormFiller must be cleaned up before the environment because
the destruction of the formfiller will trigger the destruction of the formfiller
widgets. Some of those widgets may require stopping timers, which requires
accessing the environment.
BUG=chromium:654272, chromium:653459
Committed: https://pdfium.googlesource.com/pdfium/+/709f5a9301e91365ab87610993c497e386504ead
Patch Set 1 #Patch Set 2 : Make sure annotation handler manager is destroyed at the right time. #
Total comments: 6
Patch Set 3 : Comment cleanup #Messages
Total messages: 20 (9 generated)
The CQ bit was checked by dsinclair@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...
dsinclair@chromium.org changed reviewers: + thestig@chromium.org, tsepez@chromium.org
PTAL. This fix is speculative as I can not repro the crash.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ok, LGTM. Let's see how it does.
thestig@ will this conflict with your pending CL https://codereview.chromium.org/2406893003/ ?
Will this fix bug 653459 / bug 653506 as well? I think our CLs are trying to do the same thing...
On 2016/10/11 19:36:06, Lei Zhang wrote: > Will this fix bug 653459 / bug 653506 as well? I think our CLs are trying to do > the same thing... Err, actually, you are trying to destroy |m_pFormFiller| first and I'm trying to destroy it last. Anyway, The bugs I mentioned are reproducible, so do you want to fiddle with those?
On 2016/10/11 19:39:30, Lei Zhang wrote: > On 2016/10/11 19:36:06, Lei Zhang wrote: > > Will this fix bug 653459 / bug 653506 as well? I think our CLs are trying to > do > > the same thing... > > Err, actually, you are trying to destroy |m_pFormFiller| first and I'm trying to > destroy it last. Anyway, The bugs I mentioned are reproducible, so do you want > to fiddle with those? Will try them out, thanks.
Description was changed from ========== Fixup formfiller cleanup The CFFL_InteractiveFormFiller must be cleaned up before the environment because the destruction of the formfiller will trigger the destruction of the formfiller widgets. Some of those widgets may require stopping timers, which requires accessing the environment. BUG=chromium:654272 ========== to ========== Fixup formfiller cleanup The CFFL_InteractiveFormFiller must be cleaned up before the environment because the destruction of the formfiller will trigger the destruction of the formfiller widgets. Some of those widgets may require stopping timers, which requires accessing the environment. BUG=chromium:654272, chromium:653459 ==========
PTAL. Updated to also fix 653459.
LGTM, but please update the comments. https://codereview.chromium.org/2408163003/diff/20001/fpdfsdk/cpdfsdk_formfil... File fpdfsdk/cpdfsdk_formfillenvironment.cpp (right): https://codereview.chromium.org/2408163003/diff/20001/fpdfsdk/cpdfsdk_formfil... fpdfsdk/cpdfsdk_formfillenvironment.cpp:44: // it is cleaned up before the CFFL_InteractiveFormFiller. Refer to |m_pFormFiller| explicitly here? https://codereview.chromium.org/2408163003/diff/20001/fpdfsdk/cpdfsdk_formfil... fpdfsdk/cpdfsdk_formfillenvironment.cpp:47: // Must destroy the |CFFL_InteractiveFormFiller| before the environment |m_pFormFiller| https://codereview.chromium.org/2408163003/diff/20001/fpdfsdk/cpdfsdk_formfil... fpdfsdk/cpdfsdk_formfillenvironment.cpp:49: // Those widgets may call things like |KillTimer| as they are shutdown. KillTimer() and no double space after.
https://codereview.chromium.org/2408163003/diff/20001/fpdfsdk/cpdfsdk_formfil... File fpdfsdk/cpdfsdk_formfillenvironment.cpp (right): https://codereview.chromium.org/2408163003/diff/20001/fpdfsdk/cpdfsdk_formfil... fpdfsdk/cpdfsdk_formfillenvironment.cpp:44: // it is cleaned up before the CFFL_InteractiveFormFiller. On 2016/10/11 20:54:52, Lei Zhang wrote: > Refer to |m_pFormFiller| explicitly here? Done. https://codereview.chromium.org/2408163003/diff/20001/fpdfsdk/cpdfsdk_formfil... fpdfsdk/cpdfsdk_formfillenvironment.cpp:47: // Must destroy the |CFFL_InteractiveFormFiller| before the environment On 2016/10/11 20:54:52, Lei Zhang wrote: > |m_pFormFiller| Done. https://codereview.chromium.org/2408163003/diff/20001/fpdfsdk/cpdfsdk_formfil... fpdfsdk/cpdfsdk_formfillenvironment.cpp:49: // Those widgets may call things like |KillTimer| as they are shutdown. On 2016/10/11 20:54:52, Lei Zhang wrote: > KillTimer() and no double space after. Done.
The CQ bit was checked by dsinclair@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2408163003/#ps40001 (title: "Comment cleanup")
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 ========== Fixup formfiller cleanup The CFFL_InteractiveFormFiller must be cleaned up before the environment because the destruction of the formfiller will trigger the destruction of the formfiller widgets. Some of those widgets may require stopping timers, which requires accessing the environment. BUG=chromium:654272, chromium:653459 ========== to ========== Fixup formfiller cleanup The CFFL_InteractiveFormFiller must be cleaned up before the environment because the destruction of the formfiller will trigger the destruction of the formfiller widgets. Some of those widgets may require stopping timers, which requires accessing the environment. BUG=chromium:654272, chromium:653459 Committed: https://pdfium.googlesource.com/pdfium/+/709f5a9301e91365ab87610993c497e38650... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://pdfium.googlesource.com/pdfium/+/709f5a9301e91365ab87610993c497e38650... |