Destroy window before cleaning up combobox

Destroy window before cleaning up combobox

Currently, when we destroy a CFFL_ComboBox we'll cleanup the fontmap and then
call the destructor for the parent type. This will case the PWL_Wnd to be
destroyed. In this case, the window is a PWL_Edit. On destruction it will reset
the focus which causes the text selection to change, which asks the font map
for data but we've already destroyed the font map.

This CL forces the destruction of the window earlier in order to have the
fontmap available. A followup bug is filed to correct the location of the
fontmap so we don't have this dependency.

BUG=chromium:637546
Committed: https://pdfium.googlesource.com/pdfium/+/28a4a2410f24910c709578d981cae3bb8153fdba

[dsinclair, 2016-08-22 14:39:50]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 14:39:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 14:48:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 14:48:04]

Dry run: Try jobs failed on following builders:
  linux_xfa_rel_gyp on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa_rel_g...)

[dsinclair, 2016-08-22 14:57:47]

dsinclair@chromium.org changed reviewers:
+ thestig@chromium.org, weili@chromium.org

[dsinclair, 2016-08-22 14:57:47]

PTAL

[Wei Li, 2016-08-22 19:26:56]

https://codereview.chromium.org/2266943002/diff/20001/fpdfsdk/formfiller/cffl...
File fpdfsdk/formfiller/cffl_combobox.cpp (right):

https://codereview.chromium.org/2266943002/diff/20001/fpdfsdk/formfiller/cffl...
fpdfsdk/formfiller/cffl_combobox.cpp:33: // until the PWL_Edit is done with it.
pdfium:566
Does this only happen to ComboBox? How about ListBox, TextField and such?

[dsinclair, 2016-08-22 19:32:08]

https://codereview.chromium.org/2266943002/diff/20001/fpdfsdk/formfiller/cffl...
File fpdfsdk/formfiller/cffl_combobox.cpp (right):

https://codereview.chromium.org/2266943002/diff/20001/fpdfsdk/formfiller/cffl...
fpdfsdk/formfiller/cffl_combobox.cpp:33: // until the PWL_Edit is done with it.
pdfium:566
On 2016/08/22 19:26:56, Wei Li wrote:
> Does this only happen to ComboBox? How about ListBox, TextField and such?


It doesn't look like ListBox has this issue as it doesn't do anything with
SetFocus. If the TextField is backed by a PWL_Edit, it's possible it will have
an issue, need to try to construct a test case to find out of there is a
problem. Other widgets, it will depend on if they have a PWL_Edit and what
happens when you call the SetFocus method.

[Wei Li, 2016-08-22 19:44:21]

On 2016/08/22 19:32:08, dsinclair wrote:
>
https://codereview.chromium.org/2266943002/diff/20001/fpdfsdk/formfiller/cffl...
> File fpdfsdk/formfiller/cffl_combobox.cpp (right):
> 
>
https://codereview.chromium.org/2266943002/diff/20001/fpdfsdk/formfiller/cffl...
> fpdfsdk/formfiller/cffl_combobox.cpp:33: // until the PWL_Edit is done with
it.
> pdfium:566
> On 2016/08/22 19:26:56, Wei Li wrote:
> > Does this only happen to ComboBox? How about ListBox, TextField and such?
> 
> 
> It doesn't look like ListBox has this issue as it doesn't do anything with
> SetFocus. If the TextField is backed by a PWL_Edit, it's possible it will have
> an issue, need to try to construct a test case to find out of there is a
> problem. Other widgets, it will depend on if they have a PWL_Edit and what
> happens when you call the SetFocus method.

Looks like TextField needs to be fixed as well. Or add a TODO to check later. 
Would it be better to move the comments to DestroyWindow to warn subclasses
to call it before releasing the font map?

[dsinclair, 2016-08-22 19:56:10]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 19:56:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dsinclair, 2016-08-22 19:56:54]

Patchset #3 (id:40001) has been deleted

[dsinclair, 2016-08-22 19:57:47]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 19:57:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dsinclair, 2016-08-22 19:58:28]

PTAL.

[commit-bot: I haz the power, 2016-08-22 20:22:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 20:22:17]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2016-08-22 20:34:50]

lgtm

[dsinclair, 2016-08-22 20:35:39]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-08-22 20:35:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 20:36:07]

Description was changed from

==========
Destroy window before cleaning up combobox

Currently, when we destroy a CFFL_ComboBox we'll cleanup the fontmap and then
call the destructor for the parent type. This will case the PWL_Wnd to be
destroyed. In this case, the window is a PWL_Edit. On destruction it will reset
the focus which causes the text selection to change, which asks the font map
for data but we've already destroyed the font map.

This CL forces the destruction of the window earlier in order to have the
fontmap available. A followup bug is filed to correct the location of the
fontmap so we don't have this dependency.

BUG=chromium:637546
==========

to

==========
Destroy window before cleaning up combobox

Currently, when we destroy a CFFL_ComboBox we'll cleanup the fontmap and then
call the destructor for the parent type. This will case the PWL_Wnd to be
destroyed. In this case, the window is a PWL_Edit. On destruction it will reset
the focus which causes the text selection to change, which asks the font map
for data but we've already destroyed the font map.

This CL forces the destruction of the window earlier in order to have the
fontmap available. A followup bug is filed to correct the location of the
fontmap so we don't have this dependency.

BUG=chromium:637546

Committed:
https://pdfium.googlesource.com/pdfium/+/28a4a2410f24910c709578d981cae3bb8153...
==========

[commit-bot: I haz the power, 2016-08-22 20:36:08]

Committed patchset #3 (id:60001) as
https://pdfium.googlesource.com/pdfium/+/28a4a2410f24910c709578d981cae3bb8153...