Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902373003:
Fix memory leak in unit test (Closed)
DescriptionFix memory leak CrOSComponentInstallerTest.BPPPCompatibleCrOSComponent unittest
Replace raw pointer with a stack variable.
BUG=chromium:690521, chromium:726579
TEST=passed trybots.
Review-Url: https://codereview.chromium.org/2902373003
Cr-Commit-Position: refs/heads/master@{#474911}
Committed: https://chromium.googlesource.com/chromium/src/+/44c957b81b919e80612133fc5e9d4847252b1258
Patch Set 1 #
Total comments: 2
Patch Set 2 : move bppp to member variable. #
Total comments: 2
Patch Set 3 : address comments from sky #Messages
Total messages: 28 (14 generated)
The CQ bit was checked by xiaochu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xiaochu@chromium.org changed reviewers: + sky@chromium.org, waffles@chromium.org
One-line change to fix memory leak in unittest. Please take a look.
https://codereview.chromium.org/2902373003/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/cros_component_installer_unittest.cc (right): https://codereview.chromium.org/2902373003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer_unittest.cc:90: std::unique_ptr<BrowserProcessPlatformPart> bppp( Can you instead declare BPPP on the stack?
I moved bppp to member variable and add a getBPPP function to CrOSComponentInstallerTest. https://codereview.chromium.org/2902373003/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/cros_component_installer_unittest.cc (right): https://codereview.chromium.org/2902373003/diff/1/chrome/browser/component_up... chrome/browser/component_updater/cros_component_installer_unittest.cc:90: std::unique_ptr<BrowserProcessPlatformPart> bppp( On 2017/05/26 02:27:19, sky wrote: > Can you instead declare BPPP on the stack? Done.
https://codereview.chromium.org/2902373003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/cros_component_installer_unittest.cc (right): https://codereview.chromium.org/2902373003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/cros_component_installer_unittest.cc:96: ASSERT_EQ(GetBPPP()->IsCompatibleCrOSComponent("a"), false); I'm suggesting you do this: BrowserProcessPlatformPart bppp; bppp.AddComp... Why do you need the make_shared above?
Sorry, I mis-understood (call) stack (unique_ptr does not allow me to get...). https://codereview.chromium.org/2902373003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/cros_component_installer_unittest.cc (right): https://codereview.chromium.org/2902373003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/cros_component_installer_unittest.cc:96: ASSERT_EQ(GetBPPP()->IsCompatibleCrOSComponent("a"), false); On 2017/05/26 03:11:12, sky wrote: > I'm suggesting you do this: > > BrowserProcessPlatformPart bppp; > bppp.AddComp... > > Why do you need the make_shared above? Done.
The CQ bit was checked by xiaochu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
Please change BUG= line to 726579 (which is the issue for the memory leak as opposed to the feature). sky: Are you still around? I want to fix this test ASAP. If you don't respond in ~1 hour I will TBR+CQ this because it looks like all of your comments have been addressed and it's easier to land this rather than revert or disable the test.
On 2017/05/26 04:10:01, Matt Giuca wrote: > Please change BUG= line to 726579 (which is the issue for the memory leak as > opposed to the feature). > > sky: Are you still around? I want to fix this test ASAP. If you don't respond in > ~1 hour I will TBR+CQ this because it looks like all of your comments have been > addressed and it's easier to land this rather than revert or disable the test. Also the CL description is now wrong (it isn't a smart pointer, it's a stack variable).
On 2017/05/26 04:10:35, Matt Giuca wrote: > On 2017/05/26 04:10:01, Matt Giuca wrote: > > Please change BUG= line to 726579 (which is the issue for the memory leak as > > opposed to the feature). > > > > sky: Are you still around? I want to fix this test ASAP. If you don't respond > in > > ~1 hour I will TBR+CQ this because it looks like all of your comments have > been > > addressed and it's easier to land this rather than revert or disable the test. > > Also the CL description is now wrong (it isn't a smart pointer, it's a stack > variable). Also please name the test you are fixing in the CL description (CrOSComponentInstallerTest.BPPPCompatibleCrOSComponent). Makes it much easier for sheriffs and other auditing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix memory leak in unit test Replace raw pointer with a smart pointer. BUG=chromium:690521 TEST=passed trybots. ========== to ========== Fix memory leak CrOSComponentInstallerTest.BPPPCompatibleCrOSComponent unittest Replace raw pointer with a stack variable. BUG=chromium:690521, chromium:726579 TEST=passed trybots. ==========
On 2017/05/26 04:11:33, Matt Giuca wrote: > On 2017/05/26 04:10:35, Matt Giuca wrote: > > On 2017/05/26 04:10:01, Matt Giuca wrote: > > > Please change BUG= line to 726579 (which is the issue for the memory leak as > > > opposed to the feature). > > > > > > sky: Are you still around? I want to fix this test ASAP. If you don't > respond > > in > > > ~1 hour I will TBR+CQ this because it looks like all of your comments have > > been > > > addressed and it's easier to land this rather than revert or disable the > test. > > > > Also the CL description is now wrong (it isn't a smart pointer, it's a stack > > variable). > > Also please name the test you are fixing in the CL description > (CrOSComponentInstallerTest.BPPPCompatibleCrOSComponent). Makes it much easier > for sheriffs and other auditing. Done. Thanks!
LGTM
The CQ bit was checked by xiaochu@chromium.org
On 2017/05/26 04:43:29, sky wrote: > LGTM Thanks everyone for dealing with this outside of normal work hours.
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": 40001, "attempt_start_ts": 1495774081050720,
"parent_rev": "0cab0229765caa47ee02c482d768ea65f6e24b5d", "commit_rev":
"44c957b81b919e80612133fc5e9d4847252b1258"}
Message was sent while issue was closed.
Description was changed from ========== Fix memory leak CrOSComponentInstallerTest.BPPPCompatibleCrOSComponent unittest Replace raw pointer with a stack variable. BUG=chromium:690521, chromium:726579 TEST=passed trybots. ========== to ========== Fix memory leak CrOSComponentInstallerTest.BPPPCompatibleCrOSComponent unittest Replace raw pointer with a stack variable. BUG=chromium:690521, chromium:726579 TEST=passed trybots. Review-Url: https://codereview.chromium.org/2902373003 Cr-Commit-Position: refs/heads/master@{#474911} Committed: https://chromium.googlesource.com/chromium/src/+/44c957b81b919e80612133fc5e9d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/44c957b81b919e80612133fc5e9d...
Message was sent while issue was closed.
On 2017/05/26 04:48:37, Matt Giuca wrote: > On 2017/05/26 04:43:29, sky wrote: > > LGTM > > Thanks everyone for dealing with this outside of normal work hours. Sorry for my causing the trouble... Thanks for code review! |
