Closed Bug 1215353 Opened 4 years ago Closed 4 years ago

StrictModeViolation in TabReceivedBroadcastReceiver

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed
fennec 44+ ---

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(1 file)

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+
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/mozilla-central/rev/b404b452db2b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.