Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1920843002:
Test: deleting RTM before BVR does not leak resources. (Closed)
|
Created:
4 years, 8 months ago by Tobias Sargeant Modified:
4 years, 7 months ago Reviewers:
boliu CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTest: deleting RTM before BVR does not leak resources.
Deleting the RenderThreadManager before BrowserViewRenderer
should not result in any resources being leaked.
BUG=597167
Committed: https://crrev.com/16ee97979621ef51372154264b071c6056a0d1c2
Cr-Commit-Position: refs/heads/master@{#390032}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Implement test. #Patch Set 3 : undo over-zealous git cl format #
Total comments: 26
Patch Set 4 : #
Total comments: 4
Patch Set 5 : address comments #
Total comments: 4
Patch Set 6 : fixes #Patch Set 7 : fix clang compile error #
Messages
Total messages: 22 (8 generated)
tobiasjs@chromium.org changed reviewers: + boliu@chromium.org
Do you think this is a reasonable change?
Description was changed from ========== WIP Test that we can delete RenderThreadManager independently of AwContents without leaking resources. BUG=597167 ========== to ========== WIP Test that we can delete RenderThreadManager independently of AwContents without leaking resources. No new tests at present, but this attempts to separate RenderThreadManager from FakeWindow slightly more, and moves DeleteHardwareRendererOnUI call out of FakeWindow::Detach. It only needs to be balanced by an addition in one test, and it seems like this is the setup that we want, because we don't want to be deleting the hardware renderer on view detach in future. BUG=597167 ==========
Description was changed from ========== WIP Test that we can delete RenderThreadManager independently of AwContents without leaking resources. No new tests at present, but this attempts to separate RenderThreadManager from FakeWindow slightly more, and moves DeleteHardwareRendererOnUI call out of FakeWindow::Detach. It only needs to be balanced by an addition in one test, and it seems like this is the setup that we want, because we don't want to be deleting the hardware renderer on view detach in future. BUG=597167 ========== to ========== WIP Test that we can delete RenderThreadManager independently of AwContents without leaking resources. No new tests at present, but this attempts to separate RenderThreadManager from FakeWindow slightly more, and moves DeleteHardwareRendererOnUI call out of FakeWindow::Detach. It only needs to be balanced by an addition in one test, and it seems like this is the setup that we want, because we don't want to be deleting the hardware renderer on view detach in future. BUG=597167 ==========
Description was changed from ========== WIP Test that we can delete RenderThreadManager independently of AwContents without leaking resources. No new tests at present, but this attempts to separate RenderThreadManager from FakeWindow slightly more, and moves DeleteHardwareRendererOnUI call out of FakeWindow::Detach. It only needs to be balanced by an addition in one test, and it seems like this is the setup that we want, because we don't want to be deleting the hardware renderer on view detach in future. BUG=597167 ========== to ========== WIP Test that we can delete RenderThreadManager independently of BrowserViewRenderer without leaking resources. No new tests at present, but this attempts to separate RenderThreadManager from FakeWindow slightly more, and moves DeleteHardwareRendererOnUI call out of FakeWindow::Detach. It only needs to be balanced by an addition in one test, and it seems like this is the setup that we want, because we don't want to be deleting the hardware renderer on view detach in future. BUG=597167 ==========
lgtm surprising no test needs RTM at the moment. Guess need to expose a getter if tests end up needing it also CL description, first block should be a *single* line, ideally < 72 chars https://codereview.chromium.org/1920843002/diff/1/android_webview/browser/tes... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1920843002/diff/1/android_webview/browser/tes... android_webview/browser/test/fake_window.cc:44: const base::Callback<void(AwDrawGLInfo*)> draw_gl, const& should use an alias so not typing the same signature again and again
On 2016/04/25 15:53:58, boliu wrote: > lgtm > > surprising no test needs RTM at the moment. Guess need to expose a getter if > tests end up needing it > > also CL description, first block should be a *single* line, ideally < 72 chars I know. Wasn't planning on landing it with that description, but I wanted the description to explain what I was planning. I'll roll in the test once I've got it working. > should use an alias so not typing the same signature again and again Will do.
Description was changed from ========== WIP Test that we can delete RenderThreadManager independently of BrowserViewRenderer without leaking resources. No new tests at present, but this attempts to separate RenderThreadManager from FakeWindow slightly more, and moves DeleteHardwareRendererOnUI call out of FakeWindow::Detach. It only needs to be balanced by an addition in one test, and it seems like this is the setup that we want, because we don't want to be deleting the hardware renderer on view detach in future. BUG=597167 ========== to ========== Test: deleting RTM before BVR does not leak resources. Deleting the RenderThreadManager before BrowserViewRenderer should not result in any resources being leaked. BUG=597167 ==========
https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:289: GetReturnedResourceCounts()[last_output_surface_id_]); Is it ok to do this? I suspect that if it fails, the log message won't be as informative as comparing element-wise.
https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:194: typedef std::map<uint32_t, std::map<cc::ResourceId, int>> ResourceCountMap; add some comments, what's the key here again? https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:201: next_frame_ = GetFrame(frame_number_++); maybe factor these 4 lines in to a helper? https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:208: compositor_->SetHardwareFrame(next_frame_->output_surface_id, Hmm, might need to handle the next_frame_ being null case: The UpdateParentDrawConstraints thing calls invalidate too, so might get a trailing call here. Plus generally not safe to assume test is the only thing that calls PostInvalidate, at least not the way the harness is currently written. Hmm... maybe should fix that https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:237: virtual void CheckResults() { EndTest(); } tihs should be = 0 too https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:258: static const int nFrames = int(sizeof(infos) / sizeof(infos[0])); nFrames is java style also base has arraysize: https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&q=ar... Also also.. just use arraysize where needed, don't need to save as static https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:270: expected_return_count_.clear(); and then you need to count the current frame and update last_output_surface_id_ ..old code was fine? https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:289: GetReturnedResourceCounts()[last_output_surface_id_]); On 2016/04/25 17:00:20, Tobias Sargeant wrote: > Is it ok to do this? I suspect that if it fails, the log message won't be as > informative as comparing element-wise. should be fine, previous thing wasn't that useful either https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:295: std::map<cc::ResourceId, int> expected_return_count_; alias this https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:310: frame->frame = ConstructFrame((cc::ResourceId)frame_number); static_cast c-style casts are banned https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:316: resource_counts = GetReturnedResourceCounts(); I don't think you want to assume first frame has been returned by the time CheckResults has run. I don't see any guarantee that's actually true, so it's probably just an accident. https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/test/fake_window.cc:44: const DrawGLCallback draw_gl, const &, still missing the reference part https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... File android_webview/browser/test/fake_window.h (right): https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/test/fake_window.h:45: typedef base::Callback<void(AwDrawGLInfo*)> DrawGLCallback; c++11 style is preferred now I think, using foo = bar; https://codereview.chromium.org/1920843002/diff/40001/android_webview/native/... File android_webview/native/aw_gl_functor.cc (left): https://codereview.chromium.org/1920843002/diff/40001/android_webview/native/... android_webview/native/aw_gl_functor.cc:103: DCHECK_CURRENTLY_ON(BrowserThread::UI); wrong CL..
https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:194: typedef std::map<uint32_t, std::map<cc::ResourceId, int>> ResourceCountMap; On 2016/04/25 19:56:53, boliu wrote: > add some comments, what's the key here again? After adding the aliases, I don't think this is necessary. https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:201: next_frame_ = GetFrame(frame_number_++); On 2016/04/25 19:56:54, boliu wrote: > maybe factor these 4 lines in to a helper? Done. https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:237: virtual void CheckResults() { EndTest(); } On 2016/04/25 19:56:53, boliu wrote: > tihs should be = 0 too Done. https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:258: static const int nFrames = int(sizeof(infos) / sizeof(infos[0])); On 2016/04/25 19:56:53, boliu wrote: > nFrames is java style > > also base has arraysize: > https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&q=ar... > > Also also.. just use arraysize where needed, don't need to save as static Done. I figured there was a define for this somewhere, but I didn't know what it was called. https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:270: expected_return_count_.clear(); On 2016/04/25 19:56:53, boliu wrote: > and then you need to count the current frame > > and update last_output_surface_id_ > > ..old code was fine? Oops, yeah. The old code was fine, but refactoring it to create frames as needed made it look cleaner to me to update this incrementally rather than in StartTest. Could still do it in StartTest, and just loop over infos there. https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:289: GetReturnedResourceCounts()[last_output_surface_id_]); On 2016/04/25 19:56:54, boliu wrote: > On 2016/04/25 17:00:20, Tobias Sargeant wrote: > > Is it ok to do this? I suspect that if it fails, the log message won't be as > > informative as comparing element-wise. > > should be fine, previous thing wasn't that useful either Actually, it gives really good output: C 63.323s Main Value of: GetReturnedResourceCounts()[last_output_surface_id_] C 63.323s Main Actual: { (1, 1), (2, 1), (3, 1), (4, 1) } C 63.323s Main Expected: expected_return_count_ C 63.323s Main Which is: { (1, 2), (2, 2), (3, 2), (4, 1) } https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:295: std::map<cc::ResourceId, int> expected_return_count_; On 2016/04/25 19:56:53, boliu wrote: > alias this Done. https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:310: frame->frame = ConstructFrame((cc::ResourceId)frame_number); On 2016/04/25 19:56:54, boliu wrote: > static_cast > > c-style casts are banned Done. https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:316: resource_counts = GetReturnedResourceCounts(); On 2016/04/25 19:56:53, boliu wrote: > I don't think you want to assume first frame has been returned by the time > CheckResults has run. I don't see any guarantee that's actually true, so it's > probably just an accident. Won't the ReturnResourceFromParent call in OnDrawHardware for the second frame return the first frame's resources? https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/test/fake_window.cc:44: const DrawGLCallback draw_gl, On 2016/04/25 19:56:54, boliu wrote: > const &, still missing the reference part Done. https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... File android_webview/browser/test/fake_window.h (right): https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/test/fake_window.h:45: typedef base::Callback<void(AwDrawGLInfo*)> DrawGLCallback; On 2016/04/25 19:56:54, boliu wrote: > c++11 style is preferred now I think, using foo = bar; Done. I didn't know that this existed. Thanks for pointing it out.
https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1920843002/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:316: resource_counts = GetReturnedResourceCounts(); On 2016/04/26 15:29:51, Tobias Sargeant wrote: > On 2016/04/25 19:56:53, boliu wrote: > > I don't think you want to assume first frame has been returned by the time > > CheckResults has run. I don't see any guarantee that's actually true, so it's > > probably just an accident. > > Won't the ReturnResourceFromParent call in OnDrawHardware for the second frame > return the first frame's resources? Oh definitely not. In OnDrawHardware for from N+1, frame N is still be in use by HR. HR only frees from N when frame N+1 is submitted to cc::surfaces. https://codereview.chromium.org/1920843002/diff/60001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1920843002/diff/60001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:236: bool AdvanceFrame() { private https://codereview.chromium.org/1920843002/diff/60001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:277: ++expected_return_count_[infos[frame_number].resource_id]; can you assume default is 0? wouldnt it be garbage?
https://codereview.chromium.org/1920843002/diff/60001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1920843002/diff/60001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:236: bool AdvanceFrame() { On 2016/04/26 15:47:49, boliu wrote: > private Done. https://codereview.chromium.org/1920843002/diff/60001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:277: ++expected_return_count_[infos[frame_number].resource_id]; On 2016/04/26 15:47:49, boliu wrote: > can you assume default is 0? wouldnt it be garbage? Yep. Value-initialization zero initializes primitive types.
https://codereview.chromium.org/1920843002/diff/80001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1920843002/diff/80001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:289: bool AdvanceFrame() { Wrong class? Does this compile? https://codereview.chromium.org/1920843002/diff/80001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:324: expected[0U][static_cast<cc::ResourceId>(0)] = 1; this is too far away from what it should match, ideally the expected would also be constructed from GetFrame, also improve readability: like untin32_t output_surface_id = 0u, instead of random constants that make no sense on first glance
I think the latest PS is healthier. Stupid error fixed, tests pass locally. https://codereview.chromium.org/1920843002/diff/80001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1920843002/diff/80001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:289: bool AdvanceFrame() { On 2016/04/26 18:04:52, boliu wrote: > Wrong class? Does this compile? Yep, stupid mistake. Sorry for wasting your time. https://codereview.chromium.org/1920843002/diff/80001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:324: expected[0U][static_cast<cc::ResourceId>(0)] = 1; On 2016/04/26 18:04:52, boliu wrote: > this is too far away from what it should match, ideally the expected would also > be constructed from GetFrame, > > also improve readability: like untin32_t output_surface_id = 0u, instead of > random constants that make no sense on first glance Done.
lgtm
The CQ bit was checked by tobiasjs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920843002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920843002/120001
Message was sent while issue was closed.
Description was changed from ========== Test: deleting RTM before BVR does not leak resources. Deleting the RenderThreadManager before BrowserViewRenderer should not result in any resources being leaked. BUG=597167 ========== to ========== Test: deleting RTM before BVR does not leak resources. Deleting the RenderThreadManager before BrowserViewRenderer should not result in any resources being leaked. BUG=597167 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Test: deleting RTM before BVR does not leak resources. Deleting the RenderThreadManager before BrowserViewRenderer should not result in any resources being leaked. BUG=597167 ========== to ========== Test: deleting RTM before BVR does not leak resources. Deleting the RenderThreadManager before BrowserViewRenderer should not result in any resources being leaked. BUG=597167 Committed: https://crrev.com/16ee97979621ef51372154264b071c6056a0d1c2 Cr-Commit-Position: refs/heads/master@{#390032} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/16ee97979621ef51372154264b071c6056a0d1c2 Cr-Commit-Position: refs/heads/master@{#390032} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
