DeviceManagerADB: killProcess(): Add forceKill parameter and kill all matching processes

RESOLVED FIXED in mozilla14

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: decoder, Assigned: wlach)

Tracking

Trunk
mozilla14
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch Patch (obsolete) — Splinter Review
Currently, DeviceManagerADB will use a standard kill (SIGTERM) when calling killProces(). Sometimes, Fennec may be hanging though and will not react. In that case, we'll need a way to send a SIGKILL instead. I suggest adding an optional boolean parameter "forceKill" to do this. I also added this parameter to the API and the SUT implementation, however, SUT will warn if the parameter is used (because it is ignored there for now).

Additionally, the killProcess implementation of ADB should not terminate after having located the first matching process. In fact, there may be multiple processes with the same app name that we should all kill.

I'm not sure about the return value of the ADB implementation though yet: We'd have to return multiple outputs, but the SUT implementation just returns a string. Should I join the array before returning it?
Attachment #592793 - Flags: review?(gbrown)
Comment on attachment 592793 [details] [diff] [review]
Patch

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

> I'm not sure about the return value of the ADB implementation though yet: 
> We'd have to return multiple outputs, but the SUT implementation just returns 
> a string. Should I join the array before returning it?

It looks to me like all killProcess clients ignore the return value, (at least on mozilla-central) so there isn't an immediate practical concern. However, I think it is important that we at least keep the adb and sut return types the same. Either join the array (actually, it's a "list", isn't it?) in the adb version, or wrap the string in a list in the sut version.

Perhaps you should open a bug to improve the sut killProcess capabilities also?

Otherwise, no concerns. r+ with the return type issue resolved.
Attachment #592793 - Flags: review?(gbrown) → review+
Posted patch Updated patch (obsolete) — Splinter Review
Updated patch to join returned list to string before returning it in DeviceManagerADB. R+ carried over from last patch.
Attachment #592793 - Attachment is obsolete: true
Attachment #593202 - Flags: review+
(In reply to Geoff Brown [:gbrown] from comment #1)

> Perhaps you should open a bug to improve the sut killProcess capabilities
> also?

I'm not really familiar with SUT at all. Maybe jmaher knows something about the necessary steps there?
I recommend putting a bug on file for SUT.  It seems that we put a fix in every couple months to SUT, so getting this in there is a good idea.
do we want this landed as well?
(In reply to Joel Maher (:jmaher) from comment #5)
> do we want this landed as well?

Yes please :)
(In reply to Christian Holler (:decoder) from comment #3)
> (In reply to Geoff Brown [:gbrown] from comment #1)
> 
> > Perhaps you should open a bug to improve the sut killProcess capabilities
> > also?
> 
> I'm not really familiar with SUT at all. Maybe jmaher knows something about
> the necessary steps there?

Did you file a bug on this? If so, CC me, there's another issue I'm going to be fixing in the SUTAgent this week, so I can likely pick this up at the same time.
(In reply to Clint Talbert ( :ctalbert ) from comment #7)
> (In reply to Christian Holler (:decoder) from comment #3)
> > (In reply to Geoff Brown [:gbrown] from comment #1)
> > 
> > > Perhaps you should open a bug to improve the sut killProcess capabilities
> > > also?
> > 
> > I'm not really familiar with SUT at all. Maybe jmaher knows something about
> > the necessary steps there?
> 
> Did you file a bug on this? If so, CC me, there's another issue I'm going to
> be fixing in the SUTAgent this week, so I can likely pick this up at the
> same time.

Just filed bug 729552 for the SUT stuff.
(In reply to Geoff Brown [:gbrown] from comment #1)
> Comment on attachment 592793 [details] [diff] [review]
> Patch
> 
> Review of attachment 592793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > I'm not sure about the return value of the ADB implementation though yet: 
> > We'd have to return multiple outputs, but the SUT implementation just returns 
> > a string. Should I join the array before returning it?
> 
> It looks to me like all killProcess clients ignore the return value, (at
> least on mozilla-central) so there isn't an immediate practical concern.
> However, I think it is important that we at least keep the adb and sut
> return types the same. Either join the array (actually, it's a "list", isn't
> it?) in the adb version, or wrap the string in a list in the sut version.
> 
> Perhaps you should open a bug to improve the sut killProcess capabilities
> also?
> 
> Otherwise, no concerns. r+ with the return type issue resolved.

I noticed this bug after realizing that it hasn't landed in m-c yet.

Honestly I'm not sure if the "output" of the kill command is really a useful return value. Who care about a bunch of pids that are no longer around if we succeeded? :) How about we return True if we successfully killed something and False in case of an error? As :gbrown pointed out, we're not currently using the value for anything right now, but we may as well try to keep a sensible API.

I can modify the patch to do this. I would prefer to do so after bug 728298 is landed, since that will change some details about how we call things.
Similar to Christian's earlier patch, with the following changes:

1. Remove comment explaining how we're killing processes and what the earlier behaviour was (this is obvious enough from the code IMO).
2. Incorporate my earlier suggestion to make the interface return True upon success and False on failure (instead of returning a bunch of data we don't know what to do with)
Assignee: choller → wlachance
Attachment #593202 - Attachment is obsolete: true
Attachment #601003 - Flags: review?(gbrown)
Comment on attachment 601003 [details] [diff] [review]
Add forceKill parameter and kill all matching processes

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

::: build/mobile/devicemanagerADB.py
@@ +373,5 @@
> +         if forceKill:
> +           args.append("-9")
> +         args.append(pid)
> +         p = self.runCmdAs(args)
> +         didKillProcess = True

You have probably already considered this, but I will just mention it anyway: kill might fail, due to insufficient permissions, etc, and we won't detect that failure. We could check the output for strings like "could not kill", "Operation not permitted", but that might differ across android versions and be error prone. And then what would we return if multiple processes satisfy the appname and some succeed and others fail? It's probably best just the way it is; the caller can always check the process list to verify success.
Attachment #601003 - Flags: review?(gbrown) → review+
We should try to land this. Just checked, patch still applies fine against m-c.
Keywords: checkin-needed
Please can you set the target milestone when landing on inbound, along the lines of http://blog.bonardo.net/2012/03/23/how-you-can-help-mozilla-inbound-sheriffs-when-pushing :-)

https://hg.mozilla.org/mozilla-central/rev/ba379b14fd60
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
> Please can you set the target milestone when landing on inbound, along the
> lines of
> http://blog.bonardo.net/2012/03/23/how-you-can-help-mozilla-inbound-sheriffs-
> when-pushing :-)

Sure. Thanks for the headsup. :)
You need to log in before you can comment on or make changes to this bug.