Screenshot from failed story taken too late

[perezju]

On system_health.common_mobile at android nexus-5x, some stories fail with the following dump UI:

UI dump
- (no package):
  - (no id)
- org.chromium.chrome:
  - (no id)
  - android:id/content
  - android:id/navigationBarBackground
  - android:id/statusBarBackground
  - org.chromium.chrome:id/action_bar_root
  - org.chromium.chrome:id/fre_content_wrapper
  - org.chromium.chrome:id/fre_image_and_content
  - org.chromium.chrome:id/fre_main_layout
  - org.chromium.chrome:id/fre_pager
  - org.chromium.chrome:id/image
  - org.chromium.chrome:id/send_report_checkbox['Help make Chrome better by sending usage statistics and crash reports to Google.']
  - org.chromium.chrome:id/terms_accept['Accept & continue']
  - org.chromium.chrome:id/title['Welcome to Chrome']
  - org.chromium.chrome:id/tos_and_privacy[u'By using this application, you agree to Chrome\u2019s Terms of Service and Privacy Notice.']

(full log)

But the corresponding screenshot only shows the launcher after the browser has been closed.

@nedn maybe the recent work to add retries for opening the browser is causing screenshots to be taken after it has been closed?

[nedn]

The work to add retries for opening the browser is only applied to desktop browser (desktop_browser_finder in https://codereview.chromium.org/2868703002/). We need to look at the log to see when it's taken.

[perezju]

After digging through the log and reconstructing the execution flow, this appears to be happening:

• In story_runner.py we call state.WillRunStory(story)
  • This eventually leads to a call to android_browser_backend.Start()
  • .. and a try/except block where we self._WaitForBrowserToComeUp()
  • if an AppCrashException (or subclass) is raised during this block, then loads of useful things are gathered at that point (this includes a "stack trace" which includes a UI dump on android).
  • the try/except block catches the exception, calls self.Close() to close the browser, and re-raises the exception.
• The exception is caught and re-raised a few times, until we land back in the relevant exception handling block back on story_runner.py
• Processing this error we call state.DumpStateUponFailure(story, results), which is what eventually takes the screenshot. (but now it's too late, the browser is closed!)

I'll note that the desktop_browser_backend is affected by a similar thing (but not the cros_browser_backend).

I can think of two alternatives:

1. Grab the screenshot when raising the AppCrashException (or the more generic Telemetry Error).

   Pros: This sounds like a good place (in general) for trying to grab as much information as possible about the environment where the error occurred.

   Cons: We don't get screenshots for non-Telemetry Errors. (And may be awkward to get hold of the platform object at that point to grab the screenshot.)

2. Do not close the browser upon failures in *_browser_backend.Start

   Pros: It's probably cleaner. Also the current implementation (in both android/desktop) is somewhat inconsistent. There is significant code between "opening the browser" and "waiting for it" which is not in the try/except block, and which will not cause the browser to be closed upon failure.

   Cons: Feels a bit icky for the *_browser_backend object to not clean up after itself in case of failures?

[nedn]

How about do screen shot at both when AppCrash & in state.DumpStateUponFailure ? In some sense, I am seeing this paradigm is similar to: you take browser log & platform log when thing goes wrong.