1215353 - StrictModeViolation in TabReceivedBroadcastReceiver

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I get a violation for commit of the shared preferences @ the end of onReceive. Since we need to wait for this write to finish in order to return from onReceive, our options to remove this warning are:
  * Suppress it
  * Start a background service (e.g. IntentService?) to handle this interaction instead

I think the IntentService sounds more proper since we can keep disk write in the background.

Nick, you reviewed my previous patch in bug 1208268 – what do you think?

Comment 1

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #0)
> I get a violation for commit of the shared preferences @ the end of
> onReceive. Since we need to wait for this write to finish in order to return
> from onReceive, our options to remove this warning are:
>   * Suppress it
>   * Start a background service (e.g. IntentService?) to handle this
> interaction instead
> 
> I think the IntentService sounds more proper since we can keep disk write in
> the background.

Gah!  Yeah, bounce the broadcast immediately to an IntentService.  I hate that BR is not on the background immediately.  What a crock.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Let's land this before merge.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1215353 - Move TabReceivedBroadcastReceiver notifications to IntentService. r=nalexander

Bug 1215353 - Move TabReceivedBroadcastReceiver notifications to IntentService. r=nalexander

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8677020 [details]
MozReview Request: Bug 1215353 - Move TabReceivedBroadcastReceiver notifications to IntentService. r=nalexander

https://reviewboard.mozilla.org/r/22859/#review20371

::: mobile/android/base/sync/TabReceivedService.java:22
(Diff revision 1)
>   * A BroadcastReceiver that displays a notification for a tab sent to this device.

nit: update comment.

::: mobile/android/base/sync/TabReceivedService.java:29
(Diff revision 1)
> -    private static final String LOGTAG = TabReceivedBroadcastReceiver.class.getSimpleName();
> +    private static final String LOGTAG = TabReceivedService.class.getSimpleName();

I think we always prefix with `Gecko` for such things.

::: mobile/android/base/sync/TabReceivedService.java:77
(Diff revision 1)
>          // BroadcastReceivers do not continue async methods

nit: update comment.  Is this still relevant?  I thought `.apply()` would schedule the write; are you concerned that the process would be killed before the write occurs?  It's very unlikely, since services are long-lived.

If it works for you, it works for me!

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/22859/#review20371

> nit: update comment.  Is this still relevant?  I thought `.apply()` would schedule the write; are you concerned that the process would be killed before the write occurs?  It's very unlikely, since services are long-lived.

> nit: update comment.  Is this still relevant?

After analysis, no it is not.

> I thought .apply() would schedule the write; are you concerned that the process would be killed before the write occurs?

Yes – but as it turns out, `SharedPreferencesImpl.apply` [adds the work to the `QueuedWork` class](http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/java/android/app/SharedPreferencesImpl.java#369), which, [according to the comments](http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/java/android/app/QueuedWork.java#23) (I didn't vet all the code) will wait for the write to finish before killing the process.

> It's very unlikely, since services are long-lived.

Not true of `IntentService`s – they run their queued work and stop themselves if no other intents are received.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/22859/#review20371

> > nit: update comment.  Is this still relevant?
> 
> After analysis, no it is not.
> 
> > I thought .apply() would schedule the write; are you concerned that the process would be killed before the write occurs?
> 
> Yes – but as it turns out, `SharedPreferencesImpl.apply` [adds the work to the `QueuedWork` class](http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/java/android/app/SharedPreferencesImpl.java#369), which, [according to the comments](http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/java/android/app/QueuedWork.java#23) (I didn't vet all the code) will wait for the write to finish before killing the process.
> 
> > It's very unlikely, since services are long-lived.
> 
> Not true of `IntentService`s – they run their queued work and stop themselves if no other intents are received.

Worth noting that I also tested this hypothesis in a test application, where I started an IntentService from an activity that immediately finishes, incremented a value in the service, called apply as the last action in the IntentService, and checked to ensure the increment persisted to the following run – it did.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/b404b452db2bbc78f132e5d265c18441ec137b9a
Bug 1215353 - Move TabReceivedBroadcastReceiver notifications to IntentService. r=nalexander

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b404b452db2b