Chromecast: don't try to upload crash immediately.

Chromecast: don't try to upload crash immediately.

This has already been the source of compounding issues after a crash and
isn't a great practice anyways.

To make the upload path cleaner, this also moves some internal code for
Cast ATV settings handling upstream.

R=slan@chromium.org,halliwell@chromium.org,lcwu@chromium.org
BUG=internal b/21664262

Committed: https://crrev.com/1863f66a9edf3cc220ad7159b2090d9556130df6
Cr-Commit-Position: refs/heads/master@{#334417}

[slan, 2015-06-11 15:12:46]

https://codereview.chromium.org/1180633002/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1180633002/diff/1/chromecast/chromecast.gyp#n...
chromecast/chromecast.gyp:487: 'target_name': 'cast_base_java',
Why is this target necessary?

https://codereview.chromium.org/1180633002/diff/1/chromecast/chromecast.gyp#n...
chromecast/chromecast.gyp:496: },  # end of target 'cast_shell_java'
# end of target 'cast_base_java'

https://codereview.chromium.org/1180633002/diff/1/chromecast/crash/android/cr...
File chromecast/crash/android/crash_handler.h (left):

https://codereview.chromium.org/1180633002/diff/1/chromecast/crash/android/cr...
chromecast/crash/android/crash_handler.h:57:
scoped_ptr<google_breakpad::ExceptionHandler> crash_uploader_;
This is no longer used. Please remove.

[gunsch, 2015-06-11 16:35:44]

gunsch@chromium.org changed reviewers:
+ byungchul@chromium.org

[gunsch, 2015-06-11 16:35:45]

[+byungchul]

https://codereview.chromium.org/1180633002/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1180633002/diff/1/chromecast/chromecast.gyp#n...
chromecast/chromecast.gyp:487: 'target_name': 'cast_base_java',
On 2015/06/11 15:12:46, slan wrote:
> Why is this target necessary?

So far, we haven't had a pattern for having multiple Java targets. The side
effect is that everything has ended up in cast_shell_java, which is conceptually
"browser-process shell code"---it even all lives in chromecast/browser!

So this corresponds with a new Java package (org.chromium.chromecast.base) and
exists at the same conceptual level as cast_base.

I will split the rest of cast_shell_java as a follow-up CL. CastSysInfoAndroid
and CastMetricsHelper are good examples of things that should be in
cast_base_java, for example.

https://codereview.chromium.org/1180633002/diff/1/chromecast/chromecast.gyp#n...
chromecast/chromecast.gyp:496: },  # end of target 'cast_shell_java'
On 2015/06/11 15:12:45, slan wrote:
> # end of target 'cast_base_java'

Done.

https://codereview.chromium.org/1180633002/diff/1/chromecast/crash/android/cr...
File chromecast/crash/android/crash_handler.h (left):

https://codereview.chromium.org/1180633002/diff/1/chromecast/crash/android/cr...
chromecast/crash/android/crash_handler.h:57:
scoped_ptr<google_breakpad::ExceptionHandler> crash_uploader_;
On 2015/06/11 15:12:46, slan wrote:
> This is no longer used. Please remove. 

Done.

[slan, 2015-06-11 16:59:59]

lgtm

lgtm

[byungchul, 2015-06-11 17:15:01]

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
File chromecast/base/chromecast_config_android.cc (right):

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
chromecast/base/chromecast_config_android.cc:49:
ChromecastConfigAndroid::GetInstance()->
Why is ChromecastConfigAndroid static in Java and instantiated in c++? Can we
interface CastSettingManager directly from c++?

It looks like, opt-in.stats is necessary in CrashUploader which is initialized
before CastService. Would it make sense make CastSettingsManager a singleton?
ChromecastConfigAndroid has only meaning of that, a singleton interface of
CastSettingsManager. Then, we can avoid initializing CastSettingsManager
multiple times.

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
File
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java
(right):

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java:18:
private static CastSettingsManager sSettingsManager;
So, multiple SettingsManager is created here and in
MediaShellCastReceiverService.java. Why not pass SettingsManager instance in
CastBrowserHelper.initializeBrowser()?

[gunsch, 2015-06-12 00:20:38]

gunsch@chromium.org changed reviewers:
+ lcwu@chromium.org
- alokp@chromium.org

[gunsch, 2015-06-12 00:30:30]

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
File chromecast/base/chromecast_config_android.cc (right):

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
chromecast/base/chromecast_config_android.cc:49:
ChromecastConfigAndroid::GetInstance()->
On 2015/06/11 17:15:00, byungchul wrote:
> Why is ChromecastConfigAndroid static in Java and instantiated in c++? Can we
> interface CastSettingManager directly from c++?
> 
> It looks like, opt-in.stats is necessary in CrashUploader which is initialized
> before CastService. Would it make sense make CastSettingsManager a singleton?
> ChromecastConfigAndroid has only meaning of that, a singleton interface of
> CastSettingsManager. Then, we can avoid initializing CastSettingsManager
> multiple times.

We could probably simplify this code more if we upstreamed opt-in.stats and used
that as the source of truth for the UI (e.g. stored it as a Chromium pref rather
than an Android one). I think Stephen is upstreaming it as part of the crash
handling code, and that could be a future cleanup afterwards.

There are other CastSettingsManager instances/listeners besides here and
MediaShellCastReceiverService.

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
File
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java
(right):

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java:18:
private static CastSettingsManager sSettingsManager;
On 2015/06/11 17:15:00, byungchul wrote:
> So, multiple SettingsManager is created here and in
> MediaShellCastReceiverService.java. Why not pass SettingsManager instance in
> CastBrowserHelper.initializeBrowser()?

Each CastSettingsManager instance can provide a listener for different things.
Why make it a singleton? I don't see a lot of upside, and there are the usual
reasons to avoid singletons.

We could definitely rewrite CastSettingsManager to be able to register multiple
listeners instead of just taking one in the ctor. Do you think that would be
cleaner?

Even in that case it would probably still make sense for the render process to
create its own CastSettingsManager instance.

[gunsch, 2015-06-12 00:30:41]

(+lcwu for committer stamp, since alokp is OOO)

[lcwu1, 2015-06-12 01:13:56]

rs lgtm

[byungchul, 2015-06-12 05:57:00]

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
File chromecast/base/chromecast_config_android.cc (right):

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
chromecast/base/chromecast_config_android.cc:49:
ChromecastConfigAndroid::GetInstance()->
On 2015/06/12 00:30:30, gunsch wrote:
> On 2015/06/11 17:15:00, byungchul wrote:
> > Why is ChromecastConfigAndroid static in Java and instantiated in c++? Can
we
> > interface CastSettingManager directly from c++?
> > 
> > It looks like, opt-in.stats is necessary in CrashUploader which is
initialized
> > before CastService. Would it make sense make CastSettingsManager a
singleton?
> > ChromecastConfigAndroid has only meaning of that, a singleton interface of
> > CastSettingsManager. Then, we can avoid initializing CastSettingsManager
> > multiple times.
> 
> We could probably simplify this code more if we upstreamed opt-in.stats and
used
> that as the source of truth for the UI (e.g. stored it as a Chromium pref
rather
> than an Android one). I think Stephen is upstreaming it as part of the crash
> handling code, and that could be a future cleanup afterwards.
> 
> There are other CastSettingsManager instances/listeners besides here and
> MediaShellCastReceiverService.

Right, we should use chromecast preference for ATV settings ideally. Can you
leave a TODO for that?

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
File
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java
(right):

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java:18:
private static CastSettingsManager sSettingsManager;
On 2015/06/12 00:30:30, gunsch wrote:
> On 2015/06/11 17:15:00, byungchul wrote:
> > So, multiple SettingsManager is created here and in
> > MediaShellCastReceiverService.java. Why not pass SettingsManager instance in
> > CastBrowserHelper.initializeBrowser()?
> 
> Each CastSettingsManager instance can provide a listener for different things.
> Why make it a singleton? I don't see a lot of upside, and there are the usual
> reasons to avoid singletons.
> 
> We could definitely rewrite CastSettingsManager to be able to register
multiple
> listeners instead of just taking one in the ctor. Do you think that would be
> cleaner?
> 
> Even in that case it would probably still make sense for the render process to
> create its own CastSettingsManager instance.

Once we merged preference and CastSettingsManager, api could be changed a lot.
Let's see how it goes.

By the way, is it really necessary for renderer process to access
CastSettingsManager? It looks like to call canSendUsageStats(). Can't it just
return true for renderer process since browser process removes dump files? Or,
passing a command line flag?

[gunsch, 2015-06-12 16:54:52]

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
File chromecast/base/chromecast_config_android.cc (right):

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
chromecast/base/chromecast_config_android.cc:49:
ChromecastConfigAndroid::GetInstance()->
On 2015/06/12 05:57:00, byungchul wrote:
> On 2015/06/12 00:30:30, gunsch wrote:
> > On 2015/06/11 17:15:00, byungchul wrote:
> > > Why is ChromecastConfigAndroid static in Java and instantiated in c++? Can
> we
> > > interface CastSettingManager directly from c++?
> > > 
> > > It looks like, opt-in.stats is necessary in CrashUploader which is
> initialized
> > > before CastService. Would it make sense make CastSettingsManager a
> singleton?
> > > ChromecastConfigAndroid has only meaning of that, a singleton interface of
> > > CastSettingsManager. Then, we can avoid initializing CastSettingsManager
> > > multiple times.
> > 
> > We could probably simplify this code more if we upstreamed opt-in.stats and
> used
> > that as the source of truth for the UI (e.g. stored it as a Chromium pref
> rather
> > than an Android one). I think Stephen is upstreaming it as part of the crash
> > handling code, and that could be a future cleanup afterwards.
> > 
> > There are other CastSettingsManager instances/listeners besides here and
> > MediaShellCastReceiverService.
> 
> Right, we should use chromecast preference for ATV settings ideally. Can you
> leave a TODO for that?

Done. (will upload after addressing other comments, would like your feedback
there)

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
File
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java
(right):

https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java:18:
private static CastSettingsManager sSettingsManager;
On 2015/06/12 05:57:00, byungchul wrote:
> On 2015/06/12 00:30:30, gunsch wrote:
> > On 2015/06/11 17:15:00, byungchul wrote:
> > > So, multiple SettingsManager is created here and in
> > > MediaShellCastReceiverService.java. Why not pass SettingsManager instance
in
> > > CastBrowserHelper.initializeBrowser()?
> > 
> > Each CastSettingsManager instance can provide a listener for different
things.
> > Why make it a singleton? I don't see a lot of upside, and there are the
usual
> > reasons to avoid singletons.
> > 
> > We could definitely rewrite CastSettingsManager to be able to register
> multiple
> > listeners instead of just taking one in the ctor. Do you think that would be
> > cleaner?
> > 
> > Even in that case it would probably still make sense for the render process
to
> > create its own CastSettingsManager instance.
> 
> Once we merged preference and CastSettingsManager, api could be changed a lot.
> Let's see how it goes.
> 
> By the way, is it really necessary for renderer process to access
> CastSettingsManager? It looks like to call canSendUsageStats(). Can't it just
> return true for renderer process since browser process removes dump files? Or,
> passing a command line flag?

No, it can't just return true for renderer.

Value can change over time, so I'd rather use the source of truth when it's
available. Breakpad calls into asking if upload is allowed before writing out
the crash dump. You could imagine a (unlikely, but possible) case where the
crash dump is written while collecting usage data is off, then the user turns
usage data on before the crash uploader runs again.

What API do you imagine using? Are you proposing something like a single
instance per process where listeners can be added/removed, e.g.

// Only one of these may be called per process
public static CastSettingsManager getPrivilegedSettingsManager(Context);
public static CastSettingsManager getSandboxedSettingsManager(Context);

public void addListener(Listener)
public void removeListener(Listener)

[byungchul, 2015-06-12 17:08:04]

On 2015/06/12 16:54:52, gunsch wrote:
>
https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
> File chromecast/base/chromecast_config_android.cc (right):
> 
>
https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
> chromecast/base/chromecast_config_android.cc:49:
> ChromecastConfigAndroid::GetInstance()->
> On 2015/06/12 05:57:00, byungchul wrote:
> > On 2015/06/12 00:30:30, gunsch wrote:
> > > On 2015/06/11 17:15:00, byungchul wrote:
> > > > Why is ChromecastConfigAndroid static in Java and instantiated in c++?
Can
> > we
> > > > interface CastSettingManager directly from c++?
> > > > 
> > > > It looks like, opt-in.stats is necessary in CrashUploader which is
> > initialized
> > > > before CastService. Would it make sense make CastSettingsManager a
> > singleton?
> > > > ChromecastConfigAndroid has only meaning of that, a singleton interface
of
> > > > CastSettingsManager. Then, we can avoid initializing CastSettingsManager
> > > > multiple times.
> > > 
> > > We could probably simplify this code more if we upstreamed opt-in.stats
and
> > used
> > > that as the source of truth for the UI (e.g. stored it as a Chromium pref
> > rather
> > > than an Android one). I think Stephen is upstreaming it as part of the
crash
> > > handling code, and that could be a future cleanup afterwards.
> > > 
> > > There are other CastSettingsManager instances/listeners besides here and
> > > MediaShellCastReceiverService.
> > 
> > Right, we should use chromecast preference for ATV settings ideally. Can you
> > leave a TODO for that?
> 
> Done. (will upload after addressing other comments, would like your feedback
> there)
> 
>
https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
> File
>
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java
> (right):
> 
>
https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
>
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java:18:
> private static CastSettingsManager sSettingsManager;
> On 2015/06/12 05:57:00, byungchul wrote:
> > On 2015/06/12 00:30:30, gunsch wrote:
> > > On 2015/06/11 17:15:00, byungchul wrote:
> > > > So, multiple SettingsManager is created here and in
> > > > MediaShellCastReceiverService.java. Why not pass SettingsManager
instance
> in
> > > > CastBrowserHelper.initializeBrowser()?
> > > 
> > > Each CastSettingsManager instance can provide a listener for different
> things.
> > > Why make it a singleton? I don't see a lot of upside, and there are the
> usual
> > > reasons to avoid singletons.
> > > 
> > > We could definitely rewrite CastSettingsManager to be able to register
> > multiple
> > > listeners instead of just taking one in the ctor. Do you think that would
be
> > > cleaner?
> > > 
> > > Even in that case it would probably still make sense for the render
process
> to
> > > create its own CastSettingsManager instance.
> > 
> > Once we merged preference and CastSettingsManager, api could be changed a
lot.
> > Let's see how it goes.
> > 
> > By the way, is it really necessary for renderer process to access
> > CastSettingsManager? It looks like to call canSendUsageStats(). Can't it
just
> > return true for renderer process since browser process removes dump files?
Or,
> > passing a command line flag?
> 
> No, it can't just return true for renderer.
> 
> Value can change over time, so I'd rather use the source of truth when it's
> available. Breakpad calls into asking if upload is allowed before writing out
> the crash dump. You could imagine a (unlikely, but possible) case where the
> crash dump is written while collecting usage data is off, then the user turns
> usage data on before the crash uploader runs again.
> 
> What API do you imagine using? Are you proposing something like a single
> instance per process where listeners can be added/removed, e.g.
> 
> // Only one of these may be called per process
> public static CastSettingsManager getPrivilegedSettingsManager(Context);
> public static CastSettingsManager getSandboxedSettingsManager(Context);
> 
> public void addListener(Listener)
> public void removeListener(Listener)

How does chromecast's renderer process while opt-in.stats are in prefservice? Is
prefservice available in renderer process?

[gunsch, 2015-06-12 17:12:21]

On 2015/06/12 17:08:04, byungchul wrote:
> On 2015/06/12 16:54:52, gunsch wrote:
> >
>
https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
> > File chromecast/base/chromecast_config_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/1180633002/diff/2/chromecast/base/chromecast_...
> > chromecast/base/chromecast_config_android.cc:49:
> > ChromecastConfigAndroid::GetInstance()->
> > On 2015/06/12 05:57:00, byungchul wrote:
> > > On 2015/06/12 00:30:30, gunsch wrote:
> > > > On 2015/06/11 17:15:00, byungchul wrote:
> > > > > Why is ChromecastConfigAndroid static in Java and instantiated in c++?
> Can
> > > we
> > > > > interface CastSettingManager directly from c++?
> > > > > 
> > > > > It looks like, opt-in.stats is necessary in CrashUploader which is
> > > initialized
> > > > > before CastService. Would it make sense make CastSettingsManager a
> > > singleton?
> > > > > ChromecastConfigAndroid has only meaning of that, a singleton
interface
> of
> > > > > CastSettingsManager. Then, we can avoid initializing
CastSettingsManager
> > > > > multiple times.
> > > > 
> > > > We could probably simplify this code more if we upstreamed opt-in.stats
> and
> > > used
> > > > that as the source of truth for the UI (e.g. stored it as a Chromium
pref
> > > rather
> > > > than an Android one). I think Stephen is upstreaming it as part of the
> crash
> > > > handling code, and that could be a future cleanup afterwards.
> > > > 
> > > > There are other CastSettingsManager instances/listeners besides here and
> > > > MediaShellCastReceiverService.
> > > 
> > > Right, we should use chromecast preference for ATV settings ideally. Can
you
> > > leave a TODO for that?
> > 
> > Done. (will upload after addressing other comments, would like your feedback
> > there)
> > 
> >
>
https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
> > File
> >
>
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1180633002/diff/2/chromecast/base/java/src/or...
> >
>
chromecast/base/java/src/org/chromium/chromecast/base/ChromecastConfigAndroid.java:18:
> > private static CastSettingsManager sSettingsManager;
> > On 2015/06/12 05:57:00, byungchul wrote:
> > > On 2015/06/12 00:30:30, gunsch wrote:
> > > > On 2015/06/11 17:15:00, byungchul wrote:
> > > > > So, multiple SettingsManager is created here and in
> > > > > MediaShellCastReceiverService.java. Why not pass SettingsManager
> instance
> > in
> > > > > CastBrowserHelper.initializeBrowser()?
> > > > 
> > > > Each CastSettingsManager instance can provide a listener for different
> > things.
> > > > Why make it a singleton? I don't see a lot of upside, and there are the
> > usual
> > > > reasons to avoid singletons.
> > > > 
> > > > We could definitely rewrite CastSettingsManager to be able to register
> > > multiple
> > > > listeners instead of just taking one in the ctor. Do you think that
would
> be
> > > > cleaner?
> > > > 
> > > > Even in that case it would probably still make sense for the render
> process
> > to
> > > > create its own CastSettingsManager instance.
> > > 
> > > Once we merged preference and CastSettingsManager, api could be changed a
> lot.
> > > Let's see how it goes.
> > > 
> > > By the way, is it really necessary for renderer process to access
> > > CastSettingsManager? It looks like to call canSendUsageStats(). Can't it
> just
> > > return true for renderer process since browser process removes dump files?
> Or,
> > > passing a command line flag?
> > 
> > No, it can't just return true for renderer.
> > 
> > Value can change over time, so I'd rather use the source of truth when it's
> > available. Breakpad calls into asking if upload is allowed before writing
out
> > the crash dump. You could imagine a (unlikely, but possible) case where the
> > crash dump is written while collecting usage data is off, then the user
turns
> > usage data on before the crash uploader runs again.
> > 
> > What API do you imagine using? Are you proposing something like a single
> > instance per process where listeners can be added/removed, e.g.
> > 
> > // Only one of these may be called per process
> > public static CastSettingsManager getPrivilegedSettingsManager(Context);
> > public static CastSettingsManager getSandboxedSettingsManager(Context);
> > 
> > public void addListener(Listener)
> > public void removeListener(Listener)
> 
> How does chromecast's renderer process while opt-in.stats are in prefservice?
Is
> prefservice available in renderer process?

Good point, that would not be available. 

I took a closer to look to see what Chrome does---they only invoke
GetCollectStatsConsent in the browser process. So trying to have
CastSettingsManager in the renderer process probably becomes a moot point.

[gunsch, 2015-06-12 23:52:01]

I've made a few small changes, mostly around removing the renderer path and
adding some TODOs. I think I'd still like to fix the underlying bug that spawned
this for the time being, but I agree that we should get rid of
ChromecastConfigAndroid entirely and have CastSettingsManager proxy the changes
from CastSettingsActivity to the native pref service instead. I'll do that after
Stephen's change lands. PTAL for this in the meantime.

[byungchul, 2015-06-15 17:36:25]

lgtm

[gunsch, 2015-06-15 17:51:52]

The CQ bit was checked by gunsch@chromium.org

[gunsch, 2015-06-15 17:51:52]

The patchset sent to the CQ was uploaded after l-g-t-m from lcwu@chromium.org,
slan@chromium.org, byungchul@chromium.org
Link to the patchset: https://codereview.chromium.org/1180633002/#ps50001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-15 17:52:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180633002/50001

[commit-bot: I haz the power, 2015-06-15 18:25:00]

Committed patchset #4 (id:50001)

[commit-bot: I haz the power, 2015-06-15 18:26:01]

Patchset 4 (id:??) landed as
https://crrev.com/1863f66a9edf3cc220ad7159b2090d9556130df6
Cr-Commit-Position: refs/heads/master@{#334417}