Watch for destruction of widget during callback

Watch for destruction of CPDFSDK_widget during callback

Speculative fix since I had trouble with the repro.

BUG=632709
Committed: https://pdfium.googlesource.com/pdfium/+/ca97a8ee99cad1ddb6a69e14cc7a501b3e8092a4

[Tom Sepez, 2016-07-29 21:53:31]

Description was changed from

==========
Watch for destruction of widget during callback

BUG=632709
==========

to

==========
Watch for destruction of CPDFSDK_widget during callback

Speculative fix since I had trouble with the repro.

BUG=632709
==========

[Tom Sepez, 2016-07-29 21:53:31]

tsepez@chromium.org changed reviewers:
+ thestig@chromium.org

[Tom Sepez, 2016-07-29 21:53:55]

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

[commit-bot: I haz the power, 2016-07-29 21:54:06]

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

[Tom Sepez, 2016-07-29 21:55:01]

Lei, something for a friday afternoon, Thanks.

[commit-bot: I haz the power, 2016-07-29 22:09:19]

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

[commit-bot: I haz the power, 2016-07-29 22:09:20]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-07-29 22:30:06]

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/fsdk_baseform.cpp
File fpdfsdk/fsdk_baseform.cpp (right):

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/fsdk_baseform.cpp#n...
fpdfsdk/fsdk_baseform.cpp:62: if (m_pWatchedPtr) {
Is this always true, or can OnWidgetDestroyed() get called multiple times?

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/fsdk_baseform.cpp#n...
fpdfsdk/fsdk_baseform.cpp:89: ASSERT(!pdfium::ContainsKey(m_Observers,
pObserver));
You can also just try inserting, and then ASSERT the return result to make sure
it was actually inserted. Ditto for erase() below.

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/include/fsdk_basefo...
File fpdfsdk/include/fsdk_baseform.h (right):

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/include/fsdk_basefo...
fpdfsdk/include/fsdk_baseform.h:66: Observer(CPDFSDK_Widget** pWatchedPtr);
explicit

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/include/fsdk_basefo...
fpdfsdk/include/fsdk_baseform.h:68: void OnWidgetDestroyed();
new line before

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/javascript/Field.cpp
File fpdfsdk/javascript/Field.cpp (right):

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/javascript/Field.cp...
fpdfsdk/javascript/Field.cpp:233: if (pWidget)
multiline body -> braces

[Tom Sepez, 2016-08-01 16:24:54]

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/fsdk_baseform.cpp
File fpdfsdk/fsdk_baseform.cpp (right):

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/fsdk_baseform.cpp#n...
fpdfsdk/fsdk_baseform.cpp:62: if (m_pWatchedPtr) {
On 2016/07/29 22:30:06, Lei Zhang wrote:
> Is this always true, or can OnWidgetDestroyed() get called multiple times?

Shouldn't happen. Will change to assert.

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/fsdk_baseform.cpp#n...
fpdfsdk/fsdk_baseform.cpp:89: ASSERT(!pdfium::ContainsKey(m_Observers,
pObserver));
On 2016/07/29 22:30:05, Lei Zhang wrote:
> You can also just try inserting, and then ASSERT the return result to make
sure
> it was actually inserted. Ditto for erase() below.

Probably leads to unused variable warnings when asserts not enabled.

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/include/fsdk_basefo...
File fpdfsdk/include/fsdk_baseform.h (right):

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/include/fsdk_basefo...
fpdfsdk/include/fsdk_baseform.h:66: Observer(CPDFSDK_Widget** pWatchedPtr);
On 2016/07/29 22:30:06, Lei Zhang wrote:
> explicit

Done.

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/include/fsdk_basefo...
fpdfsdk/include/fsdk_baseform.h:68: void OnWidgetDestroyed();
On 2016/07/29 22:30:06, Lei Zhang wrote:
> new line before

Done.

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/javascript/Field.cpp
File fpdfsdk/javascript/Field.cpp (right):

https://codereview.chromium.org/2197793002/diff/1/fpdfsdk/javascript/Field.cp...
fpdfsdk/javascript/Field.cpp:233: if (pWidget)
On 2016/07/29 22:30:06, Lei Zhang wrote:
> multiline body -> braces

Done.

[Tom Sepez, 2016-08-01 16:25:02]

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

[commit-bot: I haz the power, 2016-08-01 16:25:07]

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-01 16:37:38]

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

[commit-bot: I haz the power, 2016-08-01 16:37:39]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-08-01 17:03:23]

lgtm

[Tom Sepez, 2016-08-01 17:10:14]

The CQ bit was checked by tsepez@chromium.org

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

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-01 17:10:40]

Description was changed from

==========
Watch for destruction of CPDFSDK_widget during callback

Speculative fix since I had trouble with the repro.

BUG=632709
==========

to

==========
Watch for destruction of CPDFSDK_widget during callback

Speculative fix since I had trouble with the repro.

BUG=632709

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

[commit-bot: I haz the power, 2016-08-01 17:10:41]

Committed patchset #2 (id:20001) as
https://pdfium.googlesource.com/pdfium/+/ca97a8ee99cad1ddb6a69e14cc7a501b3e80...