Closed Bug 841736 Opened 7 years ago Closed 6 years ago

Alarm API - Delete Alarm API data on app uninstallation (and clear data?)

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: mounir, Assigned: swong15)

References

Details

(Whiteboard: [good-first-bug])

Attachments

(1 file, 11 obsolete files)

A discussion on the sysapps WG made me realise that we might not take care of deleting the alarms set by an app when we delete the application.

Also, should we delete the alarms when we clear the data of an application?

Note that this is important only because the Alarm API has a |data| attribute when we create an alarm which may leak sensitive data that could be retrieved.
(In reply to Mounir Lamouri (:mounir) from comment #0)
> A discussion on the sysapps WG made me realise that we might not take care
> of deleting the alarms set by an app when we delete the application.

Nice catch!

Each alarm has an attribute |manifestURL| to identify its belonging app. Both in AlarmService and AlarmDB, we should remove the deprecated alarms based on the |manifestURL| when we're aware of an app is uninstalled.

I think adding observers for "webapps-uninstall" should be the time point we want.

> Also, should we delete the alarms when we clear the data of an application?

The same. Add observer to catch "webapps-clear-data" might work.
Blocks: alarm-api
(In reply to Gene Lian [:gene] from comment #1)
> (In reply to Mounir Lamouri (:mounir) from comment #0)
> > Also, should we delete the alarms when we clear the data of an application?
> 
> The same. Add observer to catch "webapps-clear-data" might work.

You should simply listen to 'webapps-clear-data'. It is sent when an apps is uninstalled too.
I'm super busy with the MMS stuff right now. I'll appreciate if someone could please help me take this. This one sounds not a critically blocking bug and should be suitable for a newbie. Happy to be the mentor for solving this bug.
Whiteboard: [good-first-bug]
Component: DOM: Mozilla Extensions → DOM
Bug 851368 can be used as inspiration for somebody who wants to implement this.
I'd love to take a crack at this. Could someone give me an idea as to where to start looking in the codebase?
Sebastian,

dom/alarm/AlarmService is the alarms code.
app-uninstall is a system-wide event fired by the webapps system which can be observed using Services.obs.addObserver.

As I mentioned in comment 4 dom/push/src/PushService.js does something similar and is a good example.
Attached patch First attempt at bug (obsolete) — Splinter Review
Attached is my first attempt at the bug.
Attachment #740011 - Attachment is patch: true
Attachment #740011 - Flags: review?(gene.lian)
@Sebastian:

You should attach a diff and not the file itself.

Something like "hg diff > 841736.patch" and then attach 841736.patch. Ask on #developers if you run into problems.
Comment on attachment 740011 [details] [diff] [review]
First attempt at bug

Clearing the review since this isn't a reviewable patch.
Attachment #740011 - Attachment is obsolete: true
Attachment #740011 - Flags: review?(gene.lian)
Attached patch Attempt at patch v1. (obsolete) — Splinter Review
Added an observer for "app-uninstall" and introduced call to _removeAlarmFromDb. 

Please let me know what I can change. Apologies for the earlier mess up.
Comment on attachment 740162 [details] [diff] [review]
Attempt at patch v1.

Some comments.

1) Indentation is 2 spaces throughout the file, please cleanup the patch for that.

2) An app can install multiple alarms, it seems like the patch is removing only one.

It's a good first attempt, thanks for the patch!
Attachment #740162 - Attachment is patch: true
@Sebastian: once you make the changes, please mark the patch as patch and :gene as the reviewer (r?)
Hi Sebastian, glad to take this review when you're done. Thanks!
We might also want to use 'webapps-clear-data' instead of 'webapps-uninstall' if we want to clear data when the user asks to clear all data for the application. We should probably do that otherwise, if the alarms are associated with an IndexedDB entry, we might end up with alarms coming but the IDB entry would have been deleted.
Summary: Delete Alarm API data on app uninstallation → Delete Alarm API data on app uninstallation (and clear data?)
Blocks: 864945
Hi all, 

I'll be working on this during the weekend. I'm a bogged down with studying for a midterm at the moment.
I can't quite figure out how to remove multiple alarms. Could someone point me in the right direction?
alarmdb.getAll() might be what you are looking for. you'll get the manifest URL using the appid from the data component of the uninstall event.
Component: DOM → DOM: Device Interfaces
Summary: Delete Alarm API data on app uninstallation (and clear data?) → Alarm API - Delete Alarm API data on app uninstallation (and clear data?)
Attached patch Attempt at patch v2. (obsolete) — Splinter Review
Changed the event to "webapps-uninstall" as per suggestion and called _db.getAll to remove alarms in a manner similar to _restoreAlarmsFromDB() function. Please let me know what I should change.
Attachment #740162 - Attachment is obsolete: true
Attachment #747267 - Flags: review?
Attachment #747267 - Flags: review? → review?(gene.lian)
Comment on attachment 747267 [details] [diff] [review]
Attempt at patch v2.

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

I think that's close, but I'll Gene check the alarm api details. Thanks!

::: dom/alarm/AlarmService.jsm
@@ +66,5 @@
>    },
>  
> +  observe: function(subject, topic, data) {
> +    data = JSON.parse(data);
> +    data.mm = subject; 

You don't need that.

@@ +67,5 @@
>  
> +  observe: function(subject, topic, data) {
> +    data = JSON.parse(data);
> +    data.mm = subject; 
> +    

Nit: trailing whitespace.

@@ +69,5 @@
> +    data = JSON.parse(data);
> +    data.mm = subject; 
> +    
> +    switch(topic) {
> +      case "webapps-uninstall": 

Nit: trailing whitespace

@@ +71,5 @@
> +    
> +    switch(topic) {
> +      case "webapps-uninstall": 
> +      this._db.getAll(
> +        data.manifestURL, 

Nit: trailing whitespace

@@ +75,5 @@
> +        data.manifestURL, 
> +        function getAllSuccessCb(aAlarms) {
> +          aAlarms.forEach(function addAlarm(aAlarm) {
> +            this._removeAlarmFromDb(aAlarm.id, null);
> +          }.bind(this));

No need to bind with forEach. You can just do aAlarms.forEach(function() {...}, this);

You don't need to name the function either (and the other callbacks also). The js engine makes up nice names now.

@@ +78,5 @@
> +            this._removeAlarmFromDb(aAlarm.id, null);
> +          }.bind(this));
> +        }.bind(this),
> +        function getAllErrorCb(aErrorMsg) {
> +

nit: remove blank line
Attachment #747267 - Flags: feedback+
Comment on attachment 747267 [details] [diff] [review]
Attempt at patch v2.

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

Thanks for the patch. Please also follow Fabrice's comments.

::: dom/alarm/AlarmService.jsm
@@ +43,4 @@
>      let alarmHalService = this._alarmHalService = Cc["@mozilla.org/alarmHalService;1"].getService(Ci.nsIAlarmHalService);
>      alarmHalService.setAlarmFiredCb(this._onAlarmFired.bind(this));
>      alarmHalService.setTimezoneChangedCb(this._onTimezoneChanged.bind(this));
> +  Services.obs.addObserver(this, "webapps-uninstall", false);

Two spaces for indention, please.

@@ +65,4 @@
>      this._restoreAlarmsFromDb();
>    },
>  
> +  observe: function(subject, topic, data) {

Please add a name for the anonymous function and let's use a unified naming convention for the parameters within this file:

observe: function observe(aSubject, aTopic, aData) {
  ...
}

@@ +68,5 @@
> +  observe: function(subject, topic, data) {
> +    data = JSON.parse(data);
> +    data.mm = subject; 
> +    
> +    switch(topic) {

Please add one space between "switch" and "(".

@@ +70,5 @@
> +    data.mm = subject; 
> +    
> +    switch(topic) {
> +      case "webapps-uninstall": 
> +      this._db.getAll(

Two spaces for indention and please do the alignment for all the following codes:

switch(topic) {
  case "webapps-uninstall":
    this._db.getAll(

@@ +75,5 @@
> +        data.manifestURL, 
> +        function getAllSuccessCb(aAlarms) {
> +          aAlarms.forEach(function addAlarm(aAlarm) {
> +            this._removeAlarmFromDb(aAlarm.id, null);
> +          }.bind(this));

I think it's fine to add a name for the anonymous function but I'd suggest s/addAlarm/removeAlarm since what you're trying to do is to remove alarm in this callback.

In addition, there is an important issue you didn't cover here. Your codes only remove the alarms in DB but don't clean up the cached queue. I think you have to do:

  this._removeAlarmFromDb(
    aAlarm.id,
    function removeSuccessCb() {...}
  );

where the .removeSuccessCb() is exactly the same as the one at:

  case "AlarmsManager:Remove":
    this._removeAlarmFromDb(
      json.id,
      json.manifestURL,
      function removeSuccessCb() {
        ...
      }.bind(this)
    );
    break;

Please try to refactorise the common part.

@@ +78,5 @@
> +            this._removeAlarmFromDb(aAlarm.id, null);
> +          }.bind(this));
> +        }.bind(this),
> +        function getAllErrorCb(aErrorMsg) {
> +

Please add:

  throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
Attachment #747267 - Flags: review?(gene.lian) → review-
(In reply to Gene Lian [:gene] from comment #21)

> 
> Please add a name for the anonymous function and let's use a unified naming
> convention for the parameters within this file:

We really don't need to name anonymous functions anymore.
(In reply to Fabrice Desré [:fabrice] from comment #22)
> We really don't need to name anonymous functions anymore.

Some reviewers used to tell me it's a kind of coding convention, because the debug log can show the debug message with the belonging function name. If it's an anonymous function, then it only shows the nearest outer function name that isn't anonymous. It helps us to position the debug message. Please correct me if I'm wrong. Thanks!
Hi Sebastian,

Since you are a newbie (I guess), you probably need to know about the Mozilla automatic test. If you've already know, please ignore this message. :)

To make sure your codes don't break any existing functions, you also need to learn how to commit your patches to the Mozilla Try Server [1] to run thorough the automatic tests. You can see your testing results at [2] to make sure everything is green before asking for a check-in (some non-green items might be due to random bugs, which can be ignored). Btw, although you cannot commit the codes to the central for now, you can commit them to the Try Server, where the former needs level-3 permission and the latter only needs level-1 permission. Therefore, please apply for your level-1 permission while you're working on this bug because that application usually takes some time and some paper work to be done [3].

Don't hesitate to ask questions when you're stuck. Thanks!

[1] https://wiki.mozilla.org/Build:TryServer
[2] https://tbpl.mozilla.org/?tree=Try
[3] http://www.mozilla.org/hacking/commit-access-policy/
Attached patch added requested changes (obsolete) — Splinter Review
I've made what I think are the required changes. Could you take a look and tell me if it looks right? I'll request try access Try server soon. Thanks for the help so far.
Attachment #748091 - Flags: feedback?
Attachment #748091 - Flags: feedback? → feedback?(gene.lian)
I think you only need to listen to "webapps-clear-data" here. We send that also when we're uninstalling apps. In most other APIs we're only listening to "webapps-clear-data"
Attachment #748091 - Attachment is patch: true
Comment on attachment 748091 [details] [diff] [review]
added requested changes

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

::: dom/alarm/AlarmService.jsm
@@ +43,4 @@
>      let alarmHalService = this._alarmHalService = Cc["@mozilla.org/alarmHalService;1"].getService(Ci.nsIAlarmHalService);
>      alarmHalService.setAlarmFiredCb(this._onAlarmFired.bind(this));
>      alarmHalService.setTimezoneChangedCb(this._onTimezoneChanged.bind(this));
> +    Services.obs.addObserver(this, "webapps-uninstall", false);

As Jonas' comment #26 and Mounir's comment #2, you should listen to the "webapps-clear-data" observer topic, instead of the "webapps-uninstall".

@@ +70,5 @@
> +
> +    switch (topic) {
> +      case "webapps-uninstall":
> +        this._db.getAll(
> +        data.manifestURL,

2-space indention from here and below, please.

@@ +72,5 @@
> +      case "webapps-uninstall":
> +        this._db.getAll(
> +        data.manifestURL,
> +        function getAllSuccessCb(aAlarms) {
> +          aAlarms.forEach(function addAlarm(aAlarm) {

Please see my comment #21. s/addAlarm/removeAlarm.

@@ +74,5 @@
> +        data.manifestURL,
> +        function getAllSuccessCb(aAlarms) {
> +          aAlarms.forEach(function addAlarm(aAlarm) {
> +            this._removeAlarmFromDb(
> +              aAlarm.id, 

nit: trailing space (TS).

@@ +75,5 @@
> +        function getAllSuccessCb(aAlarms) {
> +          aAlarms.forEach(function addAlarm(aAlarm) {
> +            this._removeAlarmFromDb(
> +              aAlarm.id, 
> +              function removeSuccessCb() {

Please try to refactor the common part in .remove() instead of copying all the codes.

@@ +82,5 @@
> +                  return;
> +                }
> +
> +                let alarmQueue = this._alarmQueue;
> +                if (this._currentAlarm.id != data.id || 

s/data.id/aAlarm.id/

nit: TS.

@@ +85,5 @@
> +                let alarmQueue = this._alarmQueue;
> +                if (this._currentAlarm.id != data.id || 
> +                    this._currentAlarm.manifestURL != data.manifestURL) {
> +                  for (let i = 0; i < alarmQueue.length; i++) {
> +                    if (alarmQueue[i].id == data.id && 

s/data.id/aAlarm.id/

nit: TS.
Attachment #748091 - Flags: feedback?(gene.lian) → feedback-
Attached patch attempt at patch v3. (obsolete) — Splinter Review
Attachment #750874 - Flags: review?
Comment on attachment 750874 [details] [diff] [review]
attempt at patch v3.

refactored common code and added clearAlarm function. I have also submitted my try access application. Please let me know what else I should do.
Attachment #750874 - Flags: review? → review?(gene.lian)
Comment on attachment 750874 [details] [diff] [review]
attempt at patch v3.

Refactored code and added clearAlarm function. I have also completed the try access paperwork. Please let me know if there is anything I need to do.
Attachment #747267 - Attachment is obsolete: true
Attachment #748091 - Attachment is obsolete: true
Just a reminder: please obsolete the deprecated patches when you come up with a new patch.
Comment on attachment 750874 [details] [diff] [review]
attempt at patch v3.

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

In general, the patch looks good. :) Some comments:

1. I'd prefer using _removeAlarm() as a private member function instead of clearAlarm().

2. Please rebase your patch on top of the latest code base, you will get lots of conflicts due to Bug 862322 and Bug 871428. I hope to check the rebased results again before giving you review+. Thanks!

::: dom/alarm/AlarmService.jsm
@@ +43,4 @@
>      let alarmHalService = this._alarmHalService = Cc["@mozilla.org/alarmHalService;1"].getService(Ci.nsIAlarmHalService);
>      alarmHalService.setAlarmFiredCb(this._onAlarmFired.bind(this));
>      alarmHalService.setTimezoneChangedCb(this._onTimezoneChanged.bind(this));
> +    Services.obs.addObserver(this, "webapps-clear-data", false);

You need to remove the observer topic when shutting down. Recently, we just land an uninit() for it (Bug 871428). You need to rebase your codes and add the following line there:

Services.obs.removeObserver(this, "webapps-clear-data");

@@ +72,5 @@
> +      case "webapps-clear-data":
> +        this._db.getAll(
> +          data.manifestURL,
> +          function getAllSuccessCb(aAlarms) {
> +            aAlarms.forEach(function removeAlarm(aAlarms) {

s/aAlarms/aAlarm because you only deal with one alarm in this callback.

@@ +74,5 @@
> +          data.manifestURL,
> +          function getAllSuccessCb(aAlarms) {
> +            aAlarms.forEach(function removeAlarm(aAlarms) {
> +              this._removeAlarmFromDb(
> +                aAlarms.id,

s/aAlarms/aAlarm

@@ +77,5 @@
> +              this._removeAlarmFromDb(
> +                aAlarms.id,
> +                function removeSuccessCb() {
> +                  clearAlarm.bind(this,aAlarms.id,data.manifestURL);
> +                }

Please rewrite it as:

function removeSuccessCb() {
  this.clearAlarm(aAlarm.id, data.manifestURL);
}.bind(this)

Notes:
1. s/aAlarms/aAlarm
2. You need .bind(this), otherwise the |this| in the callback cannot be recognized.
3. It's fine to use func.bind(this, ...) but I'd prefer this.func(...).
4. Please add spaces among parameters.

@@ +78,5 @@
> +                aAlarms.id,
> +                function removeSuccessCb() {
> +                  clearAlarm.bind(this,aAlarms.id,data.manifestURL);
> +                }
> +              )

);

Please add a |;| at the end of a statement.

@@ +79,5 @@
> +                function removeSuccessCb() {
> +                  clearAlarm.bind(this,aAlarms.id,data.manifestURL);
> +                }
> +              )
> +            },this);

Add a space before |this|.

@@ +200,4 @@
>            json.id,
>            json.manifestURL,
>            function removeSuccessCb() {
> +            clearAlarm.bind(this,json.id,json.manifestURL);

this.clearAlarm(json.id, json.manifestURL)

@@ +379,4 @@
>    },
>  }
>  
> +function clearAlarm(id,manifestURL) {

Add spaces among parameters.

Recently, we just had lots of code changes at Bug 862322. Some comments within this function are modified. You need to rebase your patch on top of that.

Also, please s/clearAlarm/_removeAlarm everywhere and put _removeAlarm(...) into the scope of this.AlarmService to make it as a private member function.
Attachment #750874 - Flags: review?(gene.lian)
Attached patch Attempt at patch v4. (obsolete) — Splinter Review
Attachment #751161 - Flags: review?
Attachment #750874 - Attachment is obsolete: true
Comment on attachment 750874 [details] [diff] [review]
attempt at patch v3.

Bug 862322 added a remove function that is called within "AlarmsManager:Remove".
Attachment #751161 - Flags: review? → review?(gene.lian)
Attachment #751161 - Attachment is obsolete: true
Attachment #751161 - Flags: review?(gene.lian)
Attached patch Attempt at patch v4. redone (obsolete) — Splinter Review
The latest version of the code has already refactored the "remove" case code. I also merged the current observe function with my observe function.
Attachment #751172 - Flags: review?(gene.lian)
Comment on attachment 751172 [details] [diff] [review]
Attempt at patch v4. redone

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

Thanks for merging the codes. However, the new patch becomes wrong.

::: dom/alarm/AlarmService.jsm
@@ +58,4 @@
>  
>      alarmHalService.setAlarmFiredCb(this._onAlarmFired.bind(this));
>      alarmHalService.setTimezoneChangedCb(this._onTimezoneChanged.bind(this));
> +    Services.obs.addObserver(this, "webapps-clear-data", false);

Please move this line up to the next line of:

Services.obs.addObserver(this, "profile-change-teardown", false);

which is easier to keep track of all the added observers.

@@ +92,5 @@
> +      case "webapps-clear-data":
> +        this._db.getAll(
> +          data.manifestURL,
> +          function getAllSuccessCb(aAlarm) {
> +            aAlarm.forEach(function removeAlarm(aAlarm) {

Please use |aAlarms| for the getAllSuccessCb and use |aAlarm| for the removeAlarm, since the former is dealing with multiple alarms for latter is dealing with single alarm.

@@ +98,5 @@
> +                aAlarm.id,
> +                function removeSuccessCb() {
> +                  this.remove(aAlarm.id, data.manifestURL);
> +                }.bind(this);
> +              );

This is not right. The .remove(...) has called ._removeAlarmFromDb(...) in itself. You should simply call:

this.remove(aAlarm.id, data.manifestURL);
Attachment #751172 - Flags: review?(gene.lian) → review-
Attached patch added changes (obsolete) — Splinter Review
Attachment #751172 - Attachment is obsolete: true
Attachment #751694 - Flags: review?
Attachment #751694 - Flags: review? → review?(gene.lian)
Hi Sebastian,

The patch looks good. However, it seems this patch wasn't created by HG because I didn't see any HG header on the top of this patch. Regarding how to make HG patch, please refer to [1] to learn how to use HG to create patches for review. I believe you shouldn't have privilege to summit codes for now so you only need to read the sections before "How can I generate a patch for somebody else to check-in for me". To learn more about how to use the HG MQ extension you can look into [2] for more details.

Could you please come back with a new patch created by HG? Thanks!

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2] https://developer.mozilla.org/en-US/docs/Mercurial_Queues
Attached patch Created patch with hg (obsolete) — Splinter Review
Here you go! I think I created the patch correctly. Please let me know if its wrong.
Attachment #751694 - Attachment is obsolete: true
Attachment #751694 - Flags: review?(gene.lian)
Attachment #752032 - Flags: review?(gene.lian)
Comment on attachment 752032 [details] [diff] [review]
Created patch with hg

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

Wait... Have you tested your codes by practically removing an App and see if the AlarmService can receive a notification from that? Please check Webapps.jsm to see how it fires the "webapps-clear-data" notification, which doesn't use the |data|, |subject| instead. Also, you probably need to use .getManifestURLByLocalId() (in AppsService.js) to convert the subject.appId into the manifest URL to do the comparison.

This patch is impossible to work. Sorry, give r- again. :O Btw, the HG header looks good!
Attachment #752032 - Flags: review?(gene.lian) → review-
Hi Fabrice,

We're going to catch the "webapps-clear-data" observer topic in the AlarmService. We need to know that app's manifest URL. However, the observer subject only carries appId. I'm wondering at this moment can we still use .getManifestURLByLocalId() to get its manifest URL when that app has been killed?
Flags: needinfo?(fabrice)
(In reply to Gene Lian [:gene] from comment #41)
> I'm wondering at this moment can we still use
> .getManifestURLByLocalId() to get its manifest URL when that app has been
> killed?
  ^^^^^^ Sorry I mean "uninstalled".
Would it look something like this? 
      case "webapps-clear-data":
        manifestURL = appsService.getManifestURLByLocalId(subject.appId);
        this._db.getAll(
          manifestURL,
          function getAllSuccessCb(aAlarms) {
            aAlarms.forEach(function removeAlarm(aAlarm) {
              this.remove(aAlarm.id, manifestURL);
            }, this);
          }.bind(this),
          function getAllErrorCb(aErrorMsg) {
            throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
          }
        );
Sorry, I didn't realize changing the event would affect the data being passed in and I assumed it would just work. Bad assumptions... Thanks a ton for putting up with me so far!
Correct! but you need to create that appsService first.
(In reply to Gene Lian [:gene] from comment #41)
> Hi Fabrice,
> 
> We're going to catch the "webapps-clear-data" observer topic in the
> AlarmService. We need to know that app's manifest URL. However, the observer
> subject only carries appId. I'm wondering at this moment can we still use
> .getManifestURLByLocalId() to get its manifest URL when that app has been
> killed?

Yes, you can. We remove the app from the registry at the last step of the uninstall process.
Flags: needinfo?(fabrice)
Attached patch Attempt at patch v5. (obsolete) — Splinter Review
Added call to appsService.getManifestURLByLocalId
Attachment #752032 - Attachment is obsolete: true
Attachment #752397 - Flags: review?(gene.lian)
Comment on attachment 752397 [details] [diff] [review]
Attempt at patch v5.

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

Sorry, review- again because this patch couldn't be working. Close to be done!

::: dom/alarm/AlarmService.jsm
@@ +89,5 @@
> +        this.uninit();
> +        break;
> +
> +      case "webapps-clear-data":
> +        let appsService = Cc["@mozilla.org/AppsService;1"];

This is impossible to work. That would be appreciated if you could run you codes first before asking for a review. Please add the following lines on the top of the file:

XPCOMUtils.defineLazyGetter(this, "appsService", function() {
  return Cc["@mozilla.org/AppsService;1"].getService(Ci.nsIAppsService);
});

The lazy getter means the service won't be actually instantiated until the first use.
Attachment #752397 - Flags: review?(gene.lian) → review-
Attached patch current state (obsolete) — Splinter Review
This is what I have so far. I've tried removing the clock and calendar apps and I haven't had any problems. I suspect I might be testing this wrong. Could you tell me how I should go about testing this thing if I'm doing it wrong?
Attachment #752397 - Attachment is obsolete: true
Attachment #753448 - Attachment is patch: true
Attachment #753448 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 753448 [details] [diff] [review]
current state

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

Basically, if you want to test your codes, you can set |const DEBUG = true;| in the

AlarmService.jsm
AlarmDB.jsm
AlarmsManager.js

and use |adb logcat| to check the log.

::: dom/alarm/AlarmService.jsm
@@ +84,5 @@
>  
>      this._restoreAlarmsFromDb();
>    },
>  
> +  observe: function(Subject, Topic, Data) {

Wait...

s/Subject/subject
s/Topic/topic
s/Data/data

Why don't you use your previous patch which didn't have this typo?

@@ +85,5 @@
>      this._restoreAlarmsFromDb();
>    },
>  
> +  observe: function(Subject, Topic, Data) {
> +    switch (Topic) {

s/Topic/topic

@@ +89,5 @@
> +    switch (Topic) {
> +      case "profile-change-teardown":
> +        this.uninit();
> +        break;
> +	  case "webapps-clear-data":

The alignment is bad. Please correct it. Please don't use TABs.

@@ +91,5 @@
> +        this.uninit();
> +        break;
> +	  case "webapps-clear-data":
> +		debug("webapps-clear-data");
> +        manifestURL = appsService.getManifestURLByLocalId(subject.appId);

I just tested your codes. You should do:

let params =
  subject.QueryInterface(Ci.mozIApplicationClearPrivateDataParams);
let manifestURL = appsService.getManifestURLByLocalId(params.appId);

@@ -496,5 @@
> -    switch (aTopic) {
> -      case "profile-change-teardown":
> -        this.uninit();
> -    }
> -  },

Why don't you add your new codes in the existing observe()? Please move your codes to here by extending the switch-case here.
Okay I hope to god this doesn't contain any errors. I tried running the code and I haven't had any issues. Really sorry if it does.
Attachment #753448 - Attachment is obsolete: true
Attachment #754065 - Flags: review?(gene.lian)
Comment on attachment 754065 [details] [diff] [review]
Attempt at patch v6.

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

Great! Please go ahead to land this. :)
Attachment #754065 - Flags: review?(gene.lian) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6a4bbef0f572
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Gene, would be nice to try for approval-b2g18 on this. I don't know the testing field, so if you could fill out the approval request.
Sounds good. I'll do this later.
You need to log in before you can comment on or make changes to this bug.