894454 - sutagent crashes during testroot if /data/local/tmp cannot be accessed

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Geoff Brown [:gbrown] (pto Apr 11-18)]

If the sutagent cannot access /data/local/tmp while executing testroot, an NPE occurs and the sutagent crashes:

I/SUTAgentAndroid( 1939): 10.0.2.2 : testroot
I/SUTAgentAndroid( 1939): Caught exception creating file in /data/local/tmp: open failed: EACCES (Permission denied)
E/SUTAgentAndroid( 1939): ERROR: Cannot access world writeable test root
W/dalvikvm( 1939): threadid=13: thread exiting with uncaught exception (group=0xb2e1a908)
E/AndroidRuntime( 1939): FATAL EXCEPTION: CmdWorkerThread
E/AndroidRuntime( 1939): java.lang.NullPointerException
E/AndroidRuntime( 1939): 	at com.mozilla.SUTAgentAndroid.service.CmdWorkerThread.run(CmdWorkerThread.java:142)
W/ActivityManager( 1187):   Force finishing activity com.mozilla.SUTAgentAndroid/.SUTAgentAndroid

This happened on an emulator running sutagent 1.18. It looks to me like this behavior existed previous to 1.18.

I think it would be better to return an error message from testroot and continue running.

Comment 1

[Geoff Brown [:gbrown] (pto Apr 11-18)]

BTW, this failure happened because 'su' was not correctly installed on the emulator: without full su powers, sutagent could not change the permissions on /data/local/tmp.

Comment 2

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: avoid NullPointerExceptions in sutagent

I don't know of any way to bring about the NPE crash other than the specific, invalid configuration that I reported, but just in case...
 - return an error message from GetTestRoot if test root cannot be determined
 - avoid NPE if any command handler returns null

Comment 3

[William Lachance (:wlach)]

Comment on attachment 785147 [details] [diff] [review]
avoid NullPointerExceptions in sutagent

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

I think some of this isn't necessary. r+ with unnecessary changes removed. If you want to keep that stuff, let's discuss. :)

::: build/mobile/sutagent/android/CmdWorkerThread.java
@@ +146,2 @@
>                      if (outputLine.length() > 0)
>                          {

I don't think this change is necessary. We really shouldn't be returning null for processCommand, afaict. Inspecting DoCommand, I can't see other cases where we were other than the one that you fixed.

::: build/mobile/sutagent/android/DataWorkerThread.java
@@ +172,5 @@
>                          outputLine = dc.processCommand(inputLine, out, in, cmdOut);
> +                        if (outputLine == null)
> +                            {
> +                            outputLine = "";
> +                            }

As above?

Comment 4

[Geoff Brown [:gbrown] (pto Apr 11-18)]

I want to keep the "unnecessary changes". There are a lot of code paths returning from processCommand - more than I can inspect reliably! - and some of them return the value returned from Android APIs. Also, we add new commands from time to time. 

If we miss a null return, we hit an NPE on outputLine.length(), sutagent goes down, a test reports something cryptic like "timeout in command...", the machine gets rebooted, and we have almost no way to diagnose the problem -- that's worth preventing.

Comment 5

[William Lachance (:wlach)]

(In reply to Geoff Brown [:gbrown] from comment #4)
> I want to keep the "unnecessary changes". There are a lot of code paths
> returning from processCommand - more than I can inspect reliably! - and some
> of them return the value returned from Android APIs. Also, we add new
> commands from time to time. 
> 
> If we miss a null return, we hit an NPE on outputLine.length(), sutagent
> goes down, a test reports something cryptic like "timeout in command...",
> the machine gets rebooted, and we have almost no way to diagnose the problem
> -- that's worth preventing.

Ok, go ahead and commit as-is. :) This really feels wrong, but I guess there are problems further up the chain that lead us to need to paper over problems like this.

Comment 6

[Geoff Brown [:gbrown] (pto Apr 11-18)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19f48fd7faf1

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19f48fd7faf1