Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
939673002:
Test that visual state callbacks do not deadlock. (Closed)
|
Created:
5 years, 10 months ago by Ignacio Solla Modified:
5 years, 10 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@breakSwapIfNoUpdates Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTest that visual state callbacks do not deadlock.
Visual state callbacks should always be received,
even if there are no pending commits. We add
a test for that. See crbug/458577 for more details.
BUG=458577
Committed: https://crrev.com/dcc18cb569b31bc07c97d49573e695ec8b1f80c0
Cr-Commit-Position: refs/heads/master@{#317306}
Patch Set 1 #Patch Set 2 : #
Total comments: 11
Patch Set 3 : Increase timeout #Patch Set 4 : Address review comments #
Total comments: 2
Patch Set 5 : Add iddle check #Patch Set 6 : Rebase #Messages
Total messages: 25 (5 generated)
igsolla@chromium.org changed reviewers: + piman@chromium.org, sievers@chromium.org, skyostil@chromium.org
https://codereview.chromium.org/939673002/diff/20001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/939673002/diff/20001/content/content_tests.gy... content/content_tests.gypi:1594: }, { # OS!="android" Did you mean to change this and the other whitespace bits? https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... File content/renderer/visual_state_browsertest.cc (right): https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... content/renderer/visual_state_browsertest.cc:21: const int kRenderViewRoutingId = 2; Is there a way to query this, anyone? This seems a little fragile. https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... content/renderer/visual_state_browsertest.cc:58: command_line->AppendSwitch(switches::kSingleProcess); Do we really need to enforce this?
https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... File content/renderer/visual_state_browsertest.cc (right): https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... content/renderer/visual_state_browsertest.cc:29: const base::TimeDelta kCommitTimeout = base::TimeDelta::FromMilliseconds(150); Please no timeout. This is a sure way to make this test flaky (think valgrind). https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... content/renderer/visual_state_browsertest.cc:58: command_line->AppendSwitch(switches::kSingleProcess); On 2015/02/18 16:12:22, Sami wrote: > Do we really need to enforce this? Indeed, I don't think we want to run tests in that way. Single-process has a bunch of things that are different from prod, we should test what we ship. https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... content/renderer/visual_state_browsertest.cc:79: // but we know this will be the first RV so skip that and just hardcode it. please no. One day something will change in how the routing IDs are assigned and this will break.
https://codereview.chromium.org/939673002/diff/20001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/939673002/diff/20001/content/content_tests.gy... content/content_tests.gypi:1594: }, { # OS!="android" On 2015/02/18 16:12:22, Sami wrote: > Did you mean to change this and the other whitespace bits? it wasn't me, it was either git format patch or the clank style for eclipse, and they are always right :) https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... File content/renderer/visual_state_browsertest.cc (right): https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... content/renderer/visual_state_browsertest.cc:21: const int kRenderViewRoutingId = 2; On 2015/02/18 16:12:22, Sami wrote: > Is there a way to query this, anyone? This seems a little fragile. Hardcoding it seems to be an extended practice in the codebase: https://code.google.com/p/chromium/codesearch#search/&q=kRenderViewRoutingId&... https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... content/renderer/visual_state_browsertest.cc:58: command_line->AppendSwitch(switches::kSingleProcess); On 2015/02/18 16:12:22, Sami wrote: > Do we really need to enforce this? Yes, to be able to use PostTaskToInProcessRendererAndWait.
Note that I may need to increase the timeout until these tests start passing on the bots. 150ms was enough when I ran them locally, but it seems that we need a higher timeout on the bots.
https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... File content/renderer/visual_state_browsertest.cc (right): https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... content/renderer/visual_state_browsertest.cc:29: const base::TimeDelta kCommitTimeout = base::TimeDelta::FromMilliseconds(150); On 2015/02/18 16:37:18, piman (Very slow to review) wrote: > Please no timeout. This is a sure way to make this test flaky (think valgrind). Without the timeout this test is useless. If the test is flaky then we need to increase the timeout until it passes. https://codereview.chromium.org/939673002/diff/20001/content/renderer/visual_... content/renderer/visual_state_browsertest.cc:79: // but we know this will be the first RV so skip that and just hardcode it. On 2015/02/18 16:37:18, piman (Very slow to review) wrote: > please no. > One day something will change in how the routing IDs are assigned and this will > break. Hardcoding it seems to be an extended practice: https://code.google.com/p/chromium/codesearch#search/&q=kRenderViewRoutingId&... Could I keep this and then file a bug for somebody to fix all of the occurrences? (I'm leaving so I will not be able to fix it myself :()
On Wed, Feb 18, 2015 at 12:01 PM, <igsolla@chromium.org> wrote: > > https://codereview.chromium.org/939673002/diff/20001/ > content/renderer/visual_state_browsertest.cc > File content/renderer/visual_state_browsertest.cc (right): > > https://codereview.chromium.org/939673002/diff/20001/ > content/renderer/visual_state_browsertest.cc#newcode29 > content/renderer/visual_state_browsertest.cc:29: const base::TimeDelta > kCommitTimeout = base::TimeDelta::FromMilliseconds(150); > On 2015/02/18 16:37:18, piman (Very slow to review) wrote: > >> Please no timeout. This is a sure way to make this test flaky (think >> > valgrind). > > Without the timeout this test is useless. If the test is flaky then we > need to increase the timeout until it passes. A test can be written that inspects internal state rather than relying on timeouts. Increasing the timeout increases bot cycle time. If every other engineer does that we'll end up in a bad place. > https://codereview.chromium.org/939673002/diff/20001/ > content/renderer/visual_state_browsertest.cc#newcode79 > content/renderer/visual_state_browsertest.cc:79: // but we know this > will be the first RV so skip that and just hardcode it. > On 2015/02/18 16:37:18, piman (Very slow to review) wrote: > >> please no. >> One day something will change in how the routing IDs are assigned and >> > this will > >> break. >> > > Hardcoding it seems to be an extended practice: > https://code.google.com/p/chromium/codesearch#search/&q= > kRenderViewRoutingId&sq=package:chromium&type=cs And as e.g. https://chromium.googlesource.com/chromium/src/+/94d0cc1e283b81a8eca337d41128... shows, it has broken before, and will break again. Could I keep this and then file a bug for somebody to fix all of the > occurrences? (I'm leaving so I will not be able to fix it myself :() > I didn't review the other code. I'm reviewing this one. Please don't do that in new code. > > https://codereview.chromium.org/939673002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> A test can be written that inspects internal state rather than relying on > timeouts. > Increasing the timeout increases bot cycle time. If every other engineer > does that we'll end up in a bad place. How can we detect the point at which no further commits are required after loading a static page? I can't see any way to do that, but please let me know if there is one. For now I'm relying on the fact that loading "about:blank" only requires a single commit. The timeout was there to allow us to detect if this ever changes. I have removed it now, so if loading "about:blank" ever requires two commits then this test will prove nothing and we will not notice. > And as e.g. > https://chromium.googlesource.com/chromium/src/+/94d0cc1e283b81a8eca337d41128... > shows, it has broken before, and will break again. Fair enough. > I didn't review the other code. I'm reviewing this one. Not sure what this adds to the discussion. This is not about you, or me or anyone. > Please don't do that in new code. Fair enough. I have changed it now. I have to admit that I was expecting it to be more work. Given how simple getting the routing id is you're right, the tests should not be hardcoding it.
https://codereview.chromium.org/939673002/diff/60001/content/renderer/visual_... File content/renderer/visual_state_browsertest.cc (right): https://codereview.chromium.org/939673002/diff/60001/content/renderer/visual_... content/renderer/visual_state_browsertest.cc:52: EXPECT_EQ(1, observer.GetCommitCount()); I think if you add a DCHECK For MessageLoop::IsIdleForTesting here you should have some protection against unknown, possibly commit-inducing work getting queued up in the future.
https://codereview.chromium.org/939673002/diff/60001/content/renderer/visual_... File content/renderer/visual_state_browsertest.cc (right): https://codereview.chromium.org/939673002/diff/60001/content/renderer/visual_... content/renderer/visual_state_browsertest.cc:52: EXPECT_EQ(1, observer.GetCommitCount()); On 2015/02/19 10:59:34, Sami wrote: > I think if you add a DCHECK For MessageLoop::IsIdleForTesting here you should > have some protection against unknown, possibly commit-inducing work getting > queued up in the future. Done.
On 2015/02/19 11:53:54, Ignacio Solla wrote: > https://codereview.chromium.org/939673002/diff/60001/content/renderer/visual_... > File content/renderer/visual_state_browsertest.cc (right): > > https://codereview.chromium.org/939673002/diff/60001/content/renderer/visual_... > content/renderer/visual_state_browsertest.cc:52: EXPECT_EQ(1, > observer.GetCommitCount()); > On 2015/02/19 10:59:34, Sami wrote: > > I think if you add a DCHECK For MessageLoop::IsIdleForTesting here you should > > have some protection against unknown, possibly commit-inducing work getting > > queued up in the future. > > Done. Note that this test depends on https://codereview.chromium.org/925353002/ for passing, so we need to land that first.
On Thu, Feb 19, 2015 at 5:43 AM, <igsolla@chromium.org> wrote: > > A test can be written that inspects internal state rather than relying on >> timeouts. >> Increasing the timeout increases bot cycle time. If every other engineer >> does that we'll end up in a bad place. >> > > How can we detect the point at which no further commits are required after > loading a static page? I can't see any way to do that, but please let me > know > if there is one. > > For now I'm relying on the fact that loading "about:blank" only requires a > single > commit. The timeout was there to allow us to detect if this ever changes. > I have > removed it now, so if loading "about:blank" ever requires two commits then > this > test will prove nothing and we will not notice. What do you mean by "load requires a single commit" exactly? What state are you trying to test? Do you mean, after all resources are loaded in the renderer, there's exactly one commit and no more? What could cause another commit? cc::Scheduler state? A message? A posted task? Can you test that? > > > > And as e.g. >> > > https://chromium.googlesource.com/chromium/src/+/ > 94d0cc1e283b81a8eca337d411289fcef9ef4780%5E%21/content/ > renderer/dom_serializer_browsertest.cc > >> shows, it has broken before, and will break again. >> > > Fair enough. > > > > I didn't review the other code. I'm reviewing this one. >> > > Not sure what this adds to the discussion. This is not about you, or me or > anyone. > > > Please don't do that in new code. >> > > Fair enough. I have changed it now. I have to admit that I was expecting > it to > be more work. Given how simple getting the routing id is you're right, the > tests > should not be hardcoding it. > > https://codereview.chromium.org/939673002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM, but please file a bug to explore: 1- how to correctly assert that no other commit should happen 2- how to test this without --single-process
> What do you mean by "load requires a single commit" exactly? 1) That no matter how long you wait for in this test only a single commit will occur after loading "about:blank" in the fresh shell if you don't do anything else. > What state are you trying to test? I'm trying to test a state in which there are no pending commits not any pending commit-inducing work. The VisualStateCallback is commonly delivered with pending commits. When there are none we still want to ensure that it is delivered and for that we request an additional commit as we cannot invoke the callback immediately due to crbug/458550. > Do you mean, after all resources are loaded in the renderer, there's > exactly one commit and no more? No, I mean 1) above. In the general case for static pages I think that it is true that after all the resources have been loaded and in the absence of interactions with the page no further commits will occur. But I don't know how many commits it takes so this contrived example is simpler and it serves the same purpose. > What could cause another commit? I don't know, I'm not a compositor/blink expert. If I have to guess, I'd say that loading "about:blank" in a fresh view should never take more than one commit since there is nothing to be updated or loaded. > cc::Scheduler state? A message? A posted task? > Can you test that? We don't want another commit in this test.
On 2015/02/19 23:37:12, piman (Very slow to review) wrote: > LGTM, but please file a bug to explore: > 1- how to correctly assert that no other commit should happen Filed crbug/460429 for this. > 2- how to test this without --single-process Filed crbug/460435 for this but please note that the WebView runs single process and there are no other users of the visual state API, so I'm not sure how useful this other test would be.
New patchsets have been uploaded after l-g-t-m from piman@chromium.org
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/939673002/100001
The CQ bit was unchecked by igsolla@chromium.org
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/939673002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/dcc18cb569b31bc07c97d49573e695ec8b1f80c0 Cr-Commit-Position: refs/heads/master@{#317306}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/942973002/ by kbr@chromium.org. The reason for reverting is: Timing out on Blink waterfall. See https://code.google.com/p/chromium/issues/detail?id=458577#c10 . . |
