Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557173002:
Remove CFX_FormatString::Release() (Closed)
DescriptionRemove CFX_FormatString::Release()
Avoid the |delete this| anti-pattern.
Remove some checks which don't avoid other segvs anyways.
Committed: https://pdfium.googlesource.com/pdfium/+/248cb27e64b3a25230f53fc2f4ab9d483facc5f9
Patch Set 1 #
Total comments: 11
Messages
Total messages: 12 (4 generated)
tsepez@chromium.org changed reviewers: + dsinclair@chromium.org
Dan, ready for review. https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... File xfa/fxfa/parser/xfa_localevalue.cpp (left): https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... xfa/fxfa/parser/xfa_localevalue.cpp:106: if (m_pLocaleMgr) segv at 110 unless this block executes. https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... xfa/fxfa/parser/xfa_localevalue.cpp:469: if (m_pLocaleMgr) segv at 473 unless this block executes. https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... xfa/fxfa/parser/xfa_localevalue.cpp:496: if (m_pLocaleMgr) segv at 534 unless this block executes https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... xfa/fxfa/parser/xfa_localevalue.cpp:808: pFormat = new CFX_FormatString(m_pLocaleMgr, false); segv at 811, 881 unless this block executes
https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... File xfa/fxfa/parser/xfa_localevalue.cpp (left): https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... xfa/fxfa/parser/xfa_localevalue.cpp:106: if (m_pLocaleMgr) On 2016/12/07 22:56:13, Tom Sepez wrote: > segv at 110 unless this block executes. Do we know if m_pLocalMgr can be null? Should we be early returning or something if it is? Does CFX_FormatString handle the nullptr? https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... xfa/fxfa/parser/xfa_localevalue.cpp:469: if (m_pLocaleMgr) On 2016/12/07 22:56:13, Tom Sepez wrote: > segv at 473 unless this block executes. ditto https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... xfa/fxfa/parser/xfa_localevalue.cpp:496: if (m_pLocaleMgr) On 2016/12/07 22:56:13, Tom Sepez wrote: > segv at 534 unless this block executes " https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... xfa/fxfa/parser/xfa_localevalue.cpp:808: pFormat = new CFX_FormatString(m_pLocaleMgr, false); On 2016/12/07 22:56:13, Tom Sepez wrote: > segv at 811, 881 unless this block executes "
https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... File xfa/fxfa/parser/xfa_localevalue.cpp (left): https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... xfa/fxfa/parser/xfa_localevalue.cpp:106: if (m_pLocaleMgr) On 2016/12/07 23:17:43, dsinclair wrote: > On 2016/12/07 22:56:13, Tom Sepez wrote: > > segv at 110 unless this block executes. > > Do we know if m_pLocalMgr can be null? Should we be early returning or something > if it is? Does CFX_FormatString handle the nullptr? Hope not. Should we early return ? Not unless we get crashes. The format string method called here doesn't use this.
lgtm w/ question https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... File xfa/fxfa/parser/xfa_localevalue.cpp (left): https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... xfa/fxfa/parser/xfa_localevalue.cpp:106: if (m_pLocaleMgr) On 2016/12/07 23:36:36, Tom Sepez wrote: > On 2016/12/07 23:17:43, dsinclair wrote: > > On 2016/12/07 22:56:13, Tom Sepez wrote: > > > segv at 110 unless this block executes. > > > > Do we know if m_pLocalMgr can be null? Should we be early returning or > something > > if it is? Does CFX_FormatString handle the nullptr? > > Hope not. Should we early return ? Not unless we get crashes. The format > string method called here doesn't use this. If CFX_FormatString doesn't need it, can we just remove the param from the constructor?
https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... File xfa/fxfa/parser/xfa_localevalue.cpp (left): https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... xfa/fxfa/parser/xfa_localevalue.cpp:106: if (m_pLocaleMgr) On 2016/12/07 23:36:36, Tom Sepez wrote: > On 2016/12/07 23:17:43, dsinclair wrote: > > On 2016/12/07 22:56:13, Tom Sepez wrote: > > > segv at 110 unless this block executes. > > > > Do we know if m_pLocalMgr can be null? Should we be early returning or > something > > if it is? Does CFX_FormatString handle the nullptr? > > Hope not. Should we early return ? Not unless we get crashes. The format > string method called here doesn't use this. Also segv at 187, even if we manage to successfully run SplitFormatString with a null |this| (doesn't use other memembers).
On 2016/12/07 23:45:20, dsinclair wrote: > lgtm w/ question > > https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... > File xfa/fxfa/parser/xfa_localevalue.cpp (left): > > https://codereview.chromium.org/2557173002/diff/1/xfa/fxfa/parser/xfa_localev... > xfa/fxfa/parser/xfa_localevalue.cpp:106: if (m_pLocaleMgr) > On 2016/12/07 23:36:36, Tom Sepez wrote: > > On 2016/12/07 23:17:43, dsinclair wrote: > > > On 2016/12/07 22:56:13, Tom Sepez wrote: > > > > segv at 110 unless this block executes. > > > > > > Do we know if m_pLocalMgr can be null? Should we be early returning or > > something > > > if it is? Does CFX_FormatString handle the nullptr? > > > > Hope not. Should we early return ? Not unless we get crashes. The format > > string method called here doesn't use this. > > > If CFX_FormatString doesn't need it, can we just remove the param from the > constructor? Sorry, it may use it elsewhere -- just not in the SplitFormatString() immediately called.
The CQ bit was checked by tsepez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1481154609705720, "parent_rev":
"a9d29df6a774737a661d0f37f6b8aa5cba179c06", "commit_rev":
"248cb27e64b3a25230f53fc2f4ab9d483facc5f9"}
Message was sent while issue was closed.
Description was changed from ========== Remove CFX_FormatString::Release() Avoid the |delete this| anti-pattern. Remove some checks which don't avoid other segvs anyways. ========== to ========== Remove CFX_FormatString::Release() Avoid the |delete this| anti-pattern. Remove some checks which don't avoid other segvs anyways. Committed: https://pdfium.googlesource.com/pdfium/+/248cb27e64b3a25230f53fc2f4ab9d483fac... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://pdfium.googlesource.com/pdfium/+/248cb27e64b3a25230f53fc2f4ab9d483fac... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
