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

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Xianzhu Wang]

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.

[Xianzhu Wang]

Created attachment 147647 [details]
patch

[Xianzhu Wang]

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

[Dirk Pranke]

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 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 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 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]

Created attachment 147653 [details]
patch v3

[Tony Chang]

(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]

(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]

(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 on attachment 147653 [details]
patch v3

cq+. Verified with Tony offline.

[Xianzhu Wang]

Comment on attachment 147653 [details]
patch v3

Want to add a line self.stop() to ensure clear state before retry.

[Xianzhu Wang]

Created attachment 147659 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 147659 [details]
patch for landing

Clearing flags on attachment: 147659

Committed r120385: <http://trac.webkit.org/changeset/120385>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.