[Datastore] Notify apps of changes in datastore without being opened (comment #21)

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: arcturus, Assigned: selin)

Tracking

({dev-doc-complete})

unspecified
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

Right now we have the |onchange| event to notify when the content of a datastore changed, but that means that the listener app needs to be alive and listening to in order to know those changes happened.

In some cases will be necessary those changes are noticed before the app is open.

An use case for this could be the following one:
- We have several apps (PROVIDER1, PROVIDER2 ...) that declare the a 'contacts' datastore to manage their data.
- We have an application (CONSUMER), that manage data for all 'contacts' datastores in the system (merging information, performing optimizations for fast search, etc.)
- Will be nice that the CONSUMER app will get notified of the changes produced in PROVIDER1, PROVIDER2, etc without being opened, so the data will be fresh and processed once we want to open CONSUMER.

Could we have system messages to perform this?

Also it comes to my mind some possible problems like massive updates (or insert), how we could handle (throttle) those system messages
Summary: [Datastore] → [Datastore] Notify apps of changes in datastore without being opened
That's exactly what the sync() function designed for. Isn't it?

When the app launches, it has to call the sync() function to synchronize data into its local storage during its initialization process. I think this is a no-issue because you might misunderstand the way of using the sync(). Please correct me if it doesn't fit your user case.

Comment 2

5 years ago
Yes, I think you're basically asking for sync() which already exists.  Am I missing something?
Component: General → DOM
Hi folks,

sync will give me the changes from a certain version, right, I'm asking about a system message to know when the changes happend, so we can perform a sync from different source apps.

Imagine that you have to 2 datasources of contacts with 2000 contacts each, if we perform the sync operaion when opening the contacts app it will take a while. But if we have the chance to perform the sync operation when the original ds changes, contacts can be ready to display the mashup information at startup quickly, cause it will be dealing with local data.

Does that make sense for you guys?
The requested functionality has to do with the new contacts data architecture we want to introduce for contacts. In that architecture there will be a system app, called Contacts Manager, that will be listening for changes of all contacts datastores in the device (potentially any external contacts provider (Facebook, Twitter, Gmail, Linkedin ...) could leave the imported contacts in one of those Datastores. It will be the responsibility of the ContactsManager to listen to the changes on those slave datastores to update a "Global Contacts Datastore" that will have a consolidated view of all the Contacts present in the system. And those changes will have to be listened through sys messages, as the Contacts Manager could not be running permanently. Otherwise we will have to move that functionality to the System app and that will not be good. 

In general as Francisco has said, the Sys Msg mechanism will be very powerful as it will save apps from the pain of synchronizing data once they start running, which can have an impact on perceived responsiveness and overal UX.

Comment 5

5 years ago
OK I see, thanks for the explanation.  One more question: do you need to wake up the Contact Manager app after every single change to those contacts databases?  Or would it be enough to run that app off of a mozAlarm once in a while and pick up all of the changes to those data stores?
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #5)
> OK I see, thanks for the explanation.  One more question: do you need to
> wake up the Contact Manager app after every single change to those contacts
> databases?  

Obviously it would a waste to be notified of every single change. Thus, We would expect that the Contact Manager App will be woken up once a significative set of changes have been produced in the target datastores. That could be a policy or a heuristic that could depend on the number of changes or the time ellapsed between two consecutive changes. 

Or would it be enough to run that app off of a mozAlarm once in
> a while and pick up all of the changes to those data stores?

But the problem would be twofold:

a/ There will be state inconsistencies between contact importation operations and the state of the Contacts App itself as the data will be propagated with some delays

b/ A mozAlarm running every n minutes can be a problem in terms of battery comsumption

Comment 7

5 years ago
(In reply to comment #6)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #5)
> > OK I see, thanks for the explanation.  One more question: do you need to
> > wake up the Contact Manager app after every single change to those contacts
> > databases?  
> 
> Obviously it would a waste to be notified of every single change. Thus, We
> would expect that the Contact Manager App will be woken up once a significative
> set of changes have been produced in the target datastores. That could be a
> policy or a heuristic that could depend on the number of changes or the time
> ellapsed between two consecutive changes. 
> 
> Or would it be enough to run that app off of a mozAlarm once in
> > a while and pick up all of the changes to those data stores?
> 
> But the problem would be twofold:
> 
> a/ There will be state inconsistencies between contact importation operations
> and the state of the Contacts App itself as the data will be propagated with
> some delays
> 
> b/ A mozAlarm running every n minutes can be a problem in terms of battery
> comsumption

So what is your suggestion?  I'm not sure how we can reconcile not notifying after every change but notifying at the "right time" at the API level...
In order to notify changes in batches we can implement an algorithm like:

When the datastore suffers a change a timer is run. If the timer ellapses without more changes then the change it is notified through a System Message. If more changes arrive in the middle of the timer then that timer is cancelled and a new timer is run. That will be repeated until no more changes come, the timer ellapses and the sys msg is raised. 

Using this simple approach we can notify in batches instead everytime a change is performed. That will be the difference between the onchange event (very verbose) and the datastore 'change' System Message which will be sent less frequently and will encompass a bigger number of changes. In fact that sys msg will not contain any special info it will only notifying the consumer that a sync operation should be performed.

Comment 9

5 years ago
OK, that sounds reasonable.  To be clear, we're talking about sending a system message notifying the app that there is some change in the data store which can wake the app up, and then the app would call .sync() as usual.  If yes, than that sounds good to me!
Agreed. That's exactly what we want. Apps need to be able to indicate that they are interested in receiving this notification, probably through some API call. And they need to be able to unregister once they no longer are interested in getting the system message.
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #9)
> OK, that sounds reasonable.  To be clear, we're talking about sending a
> system message notifying the app that there is some change in the data store
> which can wake the app up, and then the app would call .sync() as usual.  If
> yes, than that sounds good to me!

YES \o/

Thanks, that feature will be extremely useful.
(In reply to Jonas Sicking (:sicking) from comment #10)
> Agreed. That's exactly what we want. Apps need to be able to indicate that
> they are interested in receiving this notification, probably through some
> API call. And they need to be able to unregister once they no longer are
> interested in getting the system message.

This is already supported by just calling |mozSetMessageHandler(type, null)| as if registering for a null system message handler.
However, Jonas' comment #10 doesn't make sense to me because once the app unregisters the System Message handler and then being manually killed or OOP'ed, it cannot receive that system message anymore to be woken up. Jose's comment #8 sounds more reasonable to me.

To be more customizable, I originally thought we may specify the wake-up frequency in the app's manifest or thorough a new DataStore API like |wakeMeUpWhenXRecordsChange(10)| (i.e., the app will receive the system message whenever 10 records have been changed). However, if I were an app developer, I'd always call this function as |wakeMeUpWhenXRecordsChange(1)| because it doesn't make any harm to me (i.e. more times of updates means the apps will take less time to finish each sync()). So, hard-coding the timer (comment #8) or the x-record wake-up frequency in the Gecko still sounds a reasonable alternative to me.

Btw, regarding the wake-up frequency we may have some choices:

  1. timer
  2. x-record
  3. data-size

If #3 is implementable, then it might be the best solution because it is directly correlated to the overall time period of processing the sync, whereas #1 and #2 are much easier to implement.

Comment 14

5 years ago
Yeah, I think if we give people a way of asking to be woken up for every N changes, the incentive would be to set N to 1, which destroys the battery life because we need to keep waking apps up.  I'd rather we start with something simple and see if it works well before trying the more complicated approaches.  I think starting with notifications every fixed number of changes makes sense.
Just like we should have an API to enables apps to say "wake me up and send me a system message any time that this DataStore changes", we should have an API which says "I'm no longer interested in being woken up when this DataStore changes".

Consider a chat app which uses DataStore to synchronize data for all your friends. Such an app might enable the user to choose which of "Google+ friends", "Twitter friends" and "Facebook friends". If the user chooses to include "Google+ friends", the app should be able to register for system messages whenever the Google+ DataStore changes.

However if the user later unchecks "Google+ friends", then the app should be able to unregister for system messages.

|mozSetMessageHandler(type, null)| does not enable this type of unregistration. If the app isn't running the next time the DataStore changes, the app will still be woken up.


I don't know if any use cases around saying "send me a system message once X records has changed". Unless we know if good use cases for that I don't think we should add that feature.

I think it's fine that we add a policy like "don't send the datastore-changed system message until 10 seconds after a record was changed". That way if an app updates a lot of records in a datastore we can coalesce those change notifications into a single system message.

I don't think apps need to be able to set that timeout. Just having a good default in the API is probably good enough. Let's keep it simple :)
Yeap, we just had the second-round discussion regarding this. We are all in agreement with the above-mentioned *timer* approach. Our plan is summarized as below:

1. If the timer is not running, when a record is changed, Gecko will fire a system message and runs the timer.
2. During the period of the timer, Gecko won't fire system messages for any record changes.
3. Gecko will fire another system message when the timer ends.

Notes:

- In our first step, we won't expose any new APIs for apps to configure the timer unless really needed. Gecko will just set the timer as 10 seconds (the number can be discussed).

- We need step #1 is because we hope the Contacts Manager can start to synchronize immediately (especially when there is only one record changed, we don't want to wait for the timer to trigger that).

- We need step #3 is because we're worried about when the Contacts Manager finishes the synchronization for some records and then closes itself, other new record changes in the middle of timer (step #2) will not trigger the system message to launch the Contacts Manager again. Therefore, we need to fire another system message at the end of the timer as a closer to make sure all the records can be synchronized during the timer.

Hi Jonas and Ehsan, does the plan make sense to you?
Blocks: 916089
Flags: needinfo?(jonas)
Flags: needinfo?(ehsan)

Comment 17

5 years ago
It seems to me like you're trying to design something that notifies Contacts Manager either immediately after a change or "soon enough" if it was just woken up.  That doesn't make much sense to me because:

* If the goal is to send the message immediately (which is what step #1 seems to be about) then why delay sending more of these messages in step #2?  That seems like an inconsistent approach to me where the Contacts Manager sees some messages immediately and some with a delay.

* I thought what we were previously discussing (comment 8) was about a generic solution to wake up apps in the presence of changes in a data store, but the current solution seems to very tied to the needs of Contacts Manager.  I don't think for most use cases we would need to wake the app up immediately when one record changes in the data store, so this doesn't look like a good solution to me for solving the general use case (which is what this bug is about AFAICT.)
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #17)
> It seems to me like you're trying to design something that notifies Contacts
> Manager either immediately after a change or "soon enough" if it was just
> woken up.  That doesn't make much sense to me because:
> 
> * If the goal is to send the message immediately (which is what step #1
> seems to be about) then why delay sending more of these messages in step #2?
> That seems like an inconsistent approach to me where the Contacts Manager
> sees some messages immediately and some with a delay.

The reason why we don't fire system message for step #2 is because the Contacts Manager has been woken up by step #1 and starts synchronizing. All the other changes made during the step #2 will be synchronized by the sync()/onchange APIs, so we only fire one system message for the first record change.

> 
> * I thought what we were previously discussing (comment 8) was about a
> generic solution to wake up apps in the presence of changes in a data store,
> but the current solution seems to very tied to the needs of Contacts
> Manager.

We're finding a generic solution. Contacts Manager is just a example specifically for this bug, which can be considered as just an app. 

> I don't think for most use cases we would need to wake the app up
> immediately when one record changes in the data store, so this doesn't look
> like a good solution to me for solving the general use case (which is what
> this bug is about AFAICT.)

So, according to the comment #8, suppose we have *a great many of consecutive* changes, the system message will only be fired after a certain time period starting from the *last* record change. When the app launches up, it still needs to take plenty of time to synchronize those (huge) changes, which doesn't solve this bug. Right?

This occurs to me a better solution might be a mixed one of (timer + numOfChanges). That is, the system message will be fired when:

  - A timer ellapses without more changes (comment #8) or
  - A certain number of changes have been made to restart the timer.

The mixed solution can cover the following cases:

  - Suppose we have only few record changes. The app will be woken up after the timer ellapses (starting from the last change). It won't be a burden for the app to sync, because the number of records are just few.

  - Suppose we have 1000 (i.e. many) record changes. The app will be woken up whenever DS hits each 100 changes even if the timer is still running. That is, 10 system messages will be fired. This can make sure the app doesn't have too many changes to sync when it launches up.

  - Suppose we have 1001 record changes. The app will be woken up 11 times totally. The first 10 times is because it hits 100 changes; the last time is triggered by the timer for the last record change.

What do you guys think about this solution? :)
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #18)

> So, according to the comment #8, suppose we have *a great many of
> consecutive* changes, the system message will only be fired after a certain
> time period starting from the *last* record change. When the app launches
> up, it still needs to take plenty of time to synchronize those (huge)
> changes, which doesn't solve this bug. Right?
> 
> This occurs to me a better solution might be a mixed one of (timer +
> numOfChanges). That is, the system message will be fired when:
> 
>   - A timer ellapses without more changes (comment #8) or
>   - A certain number of changes have been made to restart the timer.
> 
> The mixed solution can cover the following cases:
> 
>   - Suppose we have only few record changes. The app will be woken up after
> the timer ellapses (starting from the last change). It won't be a burden for
> the app to sync, because the number of records are just few.
> 
>   - Suppose we have 1000 (i.e. many) record changes. The app will be woken
> up whenever DS hits each 100 changes even if the timer is still running.
> That is, 10 system messages will be fired. This can make sure the app
> doesn't have too many changes to sync when it launches up.
> 
>   - Suppose we have 1001 record changes. The app will be woken up 11 times
> totally. The first 10 times is because it hits 100 changes; the last time is
> triggered by the timer for the last record change.
> 
> What do you guys think about this solution? :)

The issue I see with that solution is that the app will need to deal with "reentrant" sys messages as it is very likely that while the app is processing a certain sys msg another one will arrive. And that new one probably might have to be discarded. 

A possible solution could be that the app only processes a limited number of records per sys msg, in the example Gene gave, that would be 100 records.
(In reply to Jose Manuel Cantera from comment #19)
> 
> The issue I see with that solution is that the app will need to deal with
> "reentrant" sys messages as it is very likely that while the app is
> processing a certain sys msg another one will arrive. And that new one
> probably might have to be discarded. 
> 
> A possible solution could be that the app only processes a limited number of
> records per sys msg, in the example Gene gave, that would be 100 records.

Perhaps is not a bad idea, is moving the throttling mechanism to the app that will receive the system messages. Sounds to me like more flexible and not coupled with the idea of the contacts manager. 

The only concern is that we could wake up apps more often, and we could have some race conditions when we decide to kill ourselfs and the system is sending a system message.
The behavior I would prefer is:

1. When an entry is changed, start one timer A to fire in 10 seconds, and another timer B to fire in one minute.
2. If an entry is changed while the A timer is running, reset it to fire in 10 seconds again. Do not reset the B timer.
3. Once A or B fires fire the system message and then cancel both timers.

The goal here is to avoid firing the system message if a lot of changes are happening. It's better to queue up all those changes and enable the syncing app to send a single message to the server. However if an app is constantly updating an DataStore more often than once every 10 seconds, we'd still eventually wake up the syncing app.

I don't think that we should send a system message when the first entry is changed. It doesn't seem important that changes are synced to the server immediately. That will just result in more system messages and more messages to the server.

I also don't think that there's any point in sending a system message after 100 entries have been updated.

Obviously the 10 seconds and 1 minute limits can be tweaked.
Flags: needinfo?(jonas)

Comment 22

5 years ago
My opinion is along the lines of what Jonas has proposed to.  We should expect to tweak the timer timeout values based on experiments/measurements but I doubt we'll need anything more complicated.  Also, agreed on not tying sending the system message to the number of entries being updated.  These types of heuristics break down when the user does something that requires a bulk operation.
(In reply to Jonas Sicking (:sicking) from comment #21)
> The behavior I would prefer is:
> 
> 1. When an entry is changed, start one timer A to fire in 10 seconds, and
> another timer B to fire in one minute.
> 2. If an entry is changed while the A timer is running, reset it to fire in
> 10 seconds again. Do not reset the B timer.
> 3. Once A or B fires fire the system message and then cancel both timers.
> 
> The goal here is to avoid firing the system message if a lot of changes are
> happening. It's better to queue up all those changes and enable the syncing
> app to send a single message to the server. However if an app is constantly
> updating an DataStore more often than once every 10 seconds, we'd still
> eventually wake up the syncing app.
> 
> I don't think that we should send a system message when the first entry is
> changed. It doesn't seem important that changes are synced to the server
> immediately. That will just result in more system messages and more messages
> to the server.

I disagree here. The use case is the following. Imagine a user starts importing contacts from Gmail and the Gmail datastore is being filled. It will be very good that the syncing app (for instance the Contacts App) starts syncing as soon as possible with the Gmail datastore. In this way perceived delay by the user will be minimal. If we wait 10 seconds to start syncing this can yield to a bad user experience and a waste, as the sync process could have started sooner. 

That's why I prefer that apps get notified as soon as possible that there are ongoing changes in the datastore they are listening to. 

> 
> I also don't think that there's any point in sending a system message after
> 100 entries have been updated.
> 
> Obviously the 10 seconds and 1 minute limits can be tweaked.
(In reply to Jose Manuel Cantera from comment #23)
> I disagree here. The use case is the following. Imagine a user starts
> importing contacts from Gmail and the Gmail datastore is being filled. It
> will be very good that the syncing app (for instance the Contacts App)
> starts syncing as soon as possible with the Gmail datastore.

Why?

If the app syncs data to a server, it seems totally fine that the changes aren't synced until a minute later.

If the app syncs data locally, then it will have an opportunity to do so as soon as the user starts the app. Before then the user won't see any differences anyway.

Can you explain the use case that you are worried about? I.e. what application would register for these change messages, and what would it do when woken up?
(In reply to Jonas Sicking (:sicking) from comment #24)
> (In reply to Jose Manuel Cantera from comment #23)
> > I disagree here. The use case is the following. Imagine a user starts
> > importing contacts from Gmail and the Gmail datastore is being filled. It
> > will be very good that the syncing app (for instance the Contacts App)
> > starts syncing as soon as possible with the Gmail datastore.
> 
> Why?
> 
> If the app syncs data to a server, it seems totally fine that the changes
> aren't synced until a minute later.
> 
> If the app syncs data locally, then it will have an opportunity to do so as
> soon as the user starts the app. Before then the user won't see any
> differences anyway.

Makes sense. Obviously, what we're trying to solve is to automatically wake up the app to do the synchronization, which is usually a process that users won't be aware of (by their eyes). So, I agree with Jonas we don't need to worry too much about firing the system message for the first change.

Even if the app is launched manually (before the system message notifies the app), the timer stuff can somehow help us to make sure the app doesn't have too many records to sync most of the time, because some changes have already been sync'ed from time to time.
(In reply to Jonas Sicking (:sicking) from comment #24)
> (In reply to Jose Manuel Cantera from comment #23)
> > I disagree here. The use case is the following. Imagine a user starts
> > importing contacts from Gmail and the Gmail datastore is being filled. It
> > will be very good that the syncing app (for instance the Contacts App)
> > starts syncing as soon as possible with the Gmail datastore.
> 
> Why?
> 
> If the app syncs data to a server, it seems totally fine that the changes
> aren't synced until a minute later.
> 
> If the app syncs data locally, then it will have an opportunity to do so as
> soon as the user starts the app. Before then the user won't see any
> differences anyway.
> 
> Can you explain the use case that you are worried about? I.e. what
> application would register for these change messages, and what would it do
> when woken up?

The use case is the following:

User opens Contacts App. User goes to "Import from Gmail". The Gmail importer app opens. The user selects 100 contacts to import. The Gmail importer starts putting all the data on its datastore. The process finishes. The user goes back to the Contacts App. By that time nothing would have happened. After ten seconds the user will start seeing his imported data. 

That behavior would be really annoying for the end user.
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #25)
> (In reply to Jonas Sicking (:sicking) from comment #24)
> > (In reply to Jose Manuel Cantera from comment #23)
> > > I disagree here. The use case is the following. Imagine a user starts
> > > importing contacts from Gmail and the Gmail datastore is being filled. It
> > > will be very good that the syncing app (for instance the Contacts App)
> > > starts syncing as soon as possible with the Gmail datastore.
> > 
> > Why?
> > 
> > If the app syncs data to a server, it seems totally fine that the changes
> > aren't synced until a minute later.
> > 
> > If the app syncs data locally, then it will have an opportunity to do so as
> > soon as the user starts the app. Before then the user won't see any
> > differences anyway.
> 
> Makes sense. Obviously, what we're trying to solve is to automatically wake
> up the app to do the synchronization, which is usually a process that users
> won't be aware of (by their eyes). So, I agree with Jonas we don't need to
> worry too much about firing the system message for the first change.

I prefer the policy of starting sync as soon as it is possible and not delaying things. That would have sooner or later an impact on the user experience. 

> 
> Even if the app is launched manually (before the system message notifies the
> app), the timer stuff can somehow help us to make sure the app doesn't have
> too many records to sync most of the time, because some changes have already
> been sync'ed from time to time.

I don't understand this statement. Could you elaborate more on that?
Flags: needinfo?(gene.lian)
(In reply to Jose Manuel Cantera from comment #27)
> (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #25)
> > Even if the app is launched manually (before the system message notifies the
> > app), the timer stuff can somehow help us to make sure the app doesn't have
> > too many records to sync most of the time, because some changes have already
> > been sync'ed from time to time.
> 
> I don't understand this statement. Could you elaborate more on that?

Sure. Sorry for confusing you. Let me take an example:

Suppose we're going to have 5000 records changed in DS (each 1000-record takes 1 min for example), according to the comment #21, the B timer will fire a system message in 1 min for the first 1000-record change. If we only have A timer, the app won't be woken up until the last record change (5 mins plus 10 seconds), which means the app has to sync up totally 5000-record changes when it launches up. Compared to the 1000-record sync-up, it takes much longer.
Flags: needinfo?(gene.lian)
Hi Gene!

Any WIP that we could be testing in advance to give feedback?

Thanks a lot!!
Flags: needinfo?(gene.lian)
Yeap, we can start with the solution at comment #21. I'll find someone help me with this bug. Please stay tuned.
Summary: [Datastore] Notify apps of changes in datastore without being opened → [Datastore] Notify apps of changes in datastore without being opened (comment #21)
\o/ thanks Gene!!
Posted patch Patch v1 (obsolete) — Splinter Review
Broadcast system messages for datastore updates.
Also update some mochitests to ensure permissions and preferences are set before modules are loaded.
Assignee: nobody → selin
Attachment #8448468 - Flags: review?(gene.lian)
Comment on attachment 8448468 [details] [diff] [review]
Patch v1

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

The patch is OK but I don't think the permission check is enough. Think about this scenario:

1. we have facebook app and twitter app, both with a 'contacts' datastore.
2. A 3rd app has access to facebook contacts datastore but not twitter.
3. With this patch, this 3rd app will receives notifications when twitter contacts datastore is changed.

I think we should send the notification only to the apps that have permissions to open that particular datastore.
I want to have a feedback from Jonas about this.

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +31,5 @@
> +try {
> +  kOnChangeShortTimeoutSec =
> +    Services.prefs.getIntPref("dom.datastore.onChangeShortTimeoutSec");
> +} catch (e) {
> +  // |getIntPref| throws when the pref is not set.

Why this should happen?

@@ +126,5 @@
> +
> +    // Reset the short timer.
> +    var shortTimer = this.onChangeShortTimers[aStore];
> +    if (!shortTimer) {
> +      shortTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

if (!shortTimer) {
  dump("Some error mesage."); 
  return;
}

@@ +137,5 @@
> +                                Ci.nsITimer.TYPE_ONE_SHOT);
> +
> +    // Set the long timer if necessary.
> +    if (!this.onChangeLongTimers[aStore]) {
> +      var longTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

if (!longTimer) {
  dump("Some error mesage."); 
  return;
}

::: dom/datastore/tests/file_notify_system_message.html
@@ +99,5 @@
> +    },
> +
> +    // Add.
> +    function() { gChangeId = 1; gChangeOperation = 'added';
> +                 testStoreAdd({ number: 42 }, 1); },

you don't need all of these operations... do you? Can you just add a value and remove the put/remove/clear ?

::: dom/datastore/tests/test_bug986056.html
@@ +80,5 @@
>                                           ["dom.testing.ignore_ipc_principal", true],
>                                           ["dom.testing.datastore_enabled_for_hosted_apps", true]]}, runTest);
>      },
>  
>      // Enabling mozBrowser

remove this comment.

::: dom/datastore/tests/test_changes.html
@@ +126,5 @@
>                                           ["dom.testing.ignore_ipc_principal", true],
>                                           ["dom.testing.datastore_enabled_for_hosted_apps", true]]}, runTest);
>      },
>  
>      // Enabling mozBrowser

remove this comment.
Attachment #8448468 - Flags: review?(gene.lian) → review+
Jonas, can you write a comment about this:

Think about this scenario:

1. we have facebook app and twitter app, both with a 'contacts' datastore.
2. A 3rd app has access to the facebook 'contacts' datastore but not the twitter one.
3. With this patch, this 3rd app will receive notifications when twitter contacts datastore is changed.

I think we should send the notification only to the apps that have permissions to open that particular datastore.
Flags: needinfo?(jonas)
(In reply to Andrea Marchesini (:baku) from comment #33)

Some clarifications for the implementation of timer handling below.

> @@ +126,5 @@
> > +
> > +    // Reset the short timer.
> > +    var shortTimer = this.onChangeShortTimers[aStore];
> > +    if (!shortTimer) {
> > +      shortTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> 
> if (!shortTimer) {
>   dump("Some error mesage."); 
>   return;
> }
Actually the null check is to create a timer instance when it comes to the first datastore update since there hasn't be any short timer then. So I don't think we should treat it as an error case.

> @@ +137,5 @@
> > +                                Ci.nsITimer.TYPE_ONE_SHOT);
> > +
> > +    // Set the long timer if necessary.
> > +    if (!this.onChangeLongTimers[aStore]) {
> > +      var longTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> 
> if (!longTimer) {
>   dump("Some error mesage."); 
>   return;
> }
Similar here. There's no long timer before the first datastore update comes. So it's just the case for such scenario instead of an error one.
Comment on attachment 8448468 [details] [diff] [review]
Patch v1

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

::: b2g/app/b2g.js
@@ +878,5 @@
>  pref("dom.datastore.enabled", true);
> +// When an entry is changed, use two timers to fire system messages in a more
> +// moderate pattern.
> +pref("dom.datastore.onChangeShortTimeoutSec", 10);
> +pref("dom.datastore.onChangeLongTimeoutSec", 60);

I don't like the namings which doesn't related to the system message. Maybe:

sysMsgOnChangeShortTimeoutSec
sysMsgOnChangeLongTimeoutSec

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +26,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "systemMessenger",
> +                                   "@mozilla.org/system-message-internal;1",
> +                                   "nsISystemMessagesInternal");
> +
> +var kOnChangeShortTimeoutSec;

s/kOnChangeShortTimeoutSec/kSysMsgOnChangeShortTimeoutSec

@@ +37,5 @@
> +        + e);
> +  kOnChangeShortTimeoutSec = 10;
> +}
> +
> +var kOnChangeLongTimeoutSec;

s/kOnChangeLongTimeoutSec/kSysMsgOnChangeLongTimeoutSec

@@ +54,5 @@
>    messages: [ "DataStore:Changed", "DataStore:RegisterForMessages",
>                "DataStore:UnregisterForMessages",
>                "child-process-shutdown" ],
> +  onChangeShortTimers: {},
> +  onChangeLongTimers: {},

Add some comments to describe the schema about what the two tables will look like. Please refer to the IAC codes.

Also polish the naming.

@@ +117,5 @@
> +  },
> +
> +  // Use the following logic to broadcast system messages in a moderate pattern.
> +  // 1. When an entry is changed, start a short timer and a long timer.
> +  // 2. If an entry is changed while the short timer is running, reset it again.

nit: drop "again".

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +125,5 @@
>  
> +// This table maps system message prefix to permission(s), indicating only
> +// the system messages with specified prefixes granted by the page's permissions
> +// are allowed to be registered or sent to that page. Note the empty permission
> +// set means this type of system message is always permitted.

Add some comments to describe we will check this table only after it's not passed in the SystemMessagePermissionsTable.

@@ +151,5 @@
>      let permNames = SystemMessagePermissionsTable[aSysMsgName];
>      if (permNames === undefined) {
> +      // Try to look up in the prefix table
> +      for (let sysMsgPrefix in SystemMessagePrefixPermissionsTable) {
> +        if (aSysMsgName.indexOf(sysMsgPrefix) != -1) {

s/!= -1/== 0/ to ensure it's the case of prefix.
Attachment #8448468 - Flags: review+
Posted patch Patch v2 (obsolete) — Splinter Review
Polishing based on Andrea's and Gene's comments in their r+ reviews.

More updates may be provided depending on some follow-ups of comment 34.
Attachment #8448468 - Attachment is obsolete: true
Flags: needinfo?(gene.lian)
(In reply to Andrea Marchesini (:baku) from comment #34)
> I think we should send the notification only to the apps that have
> permissions to open that particular datastore.

I agree that would be strongly preferable.
Flags: needinfo?(jonas)
Posted patch Patch v3 (obsolete) — Splinter Review
Adding access control so that only send system messages to the apps which can access the datastore.
Attachment #8450025 - Attachment is obsolete: true
Attachment #8455179 - Flags: review?(gene.lian)
Attachment #8455179 - Flags: review?(amarchesini)
Comment on attachment 8455179 [details] [diff] [review]
Patch v3

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

I'll leave the implementation details of getAppManifestURLsForDataStore to baku. The rest looks great to me. :) r=gene

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +102,5 @@
> +      longTimer.cancel();
> +      delete this.sysMsgOnChangeLongTimers[aStore];
> +    }
> +
> +    // Get all the manifest URLs of the apps which can access the datastore. 

nit: trailing space.

::: dom/datastore/nsIDataStoreService.idl
@@ +11,2 @@
>  
>  [scriptable, uuid(43a731b9-0b5d-400a-8711-8c912c3c3572)]

Please do remember to flip the UUID.

::: dom/messages/SystemMessageInternal.js
@@ +191,5 @@
>      // Give this message an ID so that we can identify the message and
>      // clean it up from the pending message queue when apps receive it.
>      let messageID = gUUIDGenerator.generateUUID().toString();
>  
> +    let pageURLs = [];

Add:

let manifestURL = aManifestURI.spec;

and use |manifestURL| instead of |aManifestURI.spec| in the following codes.

@@ +207,3 @@
>      }
>  
> +    pageURLs.forEach(function(aPageURL){

Add a space before between ")" and "{".

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +152,5 @@
>      debug("getSystemMessagePermissions(): aSysMsgName: " + aSysMsgName);
>  
>      let permNames = SystemMessagePermissionsTable[aSysMsgName];
>      if (permNames === undefined) {
> +      // Try to look up in the prefix table

nit: add a period.
Attachment #8455179 - Flags: review?(gene.lian) → review+
Comment on attachment 8455179 [details] [diff] [review]
Patch v3

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

The getManifestURLsForDatastore is not good enough but the rest is good.

::: dom/datastore/DataStoreChangeNotifier.jsm
@@ +42,5 @@
> +  // and their correspondent timers. The object literal is defined as below:
> +  //
> +  // {
> +  //   "datastore name 1": timer1,
> +  //   "datastore name 2": timer2,

This is not enough because we can have datastore 'X' from different apps.
What about if the key is: "datastore name X" + '-' + "datastore manifest app"

@@ +103,5 @@
> +      delete this.sysMsgOnChangeLongTimers[aStore];
> +    }
> +
> +    // Get all the manifest URLs of the apps which can access the datastore. 
> +    var manifestURLs = dataStoreService.getAppManifestURLsForDataStore(aStore);

this should be: getAppManifestURLsForDataStore(aStore, aOwner);

@@ +109,5 @@
> +    while (enumerate.hasMoreElements()) {
> +      var manifestURL = enumerate.getNext().QueryInterface(Ci.nsISupportsString);
> +      debug("Notify app " + manifestURL + " of datastore updates");
> +      // Send the system message 'datastore-update-{store name}' to all the
> +      // pages for these apps. Without containing any special info, it simply

I think we should send a message like:

systemMessager.sendMessage("datastore-update-" + aStore, { manifestURL: "the manifest of the app that owns this datastore" },
                           null, ...);

@@ +169,5 @@
>  
>      switch (aMessage.name) {
>        case "DataStore:Changed":
>          this.broadcastMessage(aMessage.data);
> +        this.setSystemMessageTimeout(aMessage.data.store);

here you want the store and the owner: this.setSystemMessageTimeout(aMessage.data.store, aMessage.data.owner);

::: dom/datastore/DataStoreService.cpp
@@ +378,5 @@
> +  AssertIsInMainProcess();
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  auto* manifestURLs = static_cast<nsCOMPtr<nsIMutableArray>*>(aUserData);
> +  nsCOMPtr<nsISupportsString> manifestURL(do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID));

manifestURL can be null.

@@ +380,5 @@
> +
> +  auto* manifestURLs = static_cast<nsCOMPtr<nsIMutableArray>*>(aUserData);
> +  nsCOMPtr<nsISupportsString> manifestURL(do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID));
> +  manifestURL->SetData(aInfo->mManifestURL);
> +  (*manifestURLs)->AppendElement(manifestURL, false);

This should not be needed: manifestURLs->AppendElement... should work.

@@ +1109,5 @@
>  }
>  
> +NS_IMETHODIMP
> +DataStoreService::GetAppManifestURLsForDataStore(const nsAString& aName,
> +                                                 nsIArray** aManifestURLs)

read the comments about this in the IDL file.

@@ +1114,5 @@
> +{
> +  ASSERT_PARENT_PROCESS()
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsCOMPtr<nsIMutableArray> manifestURLs = do_CreateInstance(NS_ARRAY_CONTRACTID);

check if manifestURLs is null and return NS_ERROR_FAILURE or something.

@@ +1117,5 @@
> +
> +  nsCOMPtr<nsIMutableArray> manifestURLs = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  HashApp* apps = nullptr;
> +  if (mStores.Get(aName, &apps)) {
> +    apps->EnumerateRead(GetAppManifestURLsEnumerator, &manifestURLs);

I would do: apps->EnumerateRead(GetAppManifestURLsEnumerator, manifestURLs.get());

and in GetAppManifestURLsEnumerator:

auto* manifestURLs = static_cast<nsIMutableArray*>(aUserData);

::: dom/datastore/nsIDataStoreService.idl
@@ +26,5 @@
>  
>    nsISupports getDataStores(in nsIDOMWindow window,
>                              in DOMString name);
>  
> +  nsIArray getAppManifestURLsForDataStore(in DOMString name);

This is not enough. You want to have the manifestURLs of the app that have access to a particular dataStore 'X' owned by a particular 'manifest Y'. So this method should be:

nsIArray getAppManifestURLsFromDataStore(in DOMString name, in DOMString manifestURL);

The reason is this:

3 apps: appX, appTwitter and appFacebook.
AppX has access to the 'contact' datastore of appFacebook but not of the 'contact' datsatore of appTwitter.
appTwitter changes something in its datastore. We should not broadcast the appX.
This method should return an array with 1 manifest: appTwitter.
When appFacebook changes something, this method should return: appFacebook and appX.
Attachment #8455179 - Flags: review?(amarchesini) → review-
Posted patch Patch v4 (obsolete) — Splinter Review
Attachment #8455179 - Attachment is obsolete: true
Attachment #8465224 - Flags: review?(amarchesini)
Comment on attachment 8465224 [details] [diff] [review]
Patch v4

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

::: dom/datastore/DataStoreService.cpp
@@ +1128,5 @@
> +  }
> +
> +  HashApp* apps = nullptr;
> +  if (mStores.Get(aName, &apps)) {
> +    apps->EnumerateRead(GetAppManifestURLsEnumerator, manifestURLs.get());

I just realized that the security model for DataStore is not landed yet. So this manifestURL is not needed at the moment. Sorry for this. I'll review your previous patch again.
Attachment #8465224 - Flags: review?(amarchesini)
Patch3: r=me

Ok, I change my mind. I like the patch 3 because the security model for DataStore doesn't exist yet so we cannot decide which app should have access to which datastore using the manifests.

Just 1 thing:

 systemMessenger.sendMessage("datastore-update-" + aStore,
		{},
		null,
		Services.io.newURI(manifestURL, null, null));

Add the owner as param: { owner: the owner manifest }
Posted patch Patch v5 (obsolete) — Splinter Review
Updating based on previous r+ comments and merging with the latest code base.
Attachment #8465224 - Attachment is obsolete: true
Posted patch Patch v6 (obsolete) — Splinter Review
Merging previous r+ patch with the latest code base.
Attachment #8466077 - Attachment is obsolete: true
Posted patch Patch v7 (obsolete) — Splinter Review
Ensure system messages are enabled before actually sending one to reduce the intermittent test failures.
Attachment #8467486 - Attachment is obsolete: true
Attachment #8468282 - Flags: review?(amarchesini)
Can you give me an interdiff? Thanks!
Flags: needinfo?(selin)
Posted patch Patch v8 (obsolete) — Splinter Review
I just realized I left couple unnecessary lines of code in the test case in last patch. Simply cleaned them up.
Attachment #8468282 - Attachment is obsolete: true
Attachment #8468282 - Flags: review?(amarchesini)
Attachment #8468337 - Flags: review?(amarchesini)
Flags: needinfo?(selin)
(In reply to Andrea Marchesini (:baku) from comment #52)
> Can you give me an interdiff? Thanks!

Would it help if we use Bugzilla's built-in diff between two versions (v6 and v8)?
https://bugzilla.mozilla.org/attachment.cgi?oldid=8467486&action=interdiff&newid=8468337&headers=1

Btw, I could try to generate a separate diff patch if you still feel necessary.
Comment on attachment 8468337 [details] [diff] [review]
Patch v8

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

::: dom/datastore/tests/file_notify_system_message.html
@@ +94,5 @@
> +
> +    // Ensure the system message has fired and no more pending ones.
> +    function() {
> +      // Periodically check whether the system message has fired.
> +      var timer = setInterval(function() {

I guess this is fine because a mochitest is finished() is not called after 5~ mins.
Attachment #8468337 - Flags: review?(amarchesini) → review+
Posted patch Patch v9 (obsolete) — Splinter Review
Attachment #8468337 - Attachment is obsolete: true
The try run for this patch with the latest code base. FYI.
https://tbpl.mozilla.org/?tree=Try&rev=70429916cfe1
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3200ed3305db
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1051164
Backed out for causing bug 1051164.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddcf4f4af7d9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla34 → ---
Posted patch Patch v10 (obsolete) — Splinter Review
Disable the test case for notifying system messages on some platforms since somehow it's timeout prone and causes intermittent noise. We may consider to have a follow-up item, such as bug 1051164, if we plan to re-enable it someday.
Attachment #8469752 - Attachment is obsolete: true
Attachment #8472113 - Flags: review?(amarchesini)
hi,

In order to start using this in Gaia, Please could you put an example of what has to be declared on the manifest and how to deal with the System Message in Javascript code?

thanks
Flags: needinfo?(amarchesini)
> Disable the test case for notifying system messages on some platforms since
> somehow it's timeout prone and causes intermittent noise. We may consider to
> have a follow-up item, such as bug 1051164, if we plan to re-enable it
> someday.

Yes, file a follow up, please. I really would like to know why we have this timeout.
Can you investigate it a bit?
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #64)
> > Disable the test case for notifying system messages on some platforms since
> > somehow it's timeout prone and causes intermittent noise. We may consider to
> > have a follow-up item, such as bug 1051164, if we plan to re-enable it
> > someday.
> 
> Yes, file a follow up, please. I really would like to know why we have this
> timeout.
> Can you investigate it a bit?

I just filed bug 1053662 as a follow-up. 

Based on almost all the failed instances, somehow the test process appears halted for around 5 mins before the test logic in file_notify_system_message.html gets running. So it fails this case since it violates the 5-min grace period, even though the test logic eventually gets resumed and finishes well in couple seconds. This intermittent issue appears only to happen on some platforms. But I'm not sure if there could be some rare scheduling issues on IPC, system messages, or simply the platform.

The log details of one failed instance are provided below.

21:15:13     INFO -  3449 INFO TEST-START | /tests/dom/datastore/tests/test_notify_system_message.html
21:15:13     INFO -  3450 INFO -*- Webapps.jsm : confirmInstall
21:15:13     INFO -  3451 INFO -*- Webapps.jsm : _writeManifestFile
21:15:13     INFO -  3452 INFO -*- Webapps.jsm : Saving /tmp/tmpYvNOjJ/webapps/{55e9f024-9073-4271-ad99-e85b8fc57acc}/manifest.webapp
21:15:13     INFO -  3453 INFO -*- Webapps.jsm : app.origin: http://test
21:15:13     INFO -  3454 INFO -*- Webapps.jsm : Saving /tmp/tmpYvNOjJ/webapps/webapps.json
21:15:13     INFO -  3455 INFO -*- Webapps.jsm : updateAppHandlers: old=null new=[object Object]
21:15:13     INFO -  3456 INFO -*- Webapps.jsm : Saving /tmp/tmpYvNOjJ/webapps/webapps.json
21:15:13     INFO -  3457 INFO ###################################### forms.js loaded
21:15:13     INFO -  3458 INFO ############################### browserElementPanning.js loaded
21:15:13     INFO -  3459 INFO ######################## BrowserElementChildPreload.js loaded
21:20:14     INFO -  3460 INFO dumping last 11 message(s)
21:20:14     INFO -  3461 INFO if you need more context, please use SimpleTest.requestCompleteLog() in your test
21:20:14     INFO -  3462 INFO TEST-PASS | /tests/dom/datastore/tests/test_notify_system_message.html | Message from app: OK getDataStores('foo') returns 1 element
21:20:14     INFO -  3463 INFO TEST-PASS | /tests/dom/datastore/tests/test_notify_system_message.html | Message from app: OK The dataStore.name is foo
21:20:14     INFO -  3464 INFO TEST-PASS | /tests/dom/datastore/tests/test_notify_system_message.html | Message from app: OK The dataStore foo is not in readonly
21:20:14     INFO -  3465 INFO TEST-PASS | /tests/dom/datastore/tests/test_notify_system_message.html | Message from app: OK onChange is set
21:20:14     INFO -  3466 INFO TEST-PASS | /tests/dom/datastore/tests/test_notify_system_message.html | Message from app: OK store.add() is called
21:20:14     INFO -  3467 INFO TEST-PASS | /tests/dom/datastore/tests/test_notify_system_message.html | Message from app: OK System message 'datastore-update-foo' has been received
21:20:14     INFO -  3468 INFO TEST-PASS | /tests/dom/datastore/tests/test_notify_system_message.html | Message from app: OK DataStoreChangeEvent has been received
21:20:14     INFO -  3469 INFO TEST-PASS | /tests/dom/datastore/tests/test_notify_system_message.html | Message from app: OK OnChangeListener is called with data
21:20:14     INFO -  3470 INFO TEST-PASS | /tests/dom/datastore/tests/test_notify_system_message.html | Message from app: OK event.revisionId returns something
21:20:14     INFO -  3471 INFO TEST-PASS | /tests/dom/datastore/tests/test_notify_system_message.html | Message from app: OK OnChangeListener is called with the right ID: 1
21:20:14     INFO -  3472 INFO TEST-PASS | /tests/dom/datastore/tests/test_notify_system_message.html | Message from app: OK OnChangeListener is called with the right operation:added added
21:20:14     INFO -  3473 INFO TEST-UNEXPECTED-FAIL | /tests/dom/datastore/tests/test_notify_system_message.html | Test timed out.
21:20:14     INFO -  TEST-INFO | expected PASS
21:20:15     INFO -  3474 INFO TEST-OK | /tests/dom/datastore/tests/test_notify_system_message.html | took 302483ms
Flags: needinfo?(amarchesini)
Attachment #8472113 - Flags: review?(amarchesini)
Since the latest patch doesn't change any r+ real logic and simply disables one test case on some platforms where the intermittent issue tends to happen, and the try run looks good, it appears less risky to land the patch.

Bug 1053662 has been filed for follow-ups if we plan to re-enable that test case someday.
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Posted patch Patch v11Splinter Review
The patch for bug 1005818 was landed yesterday, and there was a new test case loading DataStoreChangeNotifier.jsm without properly setting |dom.datastore.sysMsgOnChangeShortTimeoutSec| and |dom.datastore.sysMsgOnChangeLongTimeoutSec|. Simply fix it so no more exception happens to that one.
Attachment #8472113 - Attachment is obsolete: true
Attachment #8475642 - Flags: review?(amarchesini)
Attachment #8475642 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/481ecc3747d8
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.