Fix a snackbar crash about leaking window

Fix a snackbar crash about leaking window

Sometimes the DownloadService will call showSnackbars() even when the activity is not visible. To track the activity's visibility, SnackbarManager now needs to know some activity's lifecycle signals. After this CL, all activities managing snackbars should call onStart and onStop of the SnackbarManager.

BUG=553569
Committed: https://crrev.com/91ecf3b9112abce19508faa97a7ef6504c1f22ae
Cr-Commit-Position: refs/heads/master@{#365751}

[Ian Wen, 2015-12-14 05:31:49]

ianwen@chromium.org changed reviewers:
+ newt@chromium.org

[Ian Wen, 2015-12-14 05:31:50]

PTAL

[newt (away), 2015-12-14 19:16:55]

Could you split the logging revert and the actual bug fix into separate CLs, so
it's easier to see what's going on?

Also, what happened to the idea of using onAttachedToWindow() and
onDetachedFromWindow() (or an OnAttachStateChangeListener) to determine when to
dismiss all snackbars and whether snackbars can be added? Using the
attached/detached signal seems like it would encapsulate the logic inside of
SnackbarManager more nicely and make it more robust against activities
forgetting to call SnackbarManager.onStart() or SnackbarManager.onStop(). Did
you consider this option and find that it didn't work or was too messy in
practice?

[Ian Wen, 2015-12-16 10:34:26]

Description was changed from

==========
Revert "Add debug loggings to figure out a snackbar crash"

This reverts commit 83557154975fb266458aa974a0e8c5e02fd57f8c and adds a
correct fix for the crash. After this CL, all activities managing snackbars
should call onStart and onStop on SnackbarManagers.

BUG=553569
==========

to

==========
Fix a snackbar crash about leaking window

Sometimes the DownloadService will call showSnackbars() even when the activity
is not visible. To track the activity's visibility, SnackbarManager now needs to
know some activity's lifecycle signals. After this CL, all activities managing
snackbars should call onStart and onStop of the SnackbarManager.

BUG=553569
==========

[Ian Wen, 2015-12-16 10:42:05]

On 2015/12/14 19:16:55, newt wrote:
> Could you split the logging revert and the actual bug fix into separate CLs,
so
> it's easier to see what's going on?

Done.

> Also, what happened to the idea of using onAttachedToWindow() and
> onDetachedFromWindow() (or an OnAttachStateChangeListener) to determine when
to
> dismiss all snackbars and whether snackbars can be added? Using the
> attached/detached signal seems like it would encapsulate the logic inside of
> SnackbarManager more nicely and make it more robust against activities
> forgetting to call SnackbarManager.onStart() or SnackbarManager.onStop(). Did
> you consider this option and find that it didn't work or was too messy in
> practice?

Hmmm I am not super sure that I got your idea here.

Since popups live on its own window, onAttachedToWindow does not help the
SnackbarManager to get the information about whether the activity is
visible/destroyed. Instead, each time a snackbar is shown, onAttachedToWindow()
will be called on the snackbar's view. And if the snackbar is dismissed later,
onDetachedFromWindow() is called.

Here this crash is caused by a snackbarcontroller trying to show snackbars even
if the Activity is stopped, so the SnackbarManager has to know the activity's
state.

[newt (away), 2015-12-16 23:07:15]

On 2015/12/16 10:42:05, Ian Wen wrote:
> On 2015/12/14 19:16:55, newt wrote:
> > Could you split the logging revert and the actual bug fix into separate CLs,
> so
> > it's easier to see what's going on?
> 
> Done.
> 
> > Also, what happened to the idea of using onAttachedToWindow() and
> > onDetachedFromWindow() (or an OnAttachStateChangeListener) to determine when
> to
> > dismiss all snackbars and whether snackbars can be added? Using the
> > attached/detached signal seems like it would encapsulate the logic inside of
> > SnackbarManager more nicely and make it more robust against activities
> > forgetting to call SnackbarManager.onStart() or SnackbarManager.onStop().
Did
> > you consider this option and find that it didn't work or was too messy in
> > practice?
> 
> Hmmm I am not super sure that I got your idea here.
> 
> Since popups live on its own window, onAttachedToWindow does not help the
> SnackbarManager to get the information about whether the activity is
> visible/destroyed. Instead, each time a snackbar is shown,
onAttachedToWindow()
> will be called on the snackbar's view. And if the snackbar is dismissed later,
> onDetachedFromWindow() is called.
> 
> Here this crash is caused by a snackbarcontroller trying to show snackbars
even
> if the Activity is stopped, so the SnackbarManager has to know the activity's
> state.

I was suggesting using an OnAttachStateChangeListener to detect the activity's
state changes, instead of requiring each SnackbarManageable activity to call
onStart() and onStop(). I wasn't thinking about the fact the snackbar lives in a
separate window. This idea might still work if you registered the
OnAttachStateChangeListener on Activity.getContentView(), but the content view
might change, and it's not clear when the OnAttachStateChangeListener will be
called anyway relative to the stopping/destroying of the activity. In short, I
think the current code is fine :)

[newt (away), 2015-12-16 23:10:06]

lgtm after comment

https://chromiumcodereview.appspot.com/1518373002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkActivity.java
(left):

https://chromiumcodereview.appspot.com/1518373002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkActivity.java:62:
mBookmarkManager.destroy();
add this back

[Ian Wen, 2015-12-17 03:36:28]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-12-17 03:36:28]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1518373002/#ps60001
(title: "fix a bug in EBActivity")

[commit-bot: I haz the power, 2015-12-17 03:37:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1518373002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1518373002/60001

[commit-bot: I haz the power, 2015-12-17 05:25:28]

Description was changed from

==========
Fix a snackbar crash about leaking window

Sometimes the DownloadService will call showSnackbars() even when the activity
is not visible. To track the activity's visibility, SnackbarManager now needs to
know some activity's lifecycle signals. After this CL, all activities managing
snackbars should call onStart and onStop of the SnackbarManager.

BUG=553569
==========

to

==========
Fix a snackbar crash about leaking window

Sometimes the DownloadService will call showSnackbars() even when the activity
is not visible. To track the activity's visibility, SnackbarManager now needs to
know some activity's lifecycle signals. After this CL, all activities managing
snackbars should call onStart and onStop of the SnackbarManager.

BUG=553569
==========

[commit-bot: I haz the power, 2015-12-17 05:25:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-17 05:26:57]

Description was changed from

==========
Fix a snackbar crash about leaking window

Sometimes the DownloadService will call showSnackbars() even when the activity
is not visible. To track the activity's visibility, SnackbarManager now needs to
know some activity's lifecycle signals. After this CL, all activities managing
snackbars should call onStart and onStop of the SnackbarManager.

BUG=553569
==========

to

==========
Fix a snackbar crash about leaking window

Sometimes the DownloadService will call showSnackbars() even when the activity
is not visible. To track the activity's visibility, SnackbarManager now needs to
know some activity's lifecycle signals. After this CL, all activities managing
snackbars should call onStart and onStop of the SnackbarManager.

BUG=553569

Committed: https://crrev.com/91ecf3b9112abce19508faa97a7ef6504c1f22ae
Cr-Commit-Position: refs/heads/master@{#365751}
==========

[commit-bot: I haz the power, 2015-12-17 05:26:58]

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