Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779713002:
Add a test checking that compositor works in a reused renderer. (Closed)
|
Created:
3 years, 9 months ago by Alexander Semashko Modified:
3 years, 8 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, cc-bugs_chromium.org, James Su, danakj+watch_chromium.org, enne (OOO), piman Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a test checking that compositor works in a reused renderer.
The bug was happening if the renderer submitted a frame right before
navigation commit. The swap ACK got lost, and if this RenderWidget was
reused later, it was drawing nothing because it was waiting for the ACK.
BUG=701988, 705548
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2779713002
Cr-Commit-Position: refs/heads/master@{#465322}
Committed: https://chromium.googlesource.com/chromium/src/+/4ec47030b3d06cb198562b4647094191ba83b06e
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix tests #Patch Set 4 : Reformat test page #
Total comments: 2
Patch Set 5 : Rebase & drop non-test changes. #Patch Set 6 : Use FrameWatcher, rename test. #Patch Set 7 : Don't create a new shell. #
Messages
Total messages: 55 (30 generated)
Description was changed from ========== Prevent compositor from waiting for ack forever. The bug happens when the renderer submits a frame right before navigation commit. The swap ACK gets lost, and if this RenderWidget is reused later, it draws nothing because it still waits for the ACK. BUG=705548 ========== to ========== Prevent compositor from waiting for ack forever. The bug happens when the renderer submits a frame right before navigation commit. The swap ACK gets lost, and if this RenderWidget is reused later, it draws nothing because it still waits for the ACK. BUG=705548 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
ahest@yandex-team.ru changed reviewers: + ccameron@chromium.org
The CQ bit was checked by ahest@yandex-team.ru 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: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ccameron@chromium.org changed reviewers: + fsamuel@chromium.org
This looks good, but I'm not an expert here and I'm worried that there are subtleties that I might miss. fsamuel, could you take a look?
Regarding tryjob failures, I see two problems there:
- on android opening chrome://gpu fails ("unauthorized use of WebUIBindings"),
I'll change the test to open a cross-domain test page.
- all other failures are in RenderWidgetHostViewAuraTest.TwoOutputSurfaces from
content_unittests. It explicitly expects that the ack message is not sent. I'll
just change the expectation. I guess this check was added recently and is not
present in (a slightly dated) revision that I was building this patch on.
fsamuel@chromium.org changed reviewers: + samans@chromium.org
+samans@ for his thoughts. This seems weird to me. This suggests that renderer side state is outliving browser side state. This seems related to this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=701988
The CQ bit was checked by ahest@yandex-team.ru 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...
On 2017/03/28 02:17:07, Fady Samuel wrote: > +samans@ for his thoughts. This seems weird to me. This suggests that renderer > side state is outliving browser side state. This seems related to this bug: > https://bugs.chromium.org/p/chromium/issues/detail?id=701988 Yes, it seem to be the same issue. However, I suppose that it's reasonable to land this CL (unless it makes things more broken than they are) without waiting for the discussion on the bug to settle down.
The CQ bit was checked by ahest@yandex-team.ru 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...
danakj@chromium.org changed reviewers: + danakj@chromium.org, piman@chromium.org
RenderWidget reuse (and the fact that it shouldn't happen) has been coming up a bunch lately. Seems like a bug that was recently introduced and maybe we should go fix that?
Yes, this sounds like the same bug. This implies that RenderWidgetHostView is destroyed when RendererCompositorFrameSink continues to live. I think we should land this CL because it might take a while until we properly address this problem. Thank you for fixing this.
On Tue, Mar 28, 2017 at 11:23 AM, <samans@chromium.org> wrote: > Yes, this sounds like the same bug. This implies that RenderWidgetHostView > is > destroyed when RendererCompositorFrameSink continues to live. I think we > should > land this CL because it might take a while until we properly address this > problem. Thank you for fixing this. > Maybe something to hold onto for branch point, but I am concerned at the idea of patching the symptoms of a deeper problem. It adds code that won't go away once the problem is fixed, adding unneeded complexity. > > https://codereview.chromium.org/2779713002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/28 15:25:34, danakj wrote: > It adds code that won't go away once the problem is fixed, adding unneeded complexity. Why not just revert changes in cc/ and delegated_frame_host.cc when the proper fix is in place? :)
On Tue, Mar 28, 2017 at 11:56 AM, <ahest@yandex-team.ru> wrote: > On 2017/03/28 15:25:34, danakj wrote: > > It adds code that won't go away once the problem is fixed, adding > unneeded > complexity. > > Why not just revert changes in cc/ and delegated_frame_host.cc when the > proper > fix is in place? :) > In my experience that doesn't often happen. Other code comes to rely on it, it's forgetten, or something else happens. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I found the CL that might have started this problem and I'm waiting for a response from its creator to see if there is a better fix that also solves other problems such as memory leaks. Let's see what he says. I think the tests should totally land though to avoid reintroducing the same bug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/03/28 15:58:42, danakj wrote: > In my experience that doesn't often happen. Other code comes to rely on it, > it's forgetten, or something else happens. Hi! I want to clarify that it is reproduced in current Chrome stable...
On 2017/03/28 16:44:09, Lof wrote: > On 2017/03/28 15:58:42, danakj wrote: > > In my experience that doesn't often happen. Other code comes to rely on it, > > it's forgetten, or something else happens. > > Hi! > > I want to clarify that it is reproduced in current Chrome stable... Thanks, crbug.com/701988 should cover this and we should make something to merge to stable there if possible. If not at least to beta.
This problem should be fixed (crrev.com/2786443003), but I think we should land the test.
On Mon, Apr 3, 2017 at 5:29 PM, <samans@chromium.org> wrote: > This problem should be fixed (crrev.com/2786443003), but I think we > should land > the test. > I'm all for a test that would catch that regression. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/03 21:32:11, danakj wrote: > On Mon, Apr 3, 2017 at 5:29 PM, <mailto:samans@chromium.org> wrote: > > > This problem should be fixed (crrev.com/2786443003), but I think we > > should land > > the test. > > > > I'm all for a test that would catch that regression. I agree that the test is useful, but it fails misteriously (i.e. no failures in the log, no crash stack) on android. Is it ok to disable it on android, or should I try to investigate these failures?
danakj@chromium.org changed reviewers: + boliu@chromium.org
On 2017/04/03 21:38:07, Alexander Semashko wrote: > On 2017/04/03 21:32:11, danakj wrote: > > On Mon, Apr 3, 2017 at 5:29 PM, <mailto:samans@chromium.org> wrote: > > > > > This problem should be fixed (crrev.com/2786443003), but I think we > > > should land > > > the test. > > > > > > > I'm all for a test that would catch that regression. > > I agree that the test is useful, but it fails misteriously (i.e. no failures in > the log, no crash stack) on android. Is it ok to disable it on android, or > should I try to investigate these failures? I'm not familiar with what should be expected on android. +boliu for thoughts.
https://codereview.chromium.org/2779713002/diff/40002/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_browsertest.cc (right): https://codereview.chromium.org/2779713002/diff/40002/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_browsertest.cc:209: class SwapCompositorFrameWaiter : public WebContentsObserver { This class is very similar to content::FrameWatcher. Please see if you can use that class to simplify your test. The only difference is that FrameWatcher lacks a timeout, but maybe you can add a timeout to FrameWatcher or just get rid of the timeout and the test will naturally fail if it takes too long.
Android bot is being totally useless here.. +jbudorick Logcat from device still exists, it's just infinitely harder to find than before: https://bugs.chromium.org/p/chromium/issues/detail?id=707387#c3 However the browser test name does not appear at all in logcat, so there's no way to correlate logcat to the test. I think Unknown means the test probably hung or crashed before the test finished, but not sure. I guess either get an android device and run this locally, or if that's not possible, start adding debug logs and start sending try runs to linux_android_rel_ng
On 2017/04/04 16:35:55, boliu wrote: > Android bot is being totally useless here.. +jbudorick > > Logcat from device still exists, it's just infinitely harder to find than > before: https://bugs.chromium.org/p/chromium/issues/detail?id=707387#c3 ack > > However the browser test name does not appear at all in logcat, so there's no > way to correlate logcat to the test. I think Unknown means the test probably > hung or crashed before the test finished, but not sure. > > I guess either get an android device and run this locally, or if that's not > possible, start adding debug logs and start sending try runs to > linux_android_rel_ng The test is timing out. Device(06085d6d006230af) 04-04 16:07:54.609 28611 28628 I chromium: Device(06085d6d006230af) 04-04 16:08:17.629 28493 28497 D dalvikvm: GC_CONCURRENT freed 347K, 3% free 16744K/17124K, paused 4ms+0ms, total 28ms Device(06085d6d006230af) 04-04 16:08:49.869 28493 28497 D dalvikvm: GC_CONCURRENT freed 391K, 3% free 16744K/17168K, paused 7ms+1ms, total 21ms Device(06085d6d006230af) 04-04 16:08:59.599 28508 28511 D dalvikvm: GC_CONCURRENT freed 364K, 3% free 17178K/17576K, paused 8ms+0ms, total 24ms Device(06085d6d006230af) 04-04 16:09:22.119 28493 28497 D dalvikvm: GC_CONCURRENT freed 391K, 3% free 16744K/17168K, paused 6ms+0ms, total 26ms Device(06085d6d006230af) 04-04 16:09:54.360 28493 28493 E cr_NativeTest: Test process 28508 timed out. Device(06085d6d006230af) 04-04 16:09:54.360 28493 28493 I Process : Sending signal. PID: 28508 SIG: 9 Device(06085d6d006230af) 04-04 16:09:54.379 840 24783 I ActivityManager: Process org.chromium.content_browsertests_apk:test_process (pid 28508) has died.
The CQ bit was checked by ahest@yandex-team.ru 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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ahest@yandex-team.ru 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.
There is now only the test, and it passes on android too! samans (and whoever is interested), PTAL https://codereview.chromium.org/2779713002/diff/40002/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_browsertest.cc (right): https://codereview.chromium.org/2779713002/diff/40002/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_browsertest.cc:209: class SwapCompositorFrameWaiter : public WebContentsObserver { On 2017/04/03 21:51:30, Saman Sami wrote: > This class is very similar to content::FrameWatcher. Please see if you can use > that class to simplify your test. The only difference is that FrameWatcher lacks > a timeout, but maybe you can add a timeout to FrameWatcher or just get rid of > the timeout and the test will naturally fail if it takes too long. Done. Adding timeout to FrameWatcher is not worth it, I think.
Description was changed from ========== Prevent compositor from waiting for ack forever. The bug happens when the renderer submits a frame right before navigation commit. The swap ACK gets lost, and if this RenderWidget is reused later, it draws nothing because it still waits for the ACK. BUG=705548 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add a test checking that compositor works in a reused renderer. The bug was happening if the renderer submitted a frame right before navigation commit. The swap ACK got lost, and if this RenderWidget was reused later, it was drawing nothing because it still waits for the ACK. BUG=701988,705548 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Add a test checking that compositor works in a reused renderer. The bug was happening if the renderer submitted a frame right before navigation commit. The swap ACK got lost, and if this RenderWidget was reused later, it was drawing nothing because it still waits for the ACK. BUG=701988,705548 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add a test checking that compositor works in a reused renderer. The bug was happening if the renderer submitted a frame right before navigation commit. The swap ACK got lost, and if this RenderWidget was reused later, it was drawing nothing because it was waiting for the ACK. BUG=701988,705548 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
LGTM based on my limited knowledge of content. You also need a content owner to review your CL. Thank you for working on this.
danakj@chromium.org changed reviewers: - danakj@chromium.org
renderer_host rs lgtm
The CQ bit was checked by ahest@yandex-team.ru
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": 110001, "attempt_start_ts": 1492539117885990,
"parent_rev": "166e5eede3378cc08466dae28da82845393ee2a6", "commit_rev":
"4ec47030b3d06cb198562b4647094191ba83b06e"}
Message was sent while issue was closed.
Description was changed from ========== Add a test checking that compositor works in a reused renderer. The bug was happening if the renderer submitted a frame right before navigation commit. The swap ACK got lost, and if this RenderWidget was reused later, it was drawing nothing because it was waiting for the ACK. BUG=701988,705548 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add a test checking that compositor works in a reused renderer. The bug was happening if the renderer submitted a frame right before navigation commit. The swap ACK got lost, and if this RenderWidget was reused later, it was drawing nothing because it was waiting for the ACK. BUG=701988,705548 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2779713002 Cr-Commit-Position: refs/heads/master@{#465322} Committed: https://chromium.googlesource.com/chromium/src/+/4ec47030b3d06cb198562b464709... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001) as https://chromium.googlesource.com/chromium/src/+/4ec47030b3d06cb198562b464709... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
