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

Issue 753218 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 750894

Blocking:
issue 751321



Sign in to add a comment

Add tests for out-of-process heap profiling

Project Member Reported by erikc...@chromium.org, Aug 8 2017

Issue description

1) First, add them to fyi waterfall. 
2) Then, after security review, add them to main waterfall. https://bugs.chromium.org/p/chromium/issues/detail?id=751759

vms for fyi waterfall: https://bugs.chromium.org/p/chromium/issues/detail?id=750894
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 10 2017

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

commit fc133c14a3b79b21592a81255e8765d9f3f27ff1
Author: erikchen <erikchen@chromium.org>
Date: Thu Aug 10 01:13:09 2017

Add configuration for out-of-heap profiling tests on fyi waterfall.

The test configuration was mirrored from Clang fyi test configurations, copied
from the respective platforms. Each also has the additional
oop_heap_profiling_unit_tests target.

Bug:  753218 
Change-Id: Ia661832a0ba8bb2ccf11427cb8cbd286247cd993
Reviewed-on: https://chromium-review.googlesource.com/605049
Reviewed-by: Albert J. Wong <ajwong@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493208}
[modify] https://crrev.com/fc133c14a3b79b21592a81255e8765d9f3f27ff1/BUILD.gn
[modify] https://crrev.com/fc133c14a3b79b21592a81255e8765d9f3f27ff1/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/fc133c14a3b79b21592a81255e8765d9f3f27ff1/testing/buildbot/gn_isolate_map.pyl
[modify] https://crrev.com/fc133c14a3b79b21592a81255e8765d9f3f27ff1/tools/mb/mb_config.pyl

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 12 2017

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

commit d74b2558cb70f16234d9b1782b7b63157f0648c9
Author: erikchen <erikchen@chromium.org>
Date: Sat Aug 12 01:56:07 2017

Remove references to non-existent test target chrome/profiling:unit_tests.

chrome/profiling:unit_tests is a source set included by
//chrome/test:unit_tests.

Bug:  753218 
Change-Id: I6b1a6795fe1374dec9792e85d32698ddd5dc8ae0
Reviewed-on: https://chromium-review.googlesource.com/612658
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493932}
[modify] https://crrev.com/d74b2558cb70f16234d9b1782b7b63157f0648c9/BUILD.gn
[modify] https://crrev.com/d74b2558cb70f16234d9b1782b7b63157f0648c9/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/d74b2558cb70f16234d9b1782b7b63157f0648c9/testing/buildbot/gn_isolate_map.pyl

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/6644ac7b5dc4dac9b5bb2972bf46629f71b47f2b

commit 6644ac7b5dc4dac9b5bb2972bf46629f71b47f2b
Author: erikchen <erikchen@chromium.org>
Date: Sat Aug 12 01:57:42 2017

Serialize tests for out of process profiling to reduce load.

Bug:753218, 750894
Change-Id: I6850c70793b42533dddb85a2c0360f0170caf10e
Reviewed-on: https://chromium-review.googlesource.com/611726
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>

[modify] https://crrev.com/6644ac7b5dc4dac9b5bb2972bf46629f71b47f2b/scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py

Cc: dskiba@chromium.org etienneb@chromium.org ssid@chromium.org erikc...@chromium.org
 Issue 751332  has been merged into this issue.
Blockedon: 750894
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/2f05e8525021d1d24d50cc485132e314921e41fb

commit 2f05e8525021d1d24d50cc485132e314921e41fb
Author: erikchen <erikchen@chromium.org>
Date: Wed Aug 16 00:39:20 2017

Schedule tasks for out-of-process heap-profiling fyi waterfall bots.

Bug:  753218 
Change-Id: Ia1c2c9e3341eeed81398b07166db3c2f766c7722
Reviewed-on: https://chromium-review.googlesource.com/616241
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>

[modify] https://crrev.com/2f05e8525021d1d24d50cc485132e314921e41fb/masters/master.chromium.fyi/master.cfg

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 17 2017

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

commit 65b750580f2f1054a2f9fadfe83e4bf651a14d18
Author: erikchen <erikchen@chromium.org>
Date: Thu Aug 17 18:44:16 2017

Add android specific out-of-process heap-profiling mb config.

Bug:  753218 
Change-Id: I82e44135f70cd91885c97a146a94208d56592be3
Reviewed-on: https://chromium-review.googlesource.com/619314
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495248}
[modify] https://crrev.com/65b750580f2f1054a2f9fadfe83e4bf651a14d18/tools/mb/mb_config.pyl

Owner: brettw@chromium.org
Status: Assigned (was: Untriaged)
We're starting to get a pretty good suite of unit tests. Over to brettw for end-to-end tests.
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 1 2017

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

commit 69cf4732e6fc941622eb70c84869f35b52ef8281
Author: Brett Wilson <brettw@chromium.org>
Date: Fri Sep 01 01:34:40 2017

memlog: Basic end-to-end test framework.

This set up test stubs for all 3 major modes of memlog. Currently it
only tests that the system doesn't crash, and that the ProfilingProcessHost
is only started when the flags are passed.

Note, this test is focused on ONLY the memlog interface. ProfilingProcessHost
may eventually evolved into hosting other types of profiling services thus
the test is named for Memlog and not PPH.

Also, with this CL is a shutdown crash fix for MemlogClient being deleted on
the wrong thread.

Bug:  753218 
Change-Id: I37111b7a61d9a4c18b37680b3216975be2d0535c
Reviewed-on: https://chromium-review.googlesource.com/644608
Commit-Queue: Brett Wilson <brettw@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Reviewed-by: Albert J. Wong <ajwong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499103}
[modify] https://crrev.com/69cf4732e6fc941622eb70c84869f35b52ef8281/.gn
[modify] https://crrev.com/69cf4732e6fc941622eb70c84869f35b52ef8281/chrome/browser/profiling_host/BUILD.gn
[modify] https://crrev.com/69cf4732e6fc941622eb70c84869f35b52ef8281/chrome/browser/profiling_host/chrome_browser_main_extra_parts_profiling.cc
[add] https://crrev.com/69cf4732e6fc941622eb70c84869f35b52ef8281/chrome/browser/profiling_host/memlog_browsertest.cc
[modify] https://crrev.com/69cf4732e6fc941622eb70c84869f35b52ef8281/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/69cf4732e6fc941622eb70c84869f35b52ef8281/chrome/browser/profiling_host/profiling_process_host.h
[modify] https://crrev.com/69cf4732e6fc941622eb70c84869f35b52ef8281/chrome/common/chrome_content_client.cc
[modify] https://crrev.com/69cf4732e6fc941622eb70c84869f35b52ef8281/chrome/common/chrome_content_client.h
[modify] https://crrev.com/69cf4732e6fc941622eb70c84869f35b52ef8281/chrome/test/BUILD.gn

Blocking: 751321
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 19 2017

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

commit 708c31d0f4088ce3239d9eb17e3c5917c33484b9
Author: erikchen <erikchen@chromium.org>
Date: Tue Sep 19 00:28:53 2017

Update memlog browsertest to confirm existence of leaked allocations.

Confirming leaks requires two synchronization steps. The browser process must
flush all the allocations and ensure that the messages are sent to the profiling
process. This is done by performing 68,000 flushing allocations, and was already
implemented. The profiling process must finish processing all the messages it
receives on the connection thread for the browser process. It typically has a
large backlog of tasks posted onto the connection thread's task queue.
DumpProcess() now hops onto the connection thread's task queue, and back onto
the IO thread before dumping allocations to a file. This ensures that all
previously posted tasks are processed first.

Bug:  753218 
Change-Id: I08e7f49f597fadc24617bcf3612ab147eb3a458c
Reviewed-on: https://chromium-review.googlesource.com/671192
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Albert J. Wong <ajwong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502729}
[modify] https://crrev.com/708c31d0f4088ce3239d9eb17e3c5917c33484b9/chrome/browser/profiling_host/memlog_browsertest.cc
[modify] https://crrev.com/708c31d0f4088ce3239d9eb17e3c5917c33484b9/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/708c31d0f4088ce3239d9eb17e3c5917c33484b9/chrome/profiling/memlog_connection_manager.cc
[modify] https://crrev.com/708c31d0f4088ce3239d9eb17e3c5917c33484b9/chrome/profiling/memlog_connection_manager.h
[modify] https://crrev.com/708c31d0f4088ce3239d9eb17e3c5917c33484b9/chrome/profiling/memlog_impl.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Sep 21 2017

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

commit c6aeb4201277bf549827217b5372f624167e580a
Author: erikchen <erikchen@chromium.org>
Date: Thu Sep 21 02:30:26 2017

Test partition alloc in memlog browser test.

The test uses partition alloc to make allocations in the browser process, and
then confirms that those allocations are correctly recorded in the resulting
dump.

Bug:  753218 
Change-Id: I864c475172cd0cf092d1ffd327029b759bc2cab1
Reviewed-on: https://chromium-review.googlesource.com/669295
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503318}
[modify] https://crrev.com/c6aeb4201277bf549827217b5372f624167e580a/chrome/browser/profiling_host/memlog_browsertest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Oct 3 2017

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

commit 27e8cb192bd93b79f2e73daddee8e0dd297b24f3
Author: erikchen <erikchen@chromium.org>
Date: Tue Oct 03 01:46:44 2017

Add Memlog tracing end-to-end test.

Since ProfilingProcessHost adds the heap dump to the trace asynchronously, this
CL adds a SetDumpProcessForTracingCallback() method to watch for this event.
This CL also refactors the Memlog browsertest to better accomodate verification
of dumps from both tracing and non-tracing paths.

Bug:  753218 
Change-Id: Ifd2aba0aafc7ac35f778617ecf0dd021e20dfd4c
Reviewed-on: https://chromium-review.googlesource.com/691324
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505916}
[modify] https://crrev.com/27e8cb192bd93b79f2e73daddee8e0dd297b24f3/chrome/browser/profiling_host/memlog_browsertest.cc
[modify] https://crrev.com/27e8cb192bd93b79f2e73daddee8e0dd297b24f3/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/27e8cb192bd93b79f2e73daddee8e0dd297b24f3/chrome/browser/profiling_host/profiling_process_host.h

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 3 2017

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

commit 256ca3bc2a2f4954ef4568874acbfe5d44b562af
Author: Kentaro Hara <haraken@chromium.org>
Date: Tue Oct 03 04:37:45 2017

Revert "Add Memlog tracing end-to-end test."

This reverts commit 27e8cb192bd93b79f2e73daddee8e0dd297b24f3.

Reason for revert: This broke AllProcesses/MemlogBrowserTest.EndToEndTracing/0 on many platforms.

https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/45306

Original change's description:
> Add Memlog tracing end-to-end test.
> 
> Since ProfilingProcessHost adds the heap dump to the trace asynchronously, this
> CL adds a SetDumpProcessForTracingCallback() method to watch for this event.
> This CL also refactors the Memlog browsertest to better accomodate verification
> of dumps from both tracing and non-tracing paths.
> 
> Bug:  753218 
> Change-Id: Ifd2aba0aafc7ac35f778617ecf0dd021e20dfd4c
> Reviewed-on: https://chromium-review.googlesource.com/691324
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Reviewed-by: Brett Wilson <brettw@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#505916}

TBR=brettw@chromium.org,erikchen@chromium.org,etienneb@chromium.org

Change-Id: I775f27793b85ec05d1e24e09177202f0ab17763c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  753218 
Reviewed-on: https://chromium-review.googlesource.com/696464
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505943}
[modify] https://crrev.com/256ca3bc2a2f4954ef4568874acbfe5d44b562af/chrome/browser/profiling_host/memlog_browsertest.cc
[modify] https://crrev.com/256ca3bc2a2f4954ef4568874acbfe5d44b562af/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/256ca3bc2a2f4954ef4568874acbfe5d44b562af/chrome/browser/profiling_host/profiling_process_host.h

Project Member

Comment 16 by bugdroid1@chromium.org, Oct 13 2017

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

commit ee20d70027e8dee73a77d80de4095acc61ae872c
Author: erikchen <erikchen@chromium.org>
Date: Fri Oct 13 05:18:50 2017

[Reland #1] Add Memlog tracing end-to-end test.

The first CL ran into problems at the intersection of the Linux sandbox and OOP
HP. See  Issue 772503  for more details.

> Since ProfilingProcessHost adds the heap dump to the trace asynchronously, this
> CL adds a SetDumpProcessForTracingCallback() method to watch for this event.
> This CL also refactors the Memlog browsertest to better accomodate verification
> of dumps from both tracing and non-tracing paths.
>
> Bug:  753218 
> Reviewed-on: https://chromium-review.googlesource.com/691324
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Reviewed-by: Brett Wilson <brettw@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#505916}

Bug:  753218 
TBR: brettw@chromim.org
Change-Id: I90dad2acb23982f5fe8cadae856727cce8af0e5c
Reviewed-on: https://chromium-review.googlesource.com/701454
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508601}
[modify] https://crrev.com/ee20d70027e8dee73a77d80de4095acc61ae872c/chrome/browser/profiling_host/memlog_browsertest.cc
[modify] https://crrev.com/ee20d70027e8dee73a77d80de4095acc61ae872c/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/ee20d70027e8dee73a77d80de4095acc61ae872c/chrome/browser/profiling_host/profiling_process_host.h

Status: Fixed (was: Assigned)
Owner: erikc...@chromium.org
Status: Started (was: Fixed)
Re-opening, and assigning to myself, since our e2e tests don't run on Android.

I was hoping to take the existing browser_tests and make them runnable on Android, which should be possible since content_browsertests are runnable on Android, and we don't use any UI components in browser_tests. After doing some digging around, I discovered that content_browsertests are not multi-process on Android:
https://cs.chromium.org/chromium/src/content/test/content_test_launcher.cc?type=cs&q=RegisterInProcessThreads&l=84

Multiprocess is hard, because you can't fork on Android, you need to launch new activities at the java-layer, which means we'll need to refactor/plumb through a mechanism to do this. Getting all this to work would be a non-trivial task, so I'm going to just add instrumentation tests:

https://codesearch.chromium.org/chromium/src/chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java
Project Member

Comment 19 by bugdroid1@chromium.org, Nov 17 2017

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

commit e0b4355f0651adb1ebc2c513dc4410471af712f5
Author: Erik Chen <erikchen@chromium.org>
Date: Fri Nov 17 20:38:19 2017

Add Android OOP HP end-to-end tests.

This CL has three components:
  1) The bulk of the logic in OOP HP was refactored into ProfilingTestDriver.
  2) Adds a java instrumentation test, along with a JNI shim that forwards into
  ProfilingTestDriver.
  3) Creates a new apk: chrome_public_apk_for_test that contains the same
  content as chrome_public_apk, as well as native files needed for (2).
  chrome_public_apk_test now targets chrome_public_apk_for_test instead of
  chrome_public_apk.

Other ideas, discarded:
  * Originally, I attempted to make the browser_tests target runnable on
  Android. The primary problem is that native test harness cannot fork
  or spawn processes. This is difficult to solve.

More details on each of the components:
(1) ProfilingTestDriver
  * The TracingController test was migrated to use ProfilingTestDriver, but the
  write-to-file test was left as-is. The latter behavior will likely be phased
  out, but I'll clean that up in a future CL.
  * gtest isn't supported for Android instrumentation tests. ProfilingTestDriver
  has a single function RunTest that returns a 'bool' indicating success. On
  failure, the class uses LOG(ERROR) to print the nature of the error. This will
  cause the error to be printed out on browser_test error. On instrumentation
  test failure, the error will be forwarded to logcat, which is available on all
  infra bot test runs.
(2) Instrumentation test
  * For now, I only added a single test for the "browser" mode. Furthermore, I'm
  only testing the start with command-line path.
(3) New apk
  * libchromefortest is a new shared library that contains all content from
  libchrome, but also contains native sources for the JNI shim.
  * chrome_public_apk_for_test is a new apk that contains all content from
  chrome_public_apk, but uses a single shared library libchromefortest rather
  than libchrome. This also contains java sources for the JNI shim.
  * There is no way to just add a second shared library to chrome_public_apk
  that just contains the native sources from the JNI shim without causing ODR
  issues.
  * chrome_public_test_apk now has apk_under_test = chrome_public_apk_for_test.
  * There is no way to add native JNI sources as a shared library to
  chrome_public_test_apk without causing ODR issues.

Finally, this CL drastically increases the timeout to wait for native
initialization. The previous timeout was 2 *
CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL, which flakily failed for this test.
This suggests that this step/timeout is generally flaky. I increased the timeout
to 20 * CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL.

Bug:  753218 
Change-Id: Ic224b7314fff57f1770a4048aa5753f54e040b55
Reviewed-on: https://chromium-review.googlesource.com/770148
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517541}
[modify] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/android/BUILD.gn
[add] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/ProfilingProcessHostAndroidTest.java
[add] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/TestAndroidShim.java
[add] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/browser/android/chrome_entry_point_for_test.cc
[modify] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/browser/profiling_host/BUILD.gn
[modify] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/browser/profiling_host/memlog_browsertest.cc
[modify] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/browser/profiling_host/profiling_process_host.h
[add] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/browser/profiling_host/profiling_test_driver.cc
[add] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/browser/profiling_host/profiling_test_driver.h
[add] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/browser/profiling_host/test_android_shim.cc
[add] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/browser/profiling_host/test_android_shim.h
[modify] https://crrev.com/e0b4355f0651adb1ebc2c513dc4410471af712f5/chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java

Project Member

Comment 20 by bugdroid1@chromium.org, Nov 19 2017

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

commit c94d12759d05c26d78852507fb1b64d4ec08d87b
Author: John Budorick <jbudorick@chromium.org>
Date: Sun Nov 19 02:56:32 2017

Revert "Add Android OOP HP end-to-end tests."

This reverts commit e0b4355f0651adb1ebc2c513dc4410471af712f5.

Reason for revert: Breaks chrome_public_test_apk in debug builds. Will explain more in crbug.com/786743.

Original change's description:
> Add Android OOP HP end-to-end tests.
> 
> This CL has three components:
>   1) The bulk of the logic in OOP HP was refactored into ProfilingTestDriver.
>   2) Adds a java instrumentation test, along with a JNI shim that forwards into
>   ProfilingTestDriver.
>   3) Creates a new apk: chrome_public_apk_for_test that contains the same
>   content as chrome_public_apk, as well as native files needed for (2).
>   chrome_public_apk_test now targets chrome_public_apk_for_test instead of
>   chrome_public_apk.
> 
> Other ideas, discarded:
>   * Originally, I attempted to make the browser_tests target runnable on
>   Android. The primary problem is that native test harness cannot fork
>   or spawn processes. This is difficult to solve.
> 
> More details on each of the components:
> (1) ProfilingTestDriver
>   * The TracingController test was migrated to use ProfilingTestDriver, but the
>   write-to-file test was left as-is. The latter behavior will likely be phased
>   out, but I'll clean that up in a future CL.
>   * gtest isn't supported for Android instrumentation tests. ProfilingTestDriver
>   has a single function RunTest that returns a 'bool' indicating success. On
>   failure, the class uses LOG(ERROR) to print the nature of the error. This will
>   cause the error to be printed out on browser_test error. On instrumentation
>   test failure, the error will be forwarded to logcat, which is available on all
>   infra bot test runs.
> (2) Instrumentation test
>   * For now, I only added a single test for the "browser" mode. Furthermore, I'm
>   only testing the start with command-line path.
> (3) New apk
>   * libchromefortest is a new shared library that contains all content from
>   libchrome, but also contains native sources for the JNI shim.
>   * chrome_public_apk_for_test is a new apk that contains all content from
>   chrome_public_apk, but uses a single shared library libchromefortest rather
>   than libchrome. This also contains java sources for the JNI shim.
>   * There is no way to just add a second shared library to chrome_public_apk
>   that just contains the native sources from the JNI shim without causing ODR
>   issues.
>   * chrome_public_test_apk now has apk_under_test = chrome_public_apk_for_test.
>   * There is no way to add native JNI sources as a shared library to
>   chrome_public_test_apk without causing ODR issues.
> 
> Finally, this CL drastically increases the timeout to wait for native
> initialization. The previous timeout was 2 *
> CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL, which flakily failed for this test.
> This suggests that this step/timeout is generally flaky. I increased the timeout
> to 20 * CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL.
> 
> Bug:  753218 
> Change-Id: Ic224b7314fff57f1770a4048aa5753f54e040b55
> Reviewed-on: https://chromium-review.googlesource.com/770148
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Reviewed-by: John Budorick <jbudorick@chromium.org>
> Reviewed-by: Brett Wilson <brettw@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#517541}

TBR=brettw@chromium.org,erikchen@chromium.org,jbudorick@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  753218 
Change-Id: I0ce13ddac560fc685bce8798a59d2e1c784e08af
Reviewed-on: https://chromium-review.googlesource.com/778339
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: John Budorick <jbudorick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517726}
[modify] https://crrev.com/c94d12759d05c26d78852507fb1b64d4ec08d87b/chrome/android/BUILD.gn
[delete] https://crrev.com/3bb869bf41752b57c0c0733a132ae09745dd8078/chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/ProfilingProcessHostAndroidTest.java
[delete] https://crrev.com/3bb869bf41752b57c0c0733a132ae09745dd8078/chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/TestAndroidShim.java
[delete] https://crrev.com/3bb869bf41752b57c0c0733a132ae09745dd8078/chrome/browser/android/chrome_entry_point_for_test.cc
[modify] https://crrev.com/c94d12759d05c26d78852507fb1b64d4ec08d87b/chrome/browser/profiling_host/BUILD.gn
[modify] https://crrev.com/c94d12759d05c26d78852507fb1b64d4ec08d87b/chrome/browser/profiling_host/memlog_browsertest.cc
[modify] https://crrev.com/c94d12759d05c26d78852507fb1b64d4ec08d87b/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/c94d12759d05c26d78852507fb1b64d4ec08d87b/chrome/browser/profiling_host/profiling_process_host.h
[delete] https://crrev.com/3bb869bf41752b57c0c0733a132ae09745dd8078/chrome/browser/profiling_host/profiling_test_driver.cc
[delete] https://crrev.com/3bb869bf41752b57c0c0733a132ae09745dd8078/chrome/browser/profiling_host/profiling_test_driver.h
[delete] https://crrev.com/3bb869bf41752b57c0c0733a132ae09745dd8078/chrome/browser/profiling_host/test_android_shim.cc
[delete] https://crrev.com/3bb869bf41752b57c0c0733a132ae09745dd8078/chrome/browser/profiling_host/test_android_shim.h
[modify] https://crrev.com/c94d12759d05c26d78852507fb1b64d4ec08d87b/chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java

Project Member

Comment 21 by bugdroid1@chromium.org, Nov 20 2017

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

commit 9d81094d7b0bfc8be6bba2f5084e790677e527c8
Author: Erik Chen <erikchen@chromium.org>
Date: Mon Nov 20 17:40:09 2017

[Reland #1] Add Android OOP HP end-to-end tests.

The original CL added a javatest and its dependencies to the apk_under_test.
This causes the dependencies to be stripped from the instrumentation_apk, which
causes issue. This CL updates the build configuration so that the javatest and
its dependencies are only added to the instrumentation_apk.

This is a reland of e0b4355f0651adb1ebc2c513dc4410471af712f5
Original change's description:
> Add Android OOP HP end-to-end tests.
>
> This CL has three components:
>   1) The bulk of the logic in OOP HP was refactored into ProfilingTestDriver.
>   2) Adds a java instrumentation test, along with a JNI shim that forwards into
>   ProfilingTestDriver.
>   3) Creates a new apk: chrome_public_apk_for_test that contains the same
>   content as chrome_public_apk, as well as native files needed for (2).
>   chrome_public_apk_test now targets chrome_public_apk_for_test instead of
>   chrome_public_apk.
>
> Other ideas, discarded:
>   * Originally, I attempted to make the browser_tests target runnable on
>   Android. The primary problem is that native test harness cannot fork
>   or spawn processes. This is difficult to solve.
>
> More details on each of the components:
> (1) ProfilingTestDriver
>   * The TracingController test was migrated to use ProfilingTestDriver, but the
>   write-to-file test was left as-is. The latter behavior will likely be phased
>   out, but I'll clean that up in a future CL.
>   * gtest isn't supported for Android instrumentation tests. ProfilingTestDriver
>   has a single function RunTest that returns a 'bool' indicating success. On
>   failure, the class uses LOG(ERROR) to print the nature of the error. This will
>   cause the error to be printed out on browser_test error. On instrumentation
>   test failure, the error will be forwarded to logcat, which is available on all
>   infra bot test runs.
> (2) Instrumentation test
>   * For now, I only added a single test for the "browser" mode. Furthermore, I'm
>   only testing the start with command-line path.
> (3) New apk
>   * libchromefortest is a new shared library that contains all content from
>   libchrome, but also contains native sources for the JNI shim.
>   * chrome_public_apk_for_test is a new apk that contains all content from
>   chrome_public_apk, but uses a single shared library libchromefortest rather
>   than libchrome. This also contains java sources for the JNI shim.
>   * There is no way to just add a second shared library to chrome_public_apk
>   that just contains the native sources from the JNI shim without causing ODR
>   issues.
>   * chrome_public_test_apk now has apk_under_test = chrome_public_apk_for_test.
>   * There is no way to add native JNI sources as a shared library to
>   chrome_public_test_apk without causing ODR issues.
>
> Finally, this CL drastically increases the timeout to wait for native
> initialization. The previous timeout was 2 *
> CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL, which flakily failed for this test.
> This suggests that this step/timeout is generally flaky. I increased the timeout
> to 20 * CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL.
>
> Bug:  753218 
> Change-Id: Ic224b7314fff57f1770a4048aa5753f54e040b55
> Reviewed-on: https://chromium-review.googlesource.com/770148
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Reviewed-by: John Budorick <jbudorick@chromium.org>
> Reviewed-by: Brett Wilson <brettw@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#517541}

Bug:  753218 
TBR: brettw@chromium.org
Change-Id: Ic6aafb34c2467253f75cc85da48200d19f3bc9af
Reviewed-on: https://chromium-review.googlesource.com/777697
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517850}
[modify] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/android/BUILD.gn
[add] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/ProfilingProcessHostAndroidTest.java
[add] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/android/javatests/src/org/chromium/chrome/browser/profiling_host/TestAndroidShim.java
[add] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/browser/android/chrome_entry_point_for_test.cc
[modify] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/browser/profiling_host/BUILD.gn
[modify] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/browser/profiling_host/memlog_browsertest.cc
[modify] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/browser/profiling_host/profiling_process_host.h
[add] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/browser/profiling_host/profiling_test_driver.cc
[add] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/browser/profiling_host/profiling_test_driver.h
[add] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/browser/profiling_host/test_android_shim.cc
[add] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/browser/profiling_host/test_android_shim.h
[modify] https://crrev.com/9d81094d7b0bfc8be6bba2f5084e790677e527c8/chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java

Project Member

Comment 22 by bugdroid1@chromium.org, Nov 29 2017

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

commit 872446d658e8f6972571eb4d05fc615427cf46f3
Author: erikchen <erikchen@chromium.org>
Date: Wed Nov 29 00:34:40 2017

OOP HP: Unify code for testing and heap dump upload.

Previously, there was two sets of mostly identical logic to start a trace,
ensure that heap dumps are collected, and then report the results.

Bug:  753218 
Change-Id: I8dba3c94256a8da9e9364b49bd86275db14a9df3
Reviewed-on: https://chromium-review.googlesource.com/777554
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519927}
[modify] https://crrev.com/872446d658e8f6972571eb4d05fc615427cf46f3/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/872446d658e8f6972571eb4d05fc615427cf46f3/chrome/browser/profiling_host/profiling_process_host.h
[modify] https://crrev.com/872446d658e8f6972571eb4d05fc615427cf46f3/chrome/browser/profiling_host/profiling_test_driver.cc
[modify] https://crrev.com/872446d658e8f6972571eb4d05fc615427cf46f3/chrome/browser/profiling_host/profiling_test_driver.h

Status: Fixed (was: Started)

Sign in to add a comment