Do not check pointers before deleting them.

Do not check pointers before deleting them.

XFA edition.

Committed: https://pdfium.googlesource.com/pdfium/+/495bda110a6ea8e7a6fc313ec0232a9ca6e3cfdc

[Lei Zhang, 2016-04-28 21:28:03]

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

[Lei Zhang, 2016-04-28 21:28:04]

There's some goto removal too. BC_PDF417ECErrorCorrection.cpp can use some
unique_ptrs, but I'm too lazy right now.

[Tom Sepez, 2016-04-28 21:47:23]

https://codereview.chromium.org/1925363002/diff/1/xfa/fwl/theme/cfwl_checkbox...
File xfa/fwl/theme/cfwl_checkboxtp.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fwl/theme/cfwl_checkbox...
xfa/fwl/theme/cfwl_checkboxtp.cpp:35: m_pCheckPath = nullptr;
nit: probably not necessary.

https://codereview.chromium.org/1925363002/diff/1/xfa/fxbarcode/BC_BufferedIm...
File xfa/fxbarcode/BC_BufferedImageLuminanceSource.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxbarcode/BC_BufferedIm...
xfa/fxbarcode/BC_BufferedImageLuminanceSource.cpp:59: if (status ==
FXCODEC_STATUS_ERR_PARAMS ||
Doesn't the second part cover the first part?

https://codereview.chromium.org/1925363002/diff/1/xfa/fxbarcode/pdf417/BC_PDF...
File xfa/fxbarcode/pdf417/BC_PDF417Detector.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxbarcode/pdf417/BC_PDF...
xfa/fxbarcode/pdf417/BC_PDF417Detector.cpp:117: delete (vertices);
() ? Also below.

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/app/xfa_ffpushbutt...
File xfa/fxfa/app/xfa_ffpushbutton.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/app/xfa_ffpushbutt...
xfa/fxfa/app/xfa_ffpushbutton.cpp:92: m_pRolloverTextLayout = NULL;
nullptr.

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/parser/xfa_script_...
File xfa/fxfa/parser/xfa_script_imp.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/parser/xfa_script_...
xfa/fxfa/parser/xfa_script_imp.cpp:515: CXFA_Object* lpCurNode =
(CXFA_Object*)FXJSE_Value_ToObject(hObject, NULL);
Local not needed

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/parser/xfa_script_...
xfa/fxfa/parser/xfa_script_imp.cpp:516: delete lpCurNode;
virtual dtor, I presume?

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/parser/xfa_script_...
xfa/fxfa/parser/xfa_script_imp.cpp:517: lpCurNode = nullptr;
Not needed, right?

https://codereview.chromium.org/1925363002/diff/1/xfa/fxgraphics/cagg_graphic...
File xfa/fxgraphics/cagg_graphics.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxgraphics/cagg_graphic...
xfa/fxgraphics/cagg_graphics.cpp:29: delete m_owner->m_renderDevice;
virtual dtor?

https://codereview.chromium.org/1925363002/diff/1/xfa/fxjse/context.cpp
File xfa/fxjse/context.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxjse/context.cpp#newco...
xfa/fxjse/context.cpp:23: delete hContext;
No, there's a brutal reinterpret cast going on here, the various hTypes are
usually pointers to struct { void* data; } that are blatantly cast to the actual
underlying types.

[Lei Zhang, 2016-04-28 22:31:11]

https://codereview.chromium.org/1925363002/diff/1/xfa/fwl/theme/cfwl_checkbox...
File xfa/fwl/theme/cfwl_checkboxtp.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fwl/theme/cfwl_checkbox...
xfa/fwl/theme/cfwl_checkboxtp.cpp:35: m_pCheckPath = nullptr;
On 2016/04/28 21:47:22, Tom Sepez wrote:
> nit: probably not necessary.

Done.

https://codereview.chromium.org/1925363002/diff/1/xfa/fxbarcode/BC_BufferedIm...
File xfa/fxbarcode/BC_BufferedImageLuminanceSource.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxbarcode/BC_BufferedIm...
xfa/fxbarcode/BC_BufferedImageLuminanceSource.cpp:59: if (status ==
FXCODEC_STATUS_ERR_PARAMS ||
On 2016/04/28 21:47:22, Tom Sepez wrote:
> Doesn't the second part cover the first part?

Done.

https://codereview.chromium.org/1925363002/diff/1/xfa/fxbarcode/pdf417/BC_PDF...
File xfa/fxbarcode/pdf417/BC_PDF417Detector.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxbarcode/pdf417/BC_PDF...
xfa/fxbarcode/pdf417/BC_PDF417Detector.cpp:117: delete (vertices);
On 2016/04/28 21:47:22, Tom Sepez wrote:
> () ? Also below.

Done.

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/app/xfa_ffpushbutt...
File xfa/fxfa/app/xfa_ffpushbutton.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/app/xfa_ffpushbutt...
xfa/fxfa/app/xfa_ffpushbutton.cpp:92: m_pRolloverTextLayout = NULL;
On 2016/04/28 21:47:22, Tom Sepez wrote:
> nullptr.

Done.

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/parser/xfa_script_...
File xfa/fxfa/parser/xfa_script_imp.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/parser/xfa_script_...
xfa/fxfa/parser/xfa_script_imp.cpp:515: CXFA_Object* lpCurNode =
(CXFA_Object*)FXJSE_Value_ToObject(hObject, NULL);
On 2016/04/28 21:47:22, Tom Sepez wrote:
> Local not needed

Done.

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/parser/xfa_script_...
xfa/fxfa/parser/xfa_script_imp.cpp:516: delete lpCurNode;
On 2016/04/28 21:47:22, Tom Sepez wrote:
> virtual dtor, I presume?

Sadly, no.

https://codereview.chromium.org/1925363002/diff/1/xfa/fxfa/parser/xfa_script_...
xfa/fxfa/parser/xfa_script_imp.cpp:517: lpCurNode = nullptr;
On 2016/04/28 21:47:22, Tom Sepez wrote:
> Not needed, right?

Done.

https://codereview.chromium.org/1925363002/diff/1/xfa/fxgraphics/cagg_graphic...
File xfa/fxgraphics/cagg_graphics.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxgraphics/cagg_graphic...
xfa/fxgraphics/cagg_graphics.cpp:29: delete m_owner->m_renderDevice;
On 2016/04/28 21:47:22, Tom Sepez wrote:
> virtual dtor?

Yes.

https://codereview.chromium.org/1925363002/diff/1/xfa/fxjse/context.cpp
File xfa/fxjse/context.cpp (right):

https://codereview.chromium.org/1925363002/diff/1/xfa/fxjse/context.cpp#newco...
xfa/fxjse/context.cpp:23: delete hContext;
On 2016/04/28 21:47:23, Tom Sepez wrote:
> No, there's a brutal reinterpret cast going on here, the various hTypes are
> usually pointers to struct { void* data; } that are blatantly cast to the
actual
> underlying types.

Done.

[Tom Sepez, 2016-04-28 23:07:55]

https://codereview.chromium.org/1925363002/diff/40001/xfa/fxfa/parser/xfa_scr...
File xfa/fxfa/parser/xfa_script_imp.cpp (right):

https://codereview.chromium.org/1925363002/diff/40001/xfa/fxfa/parser/xfa_scr...
xfa/fxfa/parser/xfa_script_imp.cpp:518: delete
reinterpret_cast<CXFA_ThisProxy*>(
static_cast not good enough (void return?)  Also why CXFA_ThisProxy* and not
CXFA_Object* like before?

https://codereview.chromium.org/1925363002/diff/40001/xfa/fxjse/context.cpp
File xfa/fxjse/context.cpp (right):

https://codereview.chromium.org/1925363002/diff/40001/xfa/fxjse/context.cpp#n...
xfa/fxjse/context.cpp:16: CFXJSE_Context* CFXContextFromFXContext(FXJSE_HCONTEXT
hContext) {
nit: can we call this CFXContextFromHContext(), too many fx's

[Lei Zhang, 2016-04-28 23:43:53]

I'll wait for your CLs to land and resolve the conflicts.

https://codereview.chromium.org/1925363002/diff/40001/xfa/fxfa/parser/xfa_scr...
File xfa/fxfa/parser/xfa_script_imp.cpp (right):

https://codereview.chromium.org/1925363002/diff/40001/xfa/fxfa/parser/xfa_scr...
xfa/fxfa/parser/xfa_script_imp.cpp:518: delete
reinterpret_cast<CXFA_ThisProxy*>(
On 2016/04/28 23:07:55, Tom Sepez wrote:
> static_cast not good enough (void return?)  Also why CXFA_ThisProxy* and not
> CXFA_Object* like before?

static_cast is good enough. The object gets casted twice in the original code.

https://codereview.chromium.org/1925363002/diff/40001/xfa/fxjse/context.cpp
File xfa/fxjse/context.cpp (right):

https://codereview.chromium.org/1925363002/diff/40001/xfa/fxjse/context.cpp#n...
xfa/fxjse/context.cpp:16: CFXJSE_Context* CFXContextFromFXContext(FXJSE_HCONTEXT
hContext) {
On 2016/04/28 23:07:55, Tom Sepez wrote:
> nit: can we call this CFXContextFromHContext(), too many fx's

Done.

[Tom Sepez, 2016-04-28 23:47:40]

lgtm

https://codereview.chromium.org/1925363002/diff/40001/xfa/fxfa/parser/xfa_scr...
File xfa/fxfa/parser/xfa_script_imp.cpp (right):

https://codereview.chromium.org/1925363002/diff/40001/xfa/fxfa/parser/xfa_scr...
xfa/fxfa/parser/xfa_script_imp.cpp:518: delete
reinterpret_cast<CXFA_ThisProxy*>(
On 2016/04/28 23:43:53, Lei Zhang wrote:
> On 2016/04/28 23:07:55, Tom Sepez wrote:
> > static_cast not good enough (void return?)  Also why CXFA_ThisProxy* and not
> > CXFA_Object* like before?
> 
> static_cast is good enough. The object gets casted twice in the original code.

Acknowledged.

[Lei Zhang, 2016-04-29 00:08:22]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-04-29 00:08:23]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1925363002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-29 00:08:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1925363002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1925363002/80001

[commit-bot: I haz the power, 2016-04-29 00:29:25]

Description was changed from

==========
Do not check pointers before deleting them.

XFA edition.
==========

to

==========
Do not check pointers before deleting them.

XFA edition.

Committed:
https://pdfium.googlesource.com/pdfium/+/495bda110a6ea8e7a6fc313ec0232a9ca6e3...
==========

[commit-bot: I haz the power, 2016-04-29 00:29:26]

Committed patchset #5 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/495bda110a6ea8e7a6fc313ec0232a9ca6e3...