Reset notify parameter in CPWL_ListBox OnDestroy().

Reset notify parameter in CPWL_ListBox OnDestroy().

Currently the OnDestroy() method of CPWL_ListBox will free
the |m_pListNotify| but it does not update the |m_pList| that
it has done so. This causes issues when CPWL_ListBox is
destroyed as the destructor for |m_pList| will attempt to
call into the |m_pListNotify|.

This CL resets the |m_pList| notify parameter before we
clear |m_pListNotify| in OnDestroy so it will not be
accessed during the |m_pList| destructor.

BUG=628995
Committed: https://pdfium.googlesource.com/pdfium/+/e6a6561b80e73347dd3a7bf660196455f154b573

[dsinclair, 2016-07-18 20:58:19]

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

[dsinclair, 2016-07-18 20:58:20]

PTAL.

[Lei Zhang, 2016-07-18 22:22:07]

https://codereview.chromium.org/2160023002/diff/1/fpdfsdk/pdfwindow/PWL_ListB...
File fpdfsdk/pdfwindow/PWL_ListBox.cpp (right):

https://codereview.chromium.org/2160023002/diff/1/fpdfsdk/pdfwindow/PWL_ListB...
fpdfsdk/pdfwindow/PWL_ListBox.cpp:78: // will call into the m_pListNotify
object, but, m_pListNotify may have
|m_pListNotify|, no comma after "but"

https://codereview.chromium.org/2160023002/diff/1/fpdfsdk/pdfwindow/PWL_ListB...
fpdfsdk/pdfwindow/PWL_ListBox.cpp:80: if (m_pList)
BTW, I think this always evals to true? Maybe for another CL.

https://codereview.chromium.org/2160023002/diff/1/fpdfsdk/pdfwindow/PWL_ListB...
fpdfsdk/pdfwindow/PWL_ListBox.cpp:81: m_pList->SetNotify(nullptr);
Should this be in OnDestroy() below to mirror the SetNotify(ptr) call in
OnCreated() ?

[dsinclair, 2016-07-19 13:13:45]

https://codereview.chromium.org/2160023002/diff/1/fpdfsdk/pdfwindow/PWL_ListB...
File fpdfsdk/pdfwindow/PWL_ListBox.cpp (right):

https://codereview.chromium.org/2160023002/diff/1/fpdfsdk/pdfwindow/PWL_ListB...
fpdfsdk/pdfwindow/PWL_ListBox.cpp:78: // will call into the m_pListNotify
object, but, m_pListNotify may have
On 2016/07/18 22:22:07, Lei Zhang wrote:
> |m_pListNotify|, no comma after "but"

Done.

https://codereview.chromium.org/2160023002/diff/1/fpdfsdk/pdfwindow/PWL_ListB...
fpdfsdk/pdfwindow/PWL_ListBox.cpp:80: if (m_pList)
On 2016/07/18 22:22:07, Lei Zhang wrote:
> BTW, I think this always evals to true? Maybe for another CL.

Possibly, have to check the child classes to see if they set it to nullptr or
not.

https://codereview.chromium.org/2160023002/diff/1/fpdfsdk/pdfwindow/PWL_ListB...
fpdfsdk/pdfwindow/PWL_ListBox.cpp:81: m_pList->SetNotify(nullptr);
On 2016/07/18 22:22:07, Lei Zhang wrote:
> Should this be in OnDestroy() below to mirror the SetNotify(ptr) call in
> OnCreated() ?

Done.

[Lei Zhang, 2016-07-19 17:08:28]

lgtm, CL description can use an update.

[dsinclair, 2016-07-19 17:14:53]

Description was changed from

==========
Reset notify parameter when destroying CPWL_ListBox.

When the list box is destroyed that will trigger the destruction of the m_pList
and m_pListNotify children. The m_pList child holds a pointer to the
m_pListNotify child. When m_pList is destroyed that will trigger a call to
Empty() which clears are the list children, that Empty() also triggers an
invalidation which attemps to call into m_pListNotify. If m_pListNotify was
destroyed first that invalidation will access bad memory.

This Cl explicity clears the notify object from m_pList in the CPWL_ListBox
destructor so the notification will not be sent.

BUG=628995
==========

to

==========
Reset notify parameter in CPWL_ListBox OnDestroy().

Currently the OnDestroy() method of CPWL_ListBox will free
the |m_pListNotify| but it does not update the |m_pList| that
it has done so. This causes issues when CPWL_ListBox is
destroyed as the destructor for |m_pList| will attempt to
call into the |m_pListNotify|.

This CL resets the |m_pList| notify parameter before we
clear |m_pListNotify| in OnDestroy so it will not be
accessed during the |m_pList| destructor.

BUG=628995
==========

[dsinclair, 2016-07-19 17:15:01]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-07-19 17:15:08]

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-07-19 17:15:28]

Description was changed from

==========
Reset notify parameter in CPWL_ListBox OnDestroy().

Currently the OnDestroy() method of CPWL_ListBox will free
the |m_pListNotify| but it does not update the |m_pList| that
it has done so. This causes issues when CPWL_ListBox is
destroyed as the destructor for |m_pList| will attempt to
call into the |m_pListNotify|.

This CL resets the |m_pList| notify parameter before we
clear |m_pListNotify| in OnDestroy so it will not be
accessed during the |m_pList| destructor.

BUG=628995
==========

to

==========
Reset notify parameter in CPWL_ListBox OnDestroy().

Currently the OnDestroy() method of CPWL_ListBox will free
the |m_pListNotify| but it does not update the |m_pList| that
it has done so. This causes issues when CPWL_ListBox is
destroyed as the destructor for |m_pList| will attempt to
call into the |m_pListNotify|.

This CL resets the |m_pList| notify parameter before we
clear |m_pListNotify| in OnDestroy so it will not be
accessed during the |m_pList| destructor.

BUG=628995

Committed:
https://pdfium.googlesource.com/pdfium/+/e6a6561b80e73347dd3a7bf660196455f154...
==========

[commit-bot: I haz the power, 2016-07-19 17:15:29]

Committed patchset #4 (id:60001) as
https://pdfium.googlesource.com/pdfium/+/e6a6561b80e73347dd3a7bf660196455f154...

[commit-bot: I haz the power, 2016-07-19 17:15:31]

CQ bit was unchecked.