Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1633083002:
Fix memory leakage on Linux - part3 (Closed)
|
Created:
4 years, 11 months ago by Jim Wang Modified:
4 years, 10 months ago CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@xfa Target Ref:
refs/heads/xfa Visibility:
Public. |
DescriptionFix memory leakage on Linux - part3
BUG=pdfium:348
R=jochen@chromium.org, tsepez@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/61dc96f9aa2512807b62cfaec35b1cd012459a6f
Patch Set 1 #
Total comments: 7
Patch Set 2 #Patch Set 3 : #
Total comments: 15
Patch Set 4 : #Messages
Total messages: 18 (3 generated)
jinming_wang@foxitsoftware.com changed reviewers: + jun_fang@foxitsoftware.com
https://codereview.chromium.org/1633083002/diff/1/fpdfsdk/include/jsapi/fxjs_... File fpdfsdk/include/jsapi/fxjs_v8.h (right): https://codereview.chromium.org/1633083002/diff/1/fpdfsdk/include/jsapi/fxjs_... fpdfsdk/include/jsapi/fxjs_v8.h:49: // Global week map to put the dynamic objects. weak? https://codereview.chromium.org/1633083002/diff/1/fpdfsdk/include/jsapi/fxjs_... fpdfsdk/include/jsapi/fxjs_v8.h:49: // Global week map to put the dynamic objects. nit: no 'the' https://codereview.chromium.org/1633083002/diff/1/fpdfsdk/include/jsapi/fxjs_... fpdfsdk/include/jsapi/fxjs_v8.h:109: if (m_pDynamicObjsMap) No need to test |m_pDynamicObjsMap| when we try to delete it. https://codereview.chromium.org/1633083002/diff/1/fpdfsdk/include/jsapi/fxjs_... fpdfsdk/include/jsapi/fxjs_v8.h:116: FXJS_PerIsolateData() {} Should add |m_pDynamicObjsMap(nullptr)|?
https://codereview.chromium.org/1633083002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp File fpdfsdk/src/jsapi/fxjs_v8.cpp (right): https://codereview.chromium.org/1633083002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.c... fpdfsdk/src/jsapi/fxjs_v8.cpp:133: if (id == -1) if (id == -1 || obj.IsEmpty()) https://codereview.chromium.org/1633083002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.c... fpdfsdk/src/jsapi/fxjs_v8.cpp:139: if (!obj.IsEmpty()) { Test obj at line 133. https://codereview.chromium.org/1633083002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.c... fpdfsdk/src/jsapi/fxjs_v8.cpp:148: if (V8TemplateMap* pMap = V8TemplateMap* pMap = FXJS_PerIsolateData::Get(data.GetIsolate())->m_pDynamicObjsMap; return pMap ? &pMap->m_map : nullptr;
jinming_wang@foxitsoftware.com changed reviewers: + jochen@chromium.org, thestig@chromium.org, tsepez@chromium.org
@Tom and Lei, Jochen, please start to review it.
https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/src/jsapi/fxjs_... File fpdfsdk/src/jsapi/fxjs_v8.cpp (right): https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/src/jsapi/fxjs_... fpdfsdk/src/jsapi/fxjs_v8.cpp:348: FXJS_NewFxDynamicObj(pIsolate, pIRuntime, i, TRUE); what's the difference between TRUE and true? can we use the latter? https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/src/jsapi/fxjs_... fpdfsdk/src/jsapi/fxjs_v8.cpp:476: CFXJS_PerObjectData* pPerObjData = new CFXJS_PerObjectData(nObjDefnID); where is this freed? https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/src/jsapi/fxjs_... fpdfsdk/src/jsapi/fxjs_v8.cpp:481: if (!bStatic && FXJS_PerIsolateData::Get(pIsolate)->m_pDynamicObjsMap) add {} around the if body
https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... File fpdfsdk/include/jsapi/fxjs_v8.h (right): https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... fpdfsdk/include/jsapi/fxjs_v8.h:104: V8TemplateMap* m_pDynamicObjsMap; Can |m_pDynamicObjsMap| just be a std::unique_ptr ? https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... fpdfsdk/include/jsapi/fxjs_v8.h:105: void CreateDynamicObjsMap(v8::Isolate* pIsolate) { Please put methods ahead of member variables. https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... fpdfsdk/include/jsapi/fxjs_v8.h:220: FX_BOOL bStatic = FALSE); Prefer no default arguments. https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/src/jsapi/fxjs_... File fpdfsdk/src/jsapi/fxjs_v8.cpp (right): https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/src/jsapi/fxjs_... fpdfsdk/src/jsapi/fxjs_v8.cpp:348: FXJS_NewFxDynamicObj(pIsolate, pIRuntime, i, TRUE); Prefer bool over FX_BOOL.
https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... File fpdfsdk/include/jsapi/fxjs_v8.h (right): https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... fpdfsdk/include/jsapi/fxjs_v8.h:104: V8TemplateMap* m_pDynamicObjsMap; On 2016/01/26 22:18:26, Lei Zhang wrote: > Can |m_pDynamicObjsMap| just be a std::unique_ptr ? When m_pDynamicObjsMap is destroyed, it will visit m_ObjectDefnArray in desconstrution function.If m_pDynamicObjsMap is changed to a unique_ptr, we aren't sure whether m_ObjectDefnArray is destroyed after m_pDynamicObjsMap. If not, it will cause an access violence. https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... fpdfsdk/include/jsapi/fxjs_v8.h:105: void CreateDynamicObjsMap(v8::Isolate* pIsolate) { On 2016/01/26 22:18:26, Lei Zhang wrote: > Please put methods ahead of member variables. Done.
https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... File fpdfsdk/include/jsapi/fxjs_v8.h (right): https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... fpdfsdk/include/jsapi/fxjs_v8.h:220: FX_BOOL bStatic = FALSE); On 2016/01/26 22:18:26, Lei Zhang wrote: > Prefer no default arguments. Acknowledged. https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/src/jsapi/fxjs_... File fpdfsdk/src/jsapi/fxjs_v8.cpp (right): https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/src/jsapi/fxjs_... fpdfsdk/src/jsapi/fxjs_v8.cpp:348: FXJS_NewFxDynamicObj(pIsolate, pIRuntime, i, TRUE); On 2016/01/26 17:04:17, jochen wrote: > what's the difference between TRUE and true? can we use the latter? Done.
https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/src/jsapi/fxjs_... File fpdfsdk/src/jsapi/fxjs_v8.cpp (right): https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/src/jsapi/fxjs_... fpdfsdk/src/jsapi/fxjs_v8.cpp:476: CFXJS_PerObjectData* pPerObjData = new CFXJS_PerObjectData(nObjDefnID); On 2016/01/26 17:04:17, jochen wrote: > where is this freed? FXJS_FreePrivate will free it. https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/src/jsapi/fxjs_... fpdfsdk/src/jsapi/fxjs_v8.cpp:481: if (!bStatic && FXJS_PerIsolateData::Get(pIsolate)->m_pDynamicObjsMap) On 2016/01/26 17:04:17, jochen wrote: > add {} around the if body Done.
lgtm
jochen's LGTM is sufficient approval to land this.
On 2016/01/28 00:13:38, Tom Sepez wrote: > jochen's LGTM is sufficient approval to land this. Patch 4 is uploaded, which fix issues according Lei and jochen's review. I will continue to land this patch, let me know if you have further opinion.
https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... File fpdfsdk/include/jsapi/fxjs_v8.h (right): https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... fpdfsdk/include/jsapi/fxjs_v8.h:220: FX_BOOL bStatic = FALSE); On 2016/01/26 22:18:26, Lei Zhang wrote: > Prefer no default arguments. After a second thought, in this case, i think it's more convenient for the caller with the default argument.
https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... File fpdfsdk/include/jsapi/fxjs_v8.h (right): https://codereview.chromium.org/1633083002/diff/40001/fpdfsdk/include/jsapi/f... fpdfsdk/include/jsapi/fxjs_v8.h:220: FX_BOOL bStatic = FALSE); On 2016/01/28 03:00:13, Jim Wang wrote: > On 2016/01/26 22:18:26, Lei Zhang wrote: > > Prefer no default arguments. > > After a second thought, in this case, i think it's more convenient for the > caller with the default argument. Ok. The Google C++ Style Guide has relaxed the rule for default arguments, and it is a pain to have to change all the callers.
Description was changed from ========== Fix memory leakage on Linux - part3 BUG=pdfium:348 ========== to ========== Fix memory leakage on Linux - part3 BUG=pdfium:348 R=jochen@chromium.org, tsepez@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/61dc96f9aa2512807b62cfaec35b1cd01245... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 61dc96f9aa2512807b62cfaec35b1cd012459a6f (presubmit successful). |
