629266 - org.mozilla.fennec is left running after dm.updateApp()

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Mike Taylor [:bear]]

after the push-reboot-install process Fennec is still running and this is causing all tests to fail with a warning about an unclean system.

we confirmed this by calling dm.processExist('org.mozilla.fennec') and if the PID returned was not None, doing a dm.kill('org.mozilla.fennec') call.

After doing this we are seeing green test runs

Comment 1

[Aki Sasaki (not active)]

I looked for /data/data/org.mozilla.fennec on tegras post-image after talking with Bob, and didn't find it on disk.

I think I was mistaken, and Bear had started tests on some of the newly imaged tegras, which installed fennec before I checked.  So now we're led to believe that org.mozilla.fennec is being started somewhere after installation.  I'm not sure if that's an SUT/helper bug or a Fennec bug.

We do have this workaround, but it would be good to solve the root problem and remove the workaround.

Comment 2

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

I swear I have seen this locally, but nobody can confirm when I ask in IRC.  My impressions are this is a fennec specific thing after an initial first installation.

Comment 3

[cmtalbert]

I've never seen this happen.  I could see how it could happen (perhaps if we had a pre-loader for fast start or something).  I don't think we have one of those in android (I think we did for winmo).  At any rate, given the unreproducibility of this it's probably far more deterministic to fight this with the workaround for now until we can find out exactly what is happening under the covers and figure out if that is something we can change or not.

Comment 4

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

11:00 < jmaher> blassey: when rebooting a tegra, org.mozilla.fennec is running automatically.  Is this expected?
11:02 < mwu> jmaher: yes
11:03 < fabrice> jmaher, this is because we are notified when android.net.conn.CONNECTIVITY_CHANGE is broadcasted
11:03 < jmaher> mwu: ok, is this always going to be the case?  is there a way to disable this?
11:03 < mwu> jmaher: no easy way to disable it aside from removing the connectivity change listener from androidmanifest.xml
11:03 < jmaher> I can work around this, just want to make sure it is expected and we understand when to expect it
11:04 < jmaher> mwu: will this be on all devices (at least the majority)?
11:04 < mwu> jmaher: most real world devices will see that happen on startup


we should add some code in our scripts to kill fennec.

Comment 5

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

Attachment: kill fennec before starting mochitest/reftest (1.0)

simple patch to check for fennec running and kill it if needed.  This takes place in the test harness after parsing options and setting up, but before we do the profile creation/webserver setup/test running.

testing both harnesses with and without fennec running.

Comment 6

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

Attachment: kill fennec before starting talos (1.0)

this patch for talos touches desktop code (although not specifically in the if() clause).  I tested this in a few cases on my tegra and it seems to work great.

I want good review of this and will need to run it through staging.

Comment 7

[Mike Taylor [:bear]]

Comment on attachment 509972 [details] [diff] [review]
kill fennec before starting talos (1.0)

is there any chance that browser_path config entry will have something trailing the process name?

/foo/bar/org.mozilla.fennec/

Comment 8

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

for android, browser_path=org.mozilla.fennec.  For possible other platforms (like desktop fennec or n900), it will be /root/path/subpath/fennec.xyz/fennec.  In this scenario this code works as well.

There could be a platform in the future that causes us grief.  And it could be far enough in the future that we would forget about this and have to spend a couple hours debugging.  

The good news is this code is pretty much identical to code that is in devicemanager to detect the process name for processExists().  So if there is something quirky in there other things will fail right away!

Comment 9

[cmtalbert]

Comment on attachment 509864 [details] [diff] [review]
kill fennec before starting mochitest/reftest (1.0)

This looks good.  I thought putting this in remoteAutomation would make more sense, but looking at your patch here, I like that it only hunts and stops fennec when we are first starting the tests, so there is no chance it will disguise any hangs, which could happen if we put it in remoteAutomation.  Bravo.

Comment 10

[cmtalbert]

Comment on attachment 509972 [details] [diff] [review]
kill fennec before starting talos (1.0)

While this is in desktop talos code, it's gated on browser_config having remote set, so I think it's pretty safe.  

However, mobile.config does not set the remote: argument in its sample config file.  And remote.config sets the remote attribute to False.

Shouldn't you add changes to this patch to:
* set remote: True in mobile.config
* set remote: True in remote.config

r+ with those changes or some explanation as to why they are safe the way they are.

Comment 11

[Aki Sasaki (not active)]

(In reply to comment #10)
> Shouldn't you add changes to this patch to:
> * set remote: True in mobile.config

mobile.config is currently used for the N900s, which aren't currently utilizing SUT.

Comment 12

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

what aki said:)

for remote.config (intended for android), we set remote=True if the ip address is set.  Basically throughout all of the talos code we hing the remote code paths on the remote==True condition.  Here is where we set that:
http://hg.mozilla.org/build/talos/file/2198b71577c3/remotePerfConfigurator.py#l10

If you are using an android system, you have to set '-e org.mozilla.fennec'.  For everything but android, we have to verify that the value specified to -e is valid whether it is local or via a SUT agent.  The point being that you will not be successful with {remote}PerfConfigurator.py unless you are properly specifying remote parameters.

Comment 13

[cmtalbert]

Ok cool, thanks for explaining that.  r+ for my part FWIW

Comment 14

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

talos part landed:
http://hg.mozilla.org/build/talos/pushloghtml?changeset=519c00152d54

Comment 15

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

we are fixing this as part of 634691

Comment 16

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

wrong bug, we still need to land the m-c patch.

Comment 17

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

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=8d295a60c458