New issue
Advanced search Search tips

Issue 833590 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

"org.chromium.chrome.browser.profiling_host.ProfilingProcessHostAndroidTest#testModeRendererPseudo" is flaky

Project Member Reported by chromium...@appspot.gserviceaccount.com, Apr 16 2018

Issue description

"org.chromium.chrome.browser.profiling_host.ProfilingProcessHostAndroidTest#testModeRendererPseudo" is flaky.

This issue was created automatically by the chromium-try-flakes app. Please find the right owner to fix the respective test/step and assign this issue to them. If the step/test is infrastructure-related, please add Infra-Troopers label and change issue status to Untriaged. When done, please remove the issue from Sheriff Bug Queue by removing the Sheriff-Chromium label.

We have detected 3 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNybAsSBUZsYWtlImFvcmcuY2hyb21pdW0uY2hyb21lLmJyb3dzZXIucHJvZmlsaW5nX2hvc3QuUHJvZmlsaW5nUHJvY2Vzc0hvc3RBbmRyb2lkVGVzdCN0ZXN0TW9kZVJlbmRlcmVyUHNldWRvDA.

Flaky tests should be disabled within 30 minutes unless culprit CL is found and reverted. Please see more details here: https://sites.google.com/a/chromium.org/dev/developers/tree-sheriffs/sheriffing-bug-queues#triaging-auto-filed-flakiness-bugs

This flaky test/step was previously tracked in  issue 804412 .
 

Comment 1 by moh...@chromium.org, Apr 17 2018

Owner: erikc...@chromium.org
Status: Assigned (was: Untriaged)
erikchen@: Can you please take a look at this...
Thanks, I believe this should be fixed by: https://chromium-review.googlesource.com/c/chromium/src/+/1012542


I'll disable the test while I investigate.
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/419cce880a15bdcec9035da068dc2554a6877e01

commit 419cce880a15bdcec9035da068dc2554a6877e01
Author: Erik Chen <erikchen@chromium.org>
Date: Wed Apr 18 20:34:46 2018

Disable ProfilingProcessHostAndroidTest#testModeRendererPseudo

The test is flaky, likely due to recent refactors to the heap profiling code to
support Android Webview. I'm disabling it temporarily while I investigate.

Bug: 833590
Change-Id: Idd7dd982d9e5586f76f991455d1ae717323c519d
TBR: thakis@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1017598
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551808}
[modify] https://crrev.com/419cce880a15bdcec9035da068dc2554a6877e01/chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/ProfilingProcessHostAndroidTest.java

Labels: -Sheriff-Chromium
 Issue 833998  has been merged into this issue.
On timeouts, there are no logcats. That makes this issue very hard to debug. I'll filed a feature request: https://bugs.chromium.org/p/chromium/issues/detail?id=834903
This issue started with: https://chromium-review.googlesource.com/c/chromium/src/+/1006096

That suggests that this removed code is responsible:
"""
      // On Android, there is a warm-up renderer that is sometimes started
      // before the ProfilingProcessHost can be started. This renderer will
      // therefore not be profiled, even if Chrome is started with the
      // --memlog=all-renderers switch.
      content::BrowserThread::PostTask(
          content::BrowserThread::UI, FROM_HERE,
          base::Bind(&ProfilingProcessHost::StartProfilingRenderersForTesting,
                     base::Unretained(ProfilingProcessHost::GetInstance())));
...

void ProfilingProcessHost::StartProfilingRenderersForTesting() {
  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
  for (auto iter = content::RenderProcessHost::AllHostsIterator();
       !iter.IsAtEnd(); iter.Advance()) {
    StartProfilingRenderer(iter.GetCurrentValue());
  }
}
"""

The current code does this:
"""
  // Start profiling connected renderers.
  for (auto iter = content::RenderProcessHost::AllHostsIterator();
       !iter.IsAtEnd(); iter.Advance()) {
    if (ShouldProfileNewRenderer(iter.GetCurrentValue()) &&
        iter.GetCurrentValue()->GetProcess().Handle() !=
            base::kNullProcessHandle) {
      StartProfilingRenderer(iter.GetCurrentValue());
    }
  }
"""

I wonder if the problem is that the logic which waits for renderers to start up is wrong:
"""
  65 bool RenderersAreBeingProfiled(                                                 
  66     const std::vector<base::ProcessId>& profiled_pids) {                        
  67   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));      
  68   size_t renderer_count = 0;                                                    
  69   for (auto iter = content::RenderProcessHost::AllHostsIterator();              
  70        !iter.IsAtEnd(); iter.Advance()) {                                                                                                                                          
  71     base::ProcessId pid = iter.GetCurrentValue()->GetProcess().Pid();           
  72     if (std::find(profiled_pids.begin(), profiled_pids.end(), pid) ==           
  73         profiled_pids.end()) {                                                  
  74       return false;                                                             
  75     }                                                                           
  76     ++renderer_count;                                                           
  77   }                                                                             
  78                                                                                 
  79   return renderer_count != 0;                                                   
  80 }   
"""

This assumes taht RPHIs are initialized. If there is an RPHI that stays uninitialized... [somehow??], then the test will stall.
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c32009dc704fa611bcc69906f23f1be0d7981e63

commit c32009dc704fa611bcc69906f23f1be0d7981e63
Author: Erik Chen <erikchen@chromium.org>
Date: Thu Apr 19 20:51:44 2018

Tentative fix ProfilingProcessHostAndroidTest#testModeRendererPseudo.

The test started flaking recently due to some refactoring I landed,
specifically: https://chromium-review.googlesource.com/c/chromium/src/+/1006096.

The only theory I have for how this could happen is if there's a
RenderProcessHostImpl that stays stalled in the uninitialized state. This CL
updates the waiting logic in the test to match the logic being tested - to only
look at initialized RenderProcessHostImpls.

Bug: 833590
Change-Id: Ia0d49f5c569625b3ccfc01c9dd17c8e7659f105d
Reviewed-on: https://chromium-review.googlesource.com/1020071
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552150}
[modify] https://crrev.com/c32009dc704fa611bcc69906f23f1be0d7981e63/chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/ProfilingProcessHostAndroidTest.java
[modify] https://crrev.com/c32009dc704fa611bcc69906f23f1be0d7981e63/components/heap_profiling/test_driver.cc

Project Member

Comment 11 by chromium...@appspot.gserviceaccount.com, Apr 20 2018

Labels: Sheriff-Chromium
Detected 4 new flakes for test/step "org.chromium.chrome.browser.profiling_host.ProfilingProcessHostAndroidTest#testModeRendererPseudo". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNybAsSBUZsYWtlImFvcmcuY2hyb21pdW0uY2hyb21lLmJyb3dzZXIucHJvZmlsaW5nX2hvc3QuUHJvZmlsaW5nUHJvY2Vzc0hvc3RBbmRyb2lkVGVzdCN0ZXN0TW9kZVJlbmRlcmVyUHNldWRvDA. This message was posted automatically by the chromium-try-flakes app. Since flakiness is ongoing, the issue was moved back into Sheriff Bug Queue (unless already there).
Bah. Without logcats and without a local repro, I have no idea what could be causing the flakes. I guess I will disable for now.
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/64bb35e6954cc3889005909cfd47d20c7dbf70ea

commit 64bb35e6954cc3889005909cfd47d20c7dbf70ea
Author: Erik Chen <erikchen@chromium.org>
Date: Fri Apr 20 20:05:59 2018

Disable flaky test testModeRendererPseudo.

Bug: 833590
Change-Id: Idde436685e6f5400271c2b850c2740c9ec5c9fb0
TBR: thakis@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1022394
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552442}
[modify] https://crrev.com/64bb35e6954cc3889005909cfd47d20c7dbf70ea/chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/ProfilingProcessHostAndroidTest.java

Comment 14 by jwd@chromium.org, Apr 20 2018

Labels: -Sheriff-Chromium
Ah, there was another CL that landed in the same time period which I suspect might be related:
https://chromium-review.googlesource.com/963405

There's something odd going on with spare renderers and OOP HP. 
Project Member

Comment 16 by bugdroid1@chromium.org, Apr 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7bffeb9aa927652e7b8e2194540c751d4029cf29

commit 7bffeb9aa927652e7b8e2194540c751d4029cf29
Author: Erik Chen <erikchen@chromium.org>
Date: Tue Apr 24 16:56:51 2018

Loosen restriction on profiled renderers for memlog e2e tests.

Previously, we checked that at least 1 renderer exists and that all
renderers are being profiled. This recently started causing the tests to
flake. I suspect this is related to warm-up/spare renderers. So instead,
we check that at least 1 renderer exists and is being profiled.

Bug:  835826 , 833590
Change-Id: I45ef8e1ad9ba1689ae3f0292ab80306b9d7a731f
Reviewed-on: https://chromium-review.googlesource.com/1023716
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553164}
[modify] https://crrev.com/7bffeb9aa927652e7b8e2194540c751d4029cf29/components/heap_profiling/test_driver.cc

Sign in to add a comment