New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 622793 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

SiteInstanceTests failing under Dr. Memory

Project Member Reported by reillyg@chromium.org, Jun 23 2016

Issue description

Cc: nick@chromium.org
All failures appear to be:

c:\b\build\slave\drm-cr\build\src\content\browser\site_instance_impl_unittest.cc(115): error: Value of: RenderProcessHost::AllHostsIterator().IsAtEnd()
Actual: false
Expected: true

Comment 2 by creis@chromium.org, Jun 23 2016

Cc: creis@chromium.org
Components: Internals>Core
That's concerning.  These aren't SiteIsolation tests-- these have been around since Chrome launched.

There's probably a CL in build #5150 that caused them to regress.

Tests:
SiteInstanceTest.CloneNavigationEntry
SiteInstanceTest.DefaultSubframeSiteInstance
SiteInstanceTest.GetProcess
SiteInstanceTest.GetSiteForURL
SiteInstanceTest.HasWrongProcessForURL
SiteInstanceTest.HasWrongProcessForURLInSitePerProcess
SiteInstanceTest.IsSameWebSite
SiteInstanceTest.NoProcessPerSiteForEmptySite
SiteInstanceTest.OneSiteInstancePerSite
SiteInstanceTest.OneSiteInstancePerSiteInBrowserContext
SiteInstanceTest.ProcessPerSiteWithWrongBindings
SiteInstanceTest.ProcessSharingByType
SiteInstanceTest.SetSite
SiteInstanceTest.SiteInstanceDestructor

Comment 3 by creis@chromium.org, Jun 23 2016

Labels: Needs-Bisect
That call is in SiteInstanceTest::TearDown, which explains why they're all failing the same way.  Something must be causing a RenderProcessHost to stick around after the test is over.

Could someone run a bisect?  I'm going to be out for a bit.
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 23 2016

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

commit 8e6872f8f8b89bb49bda37577c7e0800ebee2488
Author: Reilly Grant <reillyg@chromium.org>
Date: Thu Jun 23 18:46:00 2016

Disable SiteInstanceTest.* under Dr. Memory.

They are failing.

BUG= 622793 
TBR=nick@chromium.org

Review URL: https://codereview.chromium.org/2090393002 .

Cr-Commit-Position: refs/heads/master@{#401667}

[modify] https://crrev.com/8e6872f8f8b89bb49bda37577c7e0800ebee2488/tools/valgrind/gtest_exclude/content_unittests.gtest-drmemory_win32.txt

Comment 5 by creis@chromium.org, Jun 23 2016

Disabling these tests isn't ok.  There's a CL that should be reverted if it's causing these tests to fail.

Is there a way to run a bisect on Dr. Memory so we can find out which CL caused this?
These tests are only disabled under Dr. Memory, which is standard procedure when a Dr. Memory-only issue is found. I am working on a bisect now.

Comment 7 by creis@chromium.org, Jun 23 2016

Ok, thanks.  Much appreciated!
thestig@ reminded me of something I forgot about which is that the biggest difference between the normal bots and the Dr. Memory bots is that the tests don't run in parallel so one test leaving behind some state can mess up another test hundreds of tests later. Hopefully this should speed up the process of bisecting this.
Owner: ekaramad@chromium.org
Status: Assigned (was: Untriaged)
After running a bisect I found https://codereview.chromium.org/2045363002/ to be at fault. Reverting.
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 24 2016

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

commit 7225ddf9fd7c32a1de91a9f2c8b755d736b2fd0d
Author: reillyg <reillyg@chromium.org>
Date: Fri Jun 24 01:25:01 2016

Revert of Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only) (patchset #8 id:140001 of https://codereview.chromium.org/2045363002/ )

Reason for revert:
This patch added unit tests that leave behind state that will break other tests if content_unittests is run with --single-process-tests (as it is on the Dr. Memory bots).

Original issue's description:
> Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only)
>
> Currently, the IME result calls on ui::TextInputClient (RenderWidgetHostViewAura) are only
> routed to the RenderWidgetHostImpl corresponding to the tab's RenderWidgetHostView. Therefore,
> the IME related IPC arrive at the main frame's RenderWidget only. In the OOPIF structure,
> the IME result might be intended to a child frame's RenderWidget.
>
> This patch will route all such IME calls to the RenderWidgetHost which currently has the
> active text input state.
>
> The patch also adds four unit tests to RenderWidgetHostViewAuraTests to verify that the
> routing is done properly.
>
> BUG= 578168 
>
> Committed: https://crrev.com/d33fd7859cac797067a2b8a8e55f6dbac59bee19
> Cr-Commit-Position: refs/heads/master@{#401465}

TBR=kenrb@chromium.org,creis@chromium.org,ekaramad@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 578168 , 622793 

Review-Url: https://codereview.chromium.org/2095813002
Cr-Commit-Position: refs/heads/master@{#401771}

[modify] https://crrev.com/7225ddf9fd7c32a1de91a9f2c8b755d736b2fd0d/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/7225ddf9fd7c32a1de91a9f2c8b755d736b2fd0d/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/7225ddf9fd7c32a1de91a9f2c8b755d736b2fd0d/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/7225ddf9fd7c32a1de91a9f2c8b755d736b2fd0d/content/browser/renderer_host/text_input_manager.cc
[modify] https://crrev.com/7225ddf9fd7c32a1de91a9f2c8b755d736b2fd0d/content/browser/renderer_host/text_input_manager.h
[modify] https://crrev.com/7225ddf9fd7c32a1de91a9f2c8b755d736b2fd0d/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/7225ddf9fd7c32a1de91a9f2c8b755d736b2fd0d/content/test/test_render_view_host.h

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 24 2016

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

commit 31189d3191c823005b7d09f980953195ccf66765
Author: reillyg <reillyg@chromium.org>
Date: Fri Jun 24 01:26:53 2016

Revert of Disable SiteInstanceTest.* under Dr. Memory. (patchset #1 id:1 of https://codereview.chromium.org/2090393002/ )

Reason for revert:
Reverted https://codereview.chromium.org/2045363002/.

Original issue's description:
> Disable SiteInstanceTest.* under Dr. Memory.
>
> They are failing.
>
> BUG= 622793 
> TBR=nick@chromium.org
>
> Committed: https://chromium.googlesource.com/chromium/src/+/8e6872f8f8b89bb49bda37577c7e0800ebee2488

TBR=nick@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 622793 

Review-Url: https://codereview.chromium.org/2099573002
Cr-Commit-Position: refs/heads/master@{#401772}

[modify] https://crrev.com/31189d3191c823005b7d09f980953195ccf66765/tools/valgrind/gtest_exclude/content_unittests.gtest-drmemory_win32.txt

Project Member

Comment 12 by bugdroid1@chromium.org, Jun 24 2016

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

commit 8cba7886c3e170ca2dc324dbbd31b28dae623887
Author: ekaramad <ekaramad@chromium.org>
Date: Fri Jun 24 19:42:31 2016

[reland] Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only)

This is a reland of the original CL: https://codereview.chromium.org/2045363002/ which was
reverted by this CL https://codereview.chromium.org/2095813002. The reason for the revert
was the regression of several SiteInstanceTests on Dr Memory bots.

The cause of the issue was that the newly introduced content unit tests in the original CL
were not deleting a RenderWidgetHostImpl which was keeping a RenderProcessHost alive. This
was interfering with the SiteInstanceTests which have explicit asserts on the number of
RenderProceessHosts alive.

BUG= 578168 , 622793 

Review-Url: https://codereview.chromium.org/2092103002
Cr-Commit-Position: refs/heads/master@{#401939}

[modify] https://crrev.com/8cba7886c3e170ca2dc324dbbd31b28dae623887/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/8cba7886c3e170ca2dc324dbbd31b28dae623887/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/8cba7886c3e170ca2dc324dbbd31b28dae623887/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/8cba7886c3e170ca2dc324dbbd31b28dae623887/content/browser/renderer_host/text_input_manager.cc
[modify] https://crrev.com/8cba7886c3e170ca2dc324dbbd31b28dae623887/content/browser/renderer_host/text_input_manager.h
[modify] https://crrev.com/8cba7886c3e170ca2dc324dbbd31b28dae623887/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/8cba7886c3e170ca2dc324dbbd31b28dae623887/content/test/test_render_view_host.h

Status: Fixed (was: Assigned)
Marking bug as fixed since the re-landed patch has not regressed the tests in almost a week.

Sign in to add a comment