936594 - SUTAgent InstallApp error handling

Status: RESOLVED FIXED

(Testing Graveyard :: SUTAgent, defect)

Description

[Bob Clary [:bc] (inactive)]

http://mxr.mozilla.org/mozilla-central/source/build/mobile/sutagent/android/DoCommand.java#3422

When pm fails to install an apk, we get output like:

Failure [INSTALL_FAILED_INVALID_APK]

devicemanager detects this as a failure due to the presence of the string 'Failure'.

I'm running into a situation on one of mcote's old phones where the fennec installation fails due to a Segmentation Fault but the device manager fails to detect the failure since the output doesn't contain the string 'Failure'.

In the SUTAgent's InstallApp, it automatically redirects the OutputStream of the process executing pm to the OutputStream given by the caller and we don't have access to it. It isn't clear to me if this is just for stdout and we are just dropping stderr on the floor.

We also return a success condition installation complete [exitcode] even if the exitcode for the process was non-zero.

I also see a couple of places where the installation fails but the return string does not contain the string 'Failure'. For example, in the IllegalThreadStateException handler and possibly the IOException handler and InterruptedException handler.

I would like to make sure we include 'Failure' whenever the installation failed or can't be guaranteed so that the devicemanager will not erroneously report success.

1. Remove the RedirOutputThread and read the stdout and stderr from the pm process. Write these to the out OutputStream as part of the output.

2. Detect if the pm process exits with a non-zero exit code or if 'Segmentation Fault' occurs in the pm process output and add the 'Failure' string to the output.

3. Add the 'Failure' string to the output if IllegalThreadStateException, IOException or InterruptedException occurs.

Thoughts?

Comment 1

[Bob Clary [:bc] (inactive)]

The more I think about it, a better approach for detecting the failure in devicemanager is for it to test for |installation complete [0]| as success and if that is missing then it can treat it as a failure. This will also treat any case where the SUTAgent couldn't write to the out OutputStream as a failure.

We should still treat pm process non zero exit codes as failures and make sure the SUTAgent dumps both stdout and stderr to the out OutputStream for diagnostics.

Thoughts redeux?

Comment 2

[Bob Clary [:bc] (inactive)]

nevermind re the stdout and stderr, they are both covered under http://dxr.mozilla.org/mozilla-central/source/build/mobile/sutagent/android/RedirOutputThread.java

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: InstallApp-segmentation-fault.patch

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 829721 [details] [diff] [review]
InstallApp-segmentation-fault.patch

Review of attachment 829721 [details] [diff] [review]:
-----------------------------------------------------------------

to clarify, we are just adding 'Failure' to the return message so we can detect it easier, right?

Comment 5

[Bob Clary [:bc] (inactive)]

That was sort of the original plan, but it has morphed a little.

Previously, we passed the output of the pm install process back to the caller via the out OutputStream plus a string of the form 'installation complete [exitcode]'.

Callers such as devicemanagerSUT.installApp relied on pm install indicating failure through the 'Failure' string in its output. As long as the pm process didn't include the 'Failure' string devicemanagerSUT.installApp (and others?) would treat any exit code from the pm process or the case of an IllegalThreadStateException as a 'success' state (without the 'Failure' string).

This behavior is a problem since if the pm install process segfaulted or otherwise failed to include the 'Failure' string in the output we would not treat it as an installation failure.

With this patch, a successful installation will be marked with the success state 'installation complete [0]' if and only if the pm process returned with a 0 exit code and did not throw an IllegalThreadStateException.

Note if an exception occurs during the out.write, and if we didn't emit the 'installation complete [0]' to the out OutputStream, then consumers of SUTAgent.InstallApp 'can' consider that to be a failure as well. While this may flag a false installation failure if the app was installed but the output failed, it will always catch the case where the installation failed and the output failed as well. Previously this case would be hidden from the caller.

In bug 936782, I've changed devicemanagerSUT.installApp to throw if the returned data from SUTAgent.InstallApp does not contain 'installation complete [0]'. This is better than detecting a 'Failure' string, since if for *any* reason our call to SUTAgent.InstallApp fails to output 'installation complete [0]' we will treat is as a failure.

The use of 'Failure' in the output for the non-zero exit code case or IllegalThreadStateException case is really a red herring for devicemanagerSUT.installApp since we will no longer use 'Failure'. In case other consumers do use the 'Failure' string as an indicator we will still support them by providing them with the expected signal.

Whew! I hope that is clear. :-)

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

roger that

Comment 7

[Bob Clary [:bc] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2eea9b6ba1d7

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2eea9b6ba1d7