StrictModeViolation in TabReceivedBroadcastReceiver

RESOLVED FIXED in Firefox 44

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 44
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed, fennec44+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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?
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
Let's land this before merge.
Assignee: nobody → michael.l.comella
tracking-fennec: --- → 44+
Created attachment 8677020 [details]
MozReview Request: Bug 1215353 - Move TabReceivedBroadcastReceiver notifications to IntentService. r=nalexander

Bug 1215353 - Move TabReceivedBroadcastReceiver notifications to IntentService. r=nalexander
Attachment #8677020 - Flags: review?(nalexander)
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!
Attachment #8677020 - Flags: review?(nalexander) → review+
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.
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.
https://hg.mozilla.org/integration/fx-team/rev/b404b452db2bbc78f132e5d265c18441ec137b9a
Bug 1215353 - Move TabReceivedBroadcastReceiver notifications to IntentService. r=nalexander
https://hg.mozilla.org/mozilla-central/rev/b404b452db2b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.