android: Don't keep GSA alive if it supports account change broadcasts.

android: Don't keep GSA alive if it supports account change broadcasts.

Chrome currently connects to a bound service exposed by GSA to get
account change notifications. This elevates GSA's priority for the
system to the foreground level when Chrome is in the foreground,
artificially increasing Chrome's memory footprint. On some phones, the
memory consumption of the GSA process can be above 100MB (PSS), even
though only the account change notifications are required.

Newer versions of GSA send a broadcast intent when the account
changes. This CL listens to the broadcasts, and disonnects from the GSA
service when GSA support the account change broadcast mechanism, freing
the memory for Chrome or other apps on the system.

Note that Chrome still briefly connects to GSA on startup to confirm
that it supports the broadcast. A forthcoming will remove this.

BUG=614388
Committed: https://crrev.com/e1d19b388fc67f31372a4c2df33e20c0c18dfd97
Cr-Commit-Position: refs/heads/master@{#427840}

[Benoit L, 2016-10-21 00:22:05]

lizeb@chromium.org changed reviewers:
+ yusufo@chromium.org

[Benoit L, 2016-10-21 00:22:06]

Hi,

This is mostly an FYI for now, as the final GSA API has not landed yet.

[Benoit L, 2016-10-21 00:42:23]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 00:43:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 02:33:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 02:33:18]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2016-10-24 23:12:06]

On 2016/10/21 00:22:06, Benoit L wrote:
> Hi,
> 
> This is mostly an FYI for now, as the final GSA API has not landed yet.

Hi,

You can now really review this I've confirmed that it works as intended. (see
internal bug 32363735 for the APK used).

[Yusuf, 2016-10-24 23:45:59]

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java
(right):

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java:25:
* 2. If GSA supports the broadcast, disconnect from it, otherwise keep the old
method.
we keep the old method within the lifecycle of the current application instance,
right?

We don't save it as a pref or something, so I am questioning the necessity of
the second broadcastreceiver for agsa updating. That will possibly listen to all
application installs and since at the least, this will make things off for a
single lifecycle, I would say we should be OK with not getting the right account
during that time.

Am I missing a pref saved here? I guess how persistent is the USE_SERVICE,
USE_BROADCAST. Isn't it starting from USE_SERVICE everytime Chrome application
process starts?

[Benoit L, 2016-10-24 23:52:19]

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java
(right):

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java:25:
* 2. If GSA supports the broadcast, disconnect from it, otherwise keep the old
method.
On 2016/10/24 23:45:59, Yusuf wrote:
> we keep the old method within the lifecycle of the current application
instance,
> right?
>

Correct.
 
> We don't save it as a pref or something, so I am questioning the necessity of
> the second broadcastreceiver for agsa updating. That will possibly listen to
all
> application installs and since at the least, this will make things off for a
> single lifecycle, I would say we should be OK with not getting the right
account
> during that time.
> 
> Am I missing a pref saved here? I guess how persistent is the USE_SERVICE,
> USE_BROADCAST. Isn't it starting from USE_SERVICE everytime Chrome application
> process starts?

Per offline discussion, it's better to preserve the previous behavior exactly,
that is to have an accurate picture of the current GSA account at all times. In
terms of performance, we are only listening to app udpates while Chrome is
running, and the broadcast only contains a package name.

[Yusuf, 2016-10-24 23:52:22]

lgtm, OK with previous comments per offline discussion. We want to preserve the
current situation where we are always right about the account name.

[Benoit L, 2016-10-24 23:53:49]

lizeb@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Benoit L, 2016-10-24 23:53:49]

Hi Ted!

This is a change to free up to 100MB of RAM on user's devices. See the linked
bug for details.

PTAL, thanks.

[Benoit L, 2016-10-24 23:53:58]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 23:54:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ted C, 2016-10-25 00:02:38]

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:641:
GSAAccountChangeListener.getInstance(getApplicationContext()).connect();
if a ChromeActivity created another ChromeActivity, would you have a
connect/disconnect race?  Wondering if this needs to be ref counted internally.

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:744:
GSAAccountChangeListener.getInstance(getApplicationContext()).disconnect();
should we check if (GSAState.getInstance(this).isGsaAvailable()) { like above?

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java
(right):

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java:46:
public static GSAAccountChangeListener getInstance(Context context) {
If you only need the application context, use ContextUtils.getApplicationContext
and drop the param

[commit-bot: I haz the power, 2016-10-25 01:34:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 01:34:55]

Dry run: This issue passed the CQ dry run.

[jakuba, 2016-10-25 08:03:01]

jakuba@google.com changed reviewers:
+ jakuba@google.com

[jakuba, 2016-10-25 08:03:02]

Nice, I like it. Just a few suggestions.

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java
(right):

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java:24:
* 1. Connect to the GSA service.
As a further optimization, you could note GSA version, cache the info if GSA
supports the broadcast, and save even this first connect. So far, we do not plan
to use server flags, so the only way to change the support is by releasing new
version, AFAIK. +yetanothernerd@google.com is that right?

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java:58:
String accountName = intent.getStringExtra(BROADCAST_INTENT_ACCOUNT_NAME_EXTRA);
It's safer to check the intent action before acting on it.

The Intent filters used in registerReceiver(BroadcastReceiver, IntentFilter) and
in application manifests are not guaranteed to be exclusive. They are hints to
the operating system about how to find suitable recipients. It is possible for
senders to force delivery to specific recipients, bypassing filter resolution.
For this reason, onReceive() implementations should respond only to known
actions, ignoring any unexpected Intents that they may receive.

[Benoit L, 2016-10-25 18:28:43]

Thanks!

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:641:
GSAAccountChangeListener.getInstance(getApplicationContext()).connect();
On 2016/10/25 00:02:38, Ted C (OOO 10.25 - 30) wrote:
> if a ChromeActivity created another ChromeActivity, would you have a
> connect/disconnect race?  Wondering if this needs to be ref counted
internally.

Indeed, thanks!
Done.

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:744:
GSAAccountChangeListener.getInstance(getApplicationContext()).disconnect();
On 2016/10/25 00:02:38, Ted C (OOO 10.25 - 30) wrote:
> should we check if (GSAState.getInstance(this).isGsaAvailable()) { like above?

Done.

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java
(right):

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java:24:
* 1. Connect to the GSA service.
On 2016/10/25 08:03:01, jakuba wrote:
> As a further optimization, you could note GSA version, cache the info if GSA
> supports the broadcast, and save even this first connect. So far, we do not
plan
> to use server flags, so the only way to change the support is by releasing new
> version, AFAIK. mailto:+yetanothernerd@google.com is that right?

I'm not sure that would work though, unfortunately. What happens when the GSA
account changes while Chrome is not running?
Chrome would need to know the current sync account when starting.

The solution would be to make Chrome declare the broadcast listener in the
manifest, but I would prefer to avoid that, because if you have several versions
of Chrome on your system, then every GSA account changes starts multiple
processes.

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java:46:
public static GSAAccountChangeListener getInstance(Context context) {
On 2016/10/25 00:02:38, Ted C (OOO 10.25 - 30) wrote:
> If you only need the application context, use
ContextUtils.getApplicationContext
> and drop the param

Done.

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java:58:
String accountName = intent.getStringExtra(BROADCAST_INTENT_ACCOUNT_NAME_EXTRA);
On 2016/10/25 08:03:01, jakuba wrote:
> It's safer to check the intent action before acting on it.
> 
> The Intent filters used in registerReceiver(BroadcastReceiver, IntentFilter)
and
> in application manifests are not guaranteed to be exclusive. They are hints to
> the operating system about how to find suitable recipients. It is possible for
> senders to force delivery to specific recipients, bypassing filter resolution.
> For this reason, onReceive() implementations should respond only to known
> actions, ignoring any unexpected Intents that they may receive.

Thanks for the tip!

Although I wonder whether that's necessary here.
- The ACCOUNT_UPDATE_BROADCAST_PERMISSION is "signature", so IIUC, only a signed
application can send it.
- The action ACTION_PACKAGE_REPLACED can only be set on an Intent sent by the
system.

Adding the check nonetheless, thanks.
Done.

[jakuba, 2016-10-25 19:50:54]

lgtm

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java
(right):

https://codereview.chromium.org/2431223004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java:24:
* 1. Connect to the GSA service.
On 2016/10/25 18:28:42, Benoit L wrote:
> On 2016/10/25 08:03:01, jakuba wrote:
> > As a further optimization, you could note GSA version, cache the info if GSA
> > supports the broadcast, and save even this first connect. So far, we do not
> plan
> > to use server flags, so the only way to change the support is by releasing
new
> > version, AFAIK. mailto:+yetanothernerd@google.com is that right?
> 
> I'm not sure that would work though, unfortunately. What happens when the GSA
> account changes while Chrome is not running?
> Chrome would need to know the current sync account when starting.
> 
> The solution would be to make Chrome declare the broadcast listener in the
> manifest, but I would prefer to avoid that, because if you have several
versions
> of Chrome on your system, then every GSA account changes starts multiple
> processes.

Yeah, that's right. I didn't think about that case.

[Benoit L, 2016-10-26 16:51:34]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 16:51:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 17:54:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 17:54:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Benoit L, 2016-10-26 18:08:37]

lizeb@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Benoit L, 2016-10-26 18:08:38]

+Maria since Ted is out.

PTAL :-)

[Maria, 2016-10-26 19:43:47]

lgtm

https://codereview.chromium.org/2431223004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java
(right):

https://codereview.chromium.org/2431223004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java:127:
* Disconnects from the GSA service if GSA doesn't support notifications.
should the comment be disconnects if GSA DOES support notifications?

[Maria, 2016-10-26 19:44:20]

On 2016/10/26 19:43:47, Maria wrote:
> lgtm
> 
>
https://codereview.chromium.org/2431223004/diff/80001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java
> (right):
> 
>
https://codereview.chromium.org/2431223004/diff/80001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAAccountChangeListener.java:127:
> * Disconnects from the GSA service if GSA doesn't support notifications.
> should the comment be disconnects if GSA DOES support notifications?

Actually, I take back my comment -- I figured it out later and forgot to remove
it.

[Benoit L, 2016-10-26 21:18:38]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2016-10-26 21:18:39]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/2431223004/#ps80001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-10-26 21:19:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 22:06:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-26 22:08:39]

Description was changed from

==========
android: Don't keep GSA alive if it supports account change broadcasts.

Chrome currently connects to a bound service exposed by GSA to get
account change notifications. This elevates GSA's priority for the
system to the foreground level when Chrome is in the foreground,
artificially increasing Chrome's memory footprint. On some phones, the
memory consumption of the GSA process can be above 100MB (PSS), even
though only the account change notifications are required.

Newer versions of GSA send a broadcast intent when the account
changes. This CL listens to the broadcasts, and disonnects from the GSA
service when GSA support the account change broadcast mechanism, freing
the memory for Chrome or other apps on the system.

Note that Chrome still briefly connects to GSA on startup to confirm
that it supports the broadcast. A forthcoming will remove this.

BUG=614388
==========

to

==========
android: Don't keep GSA alive if it supports account change broadcasts.

Chrome currently connects to a bound service exposed by GSA to get
account change notifications. This elevates GSA's priority for the
system to the foreground level when Chrome is in the foreground,
artificially increasing Chrome's memory footprint. On some phones, the
memory consumption of the GSA process can be above 100MB (PSS), even
though only the account change notifications are required.

Newer versions of GSA send a broadcast intent when the account
changes. This CL listens to the broadcasts, and disonnects from the GSA
service when GSA support the account change broadcast mechanism, freing
the memory for Chrome or other apps on the system.

Note that Chrome still briefly connects to GSA on startup to confirm
that it supports the broadcast. A forthcoming will remove this.

BUG=614388

Committed: https://crrev.com/e1d19b388fc67f31372a4c2df33e20c0c18dfd97
Cr-Commit-Position: refs/heads/master@{#427840}
==========

[commit-bot: I haz the power, 2016-10-26 22:08:40]

Patchset 5 (id:??) landed as
https://crrev.com/e1d19b388fc67f31372a4c2df33e20c0c18dfd97
Cr-Commit-Position: refs/heads/master@{#427840}