969043 - Log warning if HomeProvider consumer tries saving data outside of sync window

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Data Providers, defect, P2)

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Log warning in save()/deleteAll() calls outside of sync window (r=margaret)

Comment 2

[:Margaret Leibovic]

Comment on attachment 8392894 [details] [diff] [review]
Log warning in save()/deleteAll() calls outside of sync window (r=margaret)

Review of attachment 8392894 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/modules/HomeProvider.jsm
@@ +302,5 @@
>        let db = yield getDatabaseConnection();
>        try {
> +        if (!gWritesAreExpected) {
> +          Cu.reportError("HomeStorage: save() called outside of sync window");
> +        }

I think it might be clearer to move these checks right to the top of save/deleteAll, instead of placing them inside the tasks.

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #2)
> Comment on attachment 8392894 [details] [diff] [review]
> Log warning in save()/deleteAll() calls outside of sync window (r=margaret)
> 
> Review of attachment 8392894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/HomeProvider.jsm
> @@ +302,5 @@
> >        let db = yield getDatabaseConnection();
> >        try {
> > +        if (!gWritesAreExpected) {
> > +          Cu.reportError("HomeStorage: save() called outside of sync window");
> > +        }
> 
> I think it might be clearer to move these checks right to the top of
> save/deleteAll, instead of placing them inside the tasks.

Fair enough, done.

Comment 4

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/3d0d92f2406b

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d0d92f2406b

Comment 6

[:Margaret Leibovic]

I just realized this patch doesn't actually work as expected if the consumer spawns a task within its requestSync callback function, which is what it would do if it's calling save/deleteAll. We need to think of a better way to address this issue...

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #6)
> I just realized this patch doesn't actually work as expected if the consumer
> spawns a task within its requestSync callback function, which is what it
> would do if it's calling save/deleteAll. We need to think of a better way to
> address this issue...

Ouch, you're right. Re-open? File a new bug?

Comment 8

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #7)
> (In reply to :Margaret Leibovic from comment #6)
> > I just realized this patch doesn't actually work as expected if the consumer
> > spawns a task within its requestSync callback function, which is what it
> > would do if it's calling save/deleteAll. We need to think of a better way to
> > address this issue...
> 
> Ouch, you're right. Re-open? File a new bug?

Normally I hate re-opening bugs, but there isn't much going on in this bug, so I think it would be fine to land a follow-up patch here.

I was thinking about this, maybe we'll need to change the API? It's not immediately clear to me how to fix this problem.

Comment 9

[:Margaret Leibovic]

I backed out the original patch here to avoid having these confusing logging messages ride the trains:
https://hg.mozilla.org/integration/fx-team/rev/d6cf23c13dde

Comment 10

[:Margaret Leibovic]

Comment on attachment 8392894 [details] [diff] [review]
Log warning in save()/deleteAll() calls outside of sync window (r=margaret)

Obsoleting, since this didn't work as desired.

Comment 11

[:Margaret Leibovic]

Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release).

Filter on epic-hub-bugs.

Comment 12

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.