Bug 89124

Summary:

[Chromium-Android] Should retry a few times when failed to start DumpRenderTree

Product:

WebKit

Reporter:

Xianzhu Wang <wangxianzhu>

Component:

Tools / Tests

Assignee:

Xianzhu Wang <wangxianzhu>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dpranke, jnd, ojan, tony, webkit.review.bot, zhenghao

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

66687

Attachments:

Description	Flags
patch	none
patch v2 (sleep before retry)	dpranke: review+
patch v3	wangxianzhu: commit-queue-
patch for landing	none

Xianzhu Wang

Reported 2012-06-14 13:48:43 PDT

On Android sometimes 'am start' fails due to temporary condition. For example: 00:49:07.657 11509 "adb shell am start -n org.chromium.native_test/.ChromeNativeTestActivity" 00:49:07.657 11509 Run adb result: Starting: Intent { cmp=org.chromium.native_test/.ChromeNativeTestActivity } android.os.DeadObjectException at android.os.BinderProxy.transact(Native Method) at android.app.ActivityManagerProxy.startActivity(ActivityManagerNative.java:1750) at com.android.commands.am.Am.runStart(Am.java:463) at com.android.commands.am.Am.run(Am.java:108) at com.android.commands.am.Am.main(Am.java:81) at com.android.internal.os.RuntimeInit.nativeFinishInit(Native Method) at com.android.internal.os.RuntimeInit.main(RuntimeInit.java:235) at dalvik.system.NativeStart.main(Native Method) Should retry a few times before giving up.

Attachments
patch (3.12 KB, patch) 2012-06-14 13:56 PDT, Xianzhu Wang	no flags	Details Formatted Diff Diff
patch v2 (sleep before retry) (3.15 KB, patch) 2012-06-14 13:58 PDT, Xianzhu Wang	dpranke: review+	Details Formatted Diff Diff
patch v3 (3.10 KB, patch) 2012-06-14 14:21 PDT, Xianzhu Wang	wangxianzhu: commit-queue-	Details Formatted Diff Diff
patch for landing (3.13 KB, patch) 2012-06-14 15:02 PDT, Xianzhu Wang	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Xianzhu Wang

Comment 1 2012-06-14 13:56:45 PDT

Created attachment 147647 [details] patch

Xianzhu Wang

Comment 2 2012-06-14 13:58:47 PDT

Created attachment 147649 [details] patch v2 (sleep before retry)

Dirk Pranke

Comment 3 2012-06-14 14:02:54 PDT

Comment on attachment 147649 [details] patch v2 (sleep before retry) View in context: https://bugs.webkit.org/attachment.cgi?id=147649&action=review r+. Leaving as cq? in case you want to make my purely optional change :). > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:494 > + time.sleep(2) View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review r+ ... leaving cq? in case you want to make the change I suggest (it's purely optional). > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:493 > + raise AssertionError('Failed to start DumpRenderTree application multiple times. Give up.') You could also do: for i in range(3): if self._start_once(pixel_tests, per_test_args): return _log.error(...) time.sleep(2) raise AssertionError(...')

Tony Chang

Comment 4 2012-06-14 14:05:46 PDT

Comment on attachment 147647 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:489 > + while True: > + if self._start_once(pixel_tests, per_test_args): > + break Nit: while not self._start_once(pixel_tests, per_test_args): > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 > self._port._run_adb_command(['logcat', '-c']) > self._port._run_adb_command(['shell', 'echo'] + self.cmd_line(pixel_tests, per_test_args) + ['>', COMMAND_LINE_FILE]) Do you want to rerun these every time? Do these ever fail?

Xianzhu Wang

Comment 5 2012-06-14 14:13:51 PDT

Comment on attachment 147649 [details] patch v2 (sleep before retry) View in context: https://bugs.webkit.org/attachment.cgi?id=147649&action=review >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:493 >> + raise AssertionError('Failed to start DumpRenderTree application multiple times. Give up.') > > You could also do: > > for i in range(3): > if self._start_once(pixel_tests, per_test_args): > return > _log.error(...) > time.sleep(2) > raise AssertionError(...') Nice suggestion. However I'd like Tony's suggestion as we use that style elsewhere in this file :)

Xianzhu Wang

Comment 6 2012-06-14 14:19:59 PDT

Comment on attachment 147647 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:489 >> + break > > Nit: while not self._start_once(pixel_tests, per_test_args): Done. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 >> self._port._run_adb_command(['shell', 'echo'] + self.cmd_line(pixel_tests, per_test_args) + ['>', COMMAND_LINE_FILE]) > > Do you want to rerun these every time? Do these ever fail? 'logcat -c' which is to clear the Android log is necessary because we don't want duplicated logs during retries. Initialize command line is not necessary to do every time, but I'd keep _start_once() a complete function to start DRT. And it is logically the precondition of 'am start' and I'd keep them together.

Xianzhu Wang

Comment 7 2012-06-14 14:21:36 PDT

Created attachment 147653 [details] patch v3

Tony Chang

Comment 8 2012-06-14 14:24:41 PDT

(In reply to comment #6) > (From update of attachment 147647 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review > > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 > >> self._port._run_adb_command(['shell', 'echo'] + self.cmd_line(pixel_tests, per_test_args) + ['>', COMMAND_LINE_FILE]) > > > > Do you want to rerun these every time? Do these ever fail? > > 'logcat -c' which is to clear the Android log is necessary because we don't want duplicated logs during retries. > Initialize command line is not necessary to do every time, but I'd keep _start_once() a complete function to start DRT. And it is logically the precondition of 'am start' and I'd keep them together. I was thinking it might be helpful to move these two commands before the while loop. It might be useful to have the logcat output for when 'am start' fails. I don't feel strongly about this.

Xianzhu Wang

Comment 9 2012-06-14 14:30:01 PDT

(In reply to comment #8) > (In reply to comment #6) > > (From update of attachment 147647 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review > > > > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 > > >> self._port._run_adb_command(['shell', 'echo'] + self.cmd_line(pixel_tests, per_test_args) + ['>', COMMAND_LINE_FILE]) > > > > > > Do you want to rerun these every time? Do these ever fail? > > > > 'logcat -c' which is to clear the Android log is necessary because we don't want duplicated logs during retries. > > Initialize command line is not necessary to do every time, but I'd keep _start_once() a complete function to start DRT. And it is logically the precondition of 'am start' and I'd keep them together. > > I was thinking it might be helpful to move these two commands before the while loop. It might be useful to have the logcat output for when 'am start' fails. I don't feel strongly about this. The error message of 'am start' won't be lost. Log is cleared before 'am start' to remove useless logs produced by other Android activities before this 'am start'.

Xianzhu Wang

Comment 10 2012-06-14 14:35:25 PDT

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #6) > > > (From update of attachment 147647 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review > > > > > > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 > > > >> self._port._run_adb_command(['shell', 'echo'] + self.cmd_line(pixel_tests, per_test_args) + ['>', COMMAND_LINE_FILE]) > > > > > > > > Do you want to rerun these every time? Do these ever fail? > > > > > > 'logcat -c' which is to clear the Android log is necessary because we don't want duplicated logs during retries. > > > Initialize command line is not necessary to do every time, but I'd keep _start_once() a complete function to start DRT. And it is logically the precondition of 'am start' and I'd keep them together. > > > > I was thinking it might be helpful to move these two commands before the while loop. It might be useful to have the logcat output for when 'am start' fails. I don't feel strongly about this. > > The error message of 'am start' won't be lost. Log is cleared before 'am start' to remove useless logs produced by other Android activities before this 'am start'. Correction: ... produced by other Android activities or previous _start_once() (which has already been outputted)...

Xianzhu Wang

Comment 11 2012-06-14 14:41:00 PDT

Comment on attachment 147653 [details] patch v3 cq+. Verified with Tony offline.

Xianzhu Wang

Comment 12 2012-06-14 14:49:44 PDT

Comment on attachment 147653 [details] patch v3 Want to add a line self.stop() to ensure clear state before retry.

Xianzhu Wang

Comment 13 2012-06-14 15:02:09 PDT

Created attachment 147659 [details] patch for landing

WebKit Review Bot

Comment 14 2012-06-14 18:26:17 PDT

Comment on attachment 147659 [details] patch for landing Clearing flags on attachment: 147659 Committed r120385: <http://trac.webkit.org/changeset/120385>

WebKit Review Bot

Comment 15 2012-06-14 18:26:22 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.