Closed Bug 1144495 Opened 5 years ago Closed 5 years ago

[Notification] Deleted notifications reappear after restart.

Categories

(Firefox OS Graveyard :: General, defect)

Other
FreeBSD
defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
2.2 S10 (17apr)
Tracking Status
firefox40 --- fixed

People

(Reporter: iwahashi.kentarou.dev, Assigned: kuoe0.tw)

References

Details

Attachments

(1 file, 4 obsolete files)

[issue]
 In our TV, there is a pattern that indelible a Notification from storage.

[Background]
Below I think to be involved in this issue.
  In the FxOS phone, app will continue to live in a suspended state.
  However, In our TV, app will die.
(Marco-san think it might be a bug on FxOS phone as well once that specific app was killed by OOM mechanism.)

[Code]
 Source code related to this matter I think below.
  gecko/b2g/components/AlertsHelper.jsm
   //(detail.type is kDesktopNotificationClose)
   listener.mm.sendAsyncMessage(kMessageAppNotificationReturn, {

 When the above is successful, to reach the following, to erase from storage.
    gecko/b2g/components/AlertsService.jsm
     notificationStorage.delete(listener.manifestURL, listener.dbId);

[success/fail pattern]
(1)After show event, during the app live, sendAsyncMessage (by close event) will succeed.
(2)After show event, during the app died, sendAsyncMessage (by close event) will fail. (NS_ERROR_NOT_INITIALIZED)
     (Perhaps because the app is receiver.)
(3) once, after it was re-start the b2g is, (Regardless of the app life and death,) sendAsyncMessage will succeed.
     (Perhaps because the system is receiver. )
Assignee: nobody → kuoe0
I tried to create a patch.
This patch removes directly in the sending side.

However, I do not have know-how.
Do you have a better how to fix?
Comment on attachment 8580409 [details] [diff] [review]
notificationStorageDeleteForError.patch

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

::: b2g/components/AlertsHelper.jsm
@@ +162,5 @@
>          }
> +	if (detail.type === kDesktopNotificationClose){
> +	  let dbID = uid.substring(uid.indexOf("{"), uid.indexOf("}") + 1);
> +	  notificationStorage.delete(listener.manifestURL, dbID);
> +	}

I think we have cached the topic and gotten the listener already. So we don't need to get the dbID by ourself. Maybe it should be modified to the following lines:

  if (topic === kTopicAlertFinished && listener.dbId) {
    notificationStorage.delete(listener.manifestURL, listener.dbId);
  }

But, I haven't test it yet. If it doesn't work, please let me know!
Thank you for your comment.

- For 'listener.dbID'
 listener.dbID can not be used here.
 listener.dbID is always undefined here.

- For 'topic'
 Because the following are also using the detail.type, I've used the detail.type.
::: b2g/components/AlertsHelper.jsm  L139
      } catch (e) {
        // we get an exception if the app is not launched yet
        if (detail.type !== kDesktopNotificationShow) {
          // excluding the 'show' event: there is no reason a unlaunched app

 In terms of clarity,
 If you modify the topic, I think the above should also be corrected in the topic.
For my modification, I just referred to b2g/components/AlertsService.js L166. I think it is also correct to use 'detail.type'. And it is strange for the undefined 'listener.dbID'. I'll try to figure out why it is undefined. Thanks a lot!
(In reply to iwahashikentarou from comment #3)

> - For 'listener.dbID'
>  listener.dbID can not be used here.
>  listener.dbID is always undefined here.
> 

Spelling mistake I'm sorry.
Correctly is listener.dbId
So, does 'listener.dbId' work correctly now? Or is it still also undefined?
I'm sorry not been described.
listener.dbId is undefined also always here.
In a show event, listener will be registered.
::: b2g/components/AlertsHelper.jsm  L293
    };
    this.registerAppListener(data.uid, listener);
    this.showNotification(data.imageURL, data.title, data.text,
In this case, data.dbId is not registered.
Thanks! So, does your patch work correctly? I'll test it on Firefox phone later.
This patch is tested in our TV.

I'm sorry regarding topic.
As you say, to be used topic.
Had also been used in the following.
::: b2g/components/AlertsHelper.jsm  L163
[Scenario]
If we close a notification of an unlaunched (killed) app in notification bar, it will reappear when we restart Firefox OS.

[Reproduce]
1. Open 'UI tests' app
2. Press 'new Notification()' to enter the UI/newnotification menu
3. Press 'Notification' to create a new notification
4. Long-press home and swipe up the UI tests app to kill it
5. Open notification bar
6. Swipe the notification we created left/right to close the notification
7. Restart Firefox OS
8. The notification we created reappear!

[Analysis & Solution]
At AlertHelper.jsm#137-159, there is no deletion from notificationStorage action like AlertsService.js#167. So, we add the deletion action there and put the dbId to listener. And add the deletion action to delete the notification from notificationStorage.

In addition, when we close the notification again after restart Firefox OS, it won't reappear again after restart Firefox OS again.
Summary: Notification: there is a pattern that indelible a Notification from storage. → [Notification] Deleted notifications reappear after restart.
Attachment #8580409 - Attachment is obsolete: true
(In reply to iwahashikentarou from comment #10)
> Created attachment 8582207 [details] [diff] [review]
> notificationStorageDeleteForError2.patch
> 
> This patch is tested in our TV.
> 
> I'm sorry regarding topic.
> As you say, to be used topic.
> Had also been used in the following.
> ::: b2g/components/AlertsHelper.jsm  L163

Hi iwahashikentarou, this bug is also found on Firefox OS phone. There is the detail in comment #11. So, we need to fix it and land the patch in our codebase. Do you want to go on the review process by yourself? Or let me take over?

If you want to go on the review process by yourself, you maybe modify the patch to fit the coding style in Moziila. The following link is the coding style reference in Mozilla.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
Thank you for many.
I want to let you take over.

I did not know the link that you tell me.
I'm not ready to develop environment of Firefox OS phone.
There is the detail description in Comment #11.

And I also want to modify the line 139 in AlertHelper.jsm. I want to modify the line below

> if (detail.type !== kDesktopNotificationShow) {

to 

> if (topic !== kTopicAlertShow) {

Because other lines use topic to check the type. Should I need to add an another patch to do it?
Attachment #8582207 - Attachment is obsolete: true
Attachment #8584940 - Flags: review?(fabrice)
Sorry, I forgot to attach the try server result. The try server result is below:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=649403fe19b2
Comment on attachment 8584940 [details] [diff] [review]
DeleteNodtificationFromStorage.patch

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

Redirecting to Michael since he wrote the storagenotification code.
Attachment #8584940 - Flags: review?(fabrice) → review?(mhenretty)
Duplicate of this bug: 1065535
Hi Tommy Kuo-san

I'm sorry in late reply.
I agree with comment # 14.
Comment on attachment 8584940 [details] [diff] [review]
DeleteNodtificationFromStorage.patch

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

Works like a charm, thanks for fixing this iwahashikentarou and Tommy!! I have a couple of notes below. Also, we need an integration test for this in Gaia, probably somewhere around here [1]. We can add that in a followup if you like, and I can help out there. Let's also have Alex do a review.

1.) https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/marionette/notification_events_test.js#L150

::: b2g/components/AlertsHelper.jsm
@@ +151,5 @@
>                imageURL: listener.imageURL,
>                lang: listener.lang,
>                dir: listener.dir,
>                id: listener.id,
> +              dbId: listener.dbId,

Do we need this for functionality, or is this just to add parity with how AlertsService.js does it? For the life of me, I can't remember why we need to pass the database id to the system message. I see it was added in bug 930794, but I'm still now sure why. Maybe Alex will know.

@@ +163,5 @@
>          }
>        }
> +      if (topic === kTopicAlertFinished && listener.dbId) {
> +        notificationStorage.delete(listener.manifestURL, listener.dbId);
> +      }

Let's move this above the `gSystemMessenger.sendMessage(kNotificationSystemMessageName` call. As it stands, if an app immediately fetches Notifications on launch (I believe both calendar and email do this), there could be a race between that GET task and the DELETE task invoked here for the Notification DB mechanism.

We should probably move the same `notificationStorage.delete` call in AlertsService.js above the system message, but in that case the app is already running and so there is a less of a change of a GET happening before the DELETE.
Attachment #8584940 - Flags: review?(mhenretty)
Attachment #8584940 - Flags: review?(lissyx+mozillians)
Attachment #8584940 - Flags: review+
Comment on attachment 8584940 [details] [diff] [review]
DeleteNodtificationFromStorage.patch

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

LGTM, but I'm really curious because I'm pretty sure we had the steps in comment 11 working properly in the past.

Can we know which versions are affected ?

::: b2g/components/AlertsHelper.jsm
@@ +151,5 @@
>                imageURL: listener.imageURL,
>                lang: listener.lang,
>                dir: listener.dir,
>                id: listener.id,
> +              dbId: listener.dbId,

I can't find any use of it on Gaia's side either, but I think we decided to expose it as parity to the other argument.
Attachment #8584940 - Flags: review?(lissyx+mozillians) → review+
Johan checked for me, this was reproduced on all branches since we had resend support. So maybe I'm getting confused.
Attached patch Bug-1144495.patch (obsolete) — Splinter Review
I traced the code to understand the mechanism of notification. I found that the `dbID` is unnecessary in both AlertsService.jsm and AlertsHelper.js. Because the `dbId` is only for notificationStorage to identify the notification. After we click (open app) or swipe (close) the notification, I think the notification will always be deleted from notificationStorage. Despite Gaia's side get the `dbId`, the dbId is useless since the notification is disappeared. So I want to delete the line in my patch and also the line 157 in AlertsService.js.

In addition, I think we should use detail.type to determine the type of coming event. Because the detail is the parameter of handleNotificationEvent, so we should use it directly to avoid the confusion. The topic is just used as the message to send to child process message manager. To add parity, I want change the line 163 in AlertsHelper.jsm to use detail.type to determine the type. And I will also update my patch to use detail.type to determine the type.
Attachment #8584940 - Attachment is obsolete: true
Flags: needinfo?(mhenretty)
Flags: needinfo?(lissyx+mozillians)
Attachment #8586720 - Flags: review?(mhenretty)
Attachment #8586720 - Flags: review?(lissyx+mozillians)
Comment on attachment 8586720 [details] [diff] [review]
Bug-1144495.patch

I'm not sure there is any value in using detail.type instead of topic, though. Mainly since it's a default fallback value.

Except this, it looks good to me, but I would really feel better with tests and a try green result.
Flags: needinfo?(lissyx+mozillians)
Attachment #8586720 - Flags: review?(lissyx+mozillians) → review+
Comment on attachment 8586720 [details] [diff] [review]
Bug-1144495.patch

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

Thanks for the patch! I really don't care too much about topic vs. detail.type, but I'd say lets keep it consistent between AlertsService and AlertsHelper.

::: b2g/components/AlertsHelper.jsm
@@ +167,3 @@
>      }
>  
>      // we"re done with this notification

While we are here, might as well fix this double quote with a single quote.
Attachment #8586720 - Flags: review?(mhenretty) → review+
Comment on attachment 8586720 [details] [diff] [review]
Bug-1144495.patch

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

::: b2g/components/AlertsHelper.jsm
@@ +160,5 @@
>              Services.io.newURI(listener.manifestURL, null, null)
>            );
>          }
>        }
> +      if (detail.type === kDesktopNotificationClose && listener.dbId) {

Oh also, don't forget to move this above the system message. In reality the delete will probably happen before the app launch since app launch takes so much longer, but better to be safe and put the delete before app launch here.
Flags: needinfo?(mhenretty)
The try server results are below.

Before applying patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28d39b9e7807

After applying patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd7e86f80563

I think I can label the keyword "checkin-needed."
Update commit message.
Attachment #8586720 - Attachment is obsolete: true
Attachment #8588634 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/319487767756
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S10 (17apr)
You need to log in before you can comment on or make changes to this bug.