1371291 - Autophone - use org.mozilla.gecko.SHUTDOWN and eliminate quitter

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

bug 1368701 is introducing a new means of shutting down fennec and geckoview_example. I tested this and you can find the commentary at bug 1368701 comment 5.

This bug will introduce the use of

adb shell am start -a "org.mozilla.gecko.SHUTDOWN" -n "appname/.App"

to cleanly shutdown the browser as well as remove the installation and use of quitter to shut down pages.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: bug-1371291-v1.patch

Removes the ep1 submodule

Adds the preference app.shutdownintent.enabled to the profile to enable shutdown via intent.

Sends a shutdown intent to tell the app to shut itself down instead of automatically force stopping it.

Adds a max_wait_time argument to stop_application to control how long we will wait for the app to close before force stopping it.

Have S1S2GeckoViewTest use S1S2Test's create_profile instead of overriding it.

This patch does not remove the use of quitter. I believe adding the shutdown intent signalling on top of the use of quitter will allow us to support both approaches on trunk and quitter alone on beta and release until the shutdown intent capability has been uplifted to beta and release.

Comment 2

[Geoff Brown [:gbrown]]

Comment on attachment 8884953 [details] [diff] [review]
bug-1371291-v1.patch

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

No serious concerns here, just questions to clarify my understanding.

::: phonetest.py
@@ +702,5 @@
>          self.dm.shell_output("am start "
>                               "-a android.intent.action.MAIN "
>                               "-c android.intent.category.HOME")
> +        self.loggerdeco.debug('stop_application: %s' % self.build.app_name)
> +        if self.build.app_name == 'org.mozilla.geckoview_example':

It is unfortunate that the -n argument differs like this - a little messy. But I don't have a better way of handling this.

@@ +749,5 @@
>          # make sure firefox isn't running when we try to
>          # install the profile.
>  
> +        self.loggerdeco.info('Creating profile')
> +        self.stop_application(max_wait_time=0)

max_wait_time=0 here because you just want to ensure it is stopped and don't care if it is forced/killed - is that right?

@@ +760,5 @@
>              return False
>  
> +        self.loggerdeco.debug('Initializing profile')
> +        self.run_fennec_with_profile(self.build.app_name, self._initialize_url)
> +        self.stop_application()

No max_wait_time here so the default is used. Have you checked that 10 seconds is generally long enough for the intent to work? Is there a good margin, to allow for variation?

::: tests/s1s2test.py
@@ +298,5 @@
>          starttime, throbberstart, throbberstop = self.analyze_logcat()
>  
> +        self.wait_for_fennec(max_wait_time=1,
> +                             wait_time=1,
> +                             kill_wait_time=1)

Why reduce the wait times here?

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #2)
> Comment on attachment 8884953 [details] [diff] [review]
> bug-1371291-v1.patch
> 
> Review of attachment 8884953 [details] [diff] [review]:

> >  
> > +        self.loggerdeco.info('Creating profile')
> > +        self.stop_application(max_wait_time=0)
> 
> max_wait_time=0 here because you just want to ensure it is stopped and don't
> care if it is forced/killed - is that right?

Yes.

> 
> @@ +760,5 @@
> >              return False
> >  
> > +        self.loggerdeco.debug('Initializing profile')
> > +        self.run_fennec_with_profile(self.build.app_name, self._initialize_url)
> > +        self.stop_application()
> 
> No max_wait_time here so the default is used. Have you checked that 10
> seconds is generally long enough for the intent to work? Is there a good
> margin, to allow for variation?
> 

Yes. During testing, it would show the app shutting down usually within a second or so. There are some outlier cases but 10 seconds is an eon in launching the app and initializing the profile.

> ::: tests/s1s2test.py
> @@ +298,5 @@
> >          starttime, throbberstart, throbberstop = self.analyze_logcat()
> >  
> > +        self.wait_for_fennec(max_wait_time=1,
> > +                             wait_time=1,
> > +                             kill_wait_time=1)
> 
> Why reduce the wait times here?

I already have the measurement, so I'm just waiting for the app to shut down. Using quitter, it would shut down automatically, but without quitter I have to tell it to shut down. I do this call with a short wait in order to shorten the time I am waiting for the app to shut down. This call to wait will wait for a second, then call stop_application. stop_application will issue the shutdown intent, then check if the app exists before it falls back to a force-stop after waiting the default 10 seconds.

If the build doesn't support the shutdown intent (beta, release), then the wait will just result in a force-stop after 10 seconds if quitter hasn't already shut the app down which is the same as now.

If the build does support the shutdown intent (autoland, inbound, central), then we do have a bit of a race between quitter shutting the app down and the intent shutting it down. Either way, we will still wait the default 10 seconds in stop_application waiting for the app to shut down.

Once we get the shutdown intent on all branches, we can eliminate quitter altogether with minimal changes required.

I tested extensively with a tree that did not have quitter and whose test pages had all references to quitter removed. I have done some testing  with quitter still available and with the shutdown intents, but not as much.  

Does that sound ok?

Comment 4

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #3)

Thanks for the great explanation.

> Does that sound ok?

Absolutely!

Comment 5

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/cf03100b7463c2e16c30f8d0d90774b472635046

Comment 6

[Bob Clary [:bc] (inactive)]

Reopening due to bug 1380134.

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: bug-1371291-revert.patch

Comment 8

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/75bb3d7e31915c872bff61037c78b7bb5ca3de6b

Let's see if this fixes the crashes.

Comment 9

[Bob Clary [:bc] (inactive)]

Backing out did indeed stop the bleeding. We'll have to follow up with esawin to see why this failed.

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: bug-1371291-v3.patch

v3 changes

* Added webrtc logging environment variables.

* Dump logcat when crash detected

* Collect logcat prior to loading the initialize_profile.html page to prevent possibility of accidentally detecting a page load event from a previous invocation which is lurking unconsumed in logcat.

I did 10 runs on autophone-4 using nexus 5 and pixels:

https://treeherder.allizom.org/#/jobs?repo=mozilla-inbound&revision=ef38fd38b435f3d4d5f29d27235326785794cce3&filter-searchStr=autophone&exclusion_profile=false

I did 10 runs on my local machine with a samsung gs3:

https://treeherder.allizom.org/#/jobs?repo=mozilla-release&revision=db35b8b566a33c2927bbc098ef37ab818c3a1a01&filter-searchStr=autophone&exclusion_profile=false

I believe the crashes found have an acceptable rate and are not related to the shutdown intent or this patch.

I tweaked the patch after starting the runs to include the additional webrtc logging envvars and to dump logcat if a crash is detected therefore they will not appear in the test runs unfortunately.

Comment 11

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

Comment on attachment 8899884 [details] [diff] [review]
bug-1371291-v3.patch

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

glad to see this moving forward

Comment 12

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/beb5d0c7a411ea4da3cdde2741ab66eb27b1119e
deployed 2017-08-22 09:23

Also deployed the new ep1 files.

I am leaving mozilla-release in the schedule even though it does not support the shutdown intent. I believe the volume is so low that the extra time to shut down will be negligible and that the stop_application will just force stop the app once it finishes waiting for the shutdown intent to take effect. If this turns out to be untrue, we can always remove mozilla-release until 56 is released.

Comment 13

[Bob Clary [:bc] (inactive)]

There appears to be some degree of regression with the throbber values after this landed.