Closed Bug 963035 Opened 6 years ago Closed 6 years ago

NotificationDB does not close file

Categories

(Core :: General, defect)

29 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

Attachments

(1 file)

When we create the notification store, we do not close the file.
Please find attached a trivial patch that fixes the issue.

Try build at: https://tbpl.mozilla.org/?tree=Try&rev=440fa5b79643
Attachment #8364295 - Flags: review?(mhenretty)
Comment on attachment 8364295 [details] [diff] [review]
0001-Bug-963035-Make-sure-we-close-the-notification-stora.patch

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

LG. Probably would be more efficient if we didn't explicitly create the file, instead relying on writeAtomic to do it. But this is fine.

Also, the TRY link you posted looks weird. Perhaps TRY is having issues?
Attachment #8364295 - Flags: review?(mhenretty) → review+
(In reply to Michael Henretty [:mhenretty] from comment #2)
> Comment on attachment 8364295 [details] [diff] [review]
> 0001-Bug-963035-Make-sure-we-close-the-notification-stora.patch
> 
> Review of attachment 8364295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LG. Probably would be more efficient if we didn't explicitly create the
> file, instead relying on writeAtomic to do it. But this is fine.
> 
> Also, the TRY link you posted looks weird. Perhaps TRY is having issues?

Yep sorry, I sent my request 10 mins before the Try reset :)
Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=d07688b68fa8
Flags: needinfo?(mhenretty)
Since I applied this patch I have not been able to reproduce the issue :)
Blocks: 963518
Inbound! Thanks for fixing this Alexandre.
Flags: needinfo?(mhenretty)
https://hg.mozilla.org/mozilla-central/rev/8960899f23d0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
While starting to investigate on bug 977593, I noticed that this one is not in 1.3. We need it.
blocking-b2g: --- → 1.3?
Flags: needinfo?(mhenretty)
Comment on attachment 8364295 [details] [diff] [review]
0001-Bug-963035-Make-sure-we-close-the-notification-stora.patch

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Huge, notifications sent with the new API will be more or less broken from a user point of view.
Testing completed: On several devices since it is in gecko 29, it hepled fixing issues on notifications
Risk to taking this patch (and alternatives if risky): low, we are just closing a file when it is needed
String or UUID changes made by this patch: None
Attachment #8364295 - Flags: approval-mozilla-b2g28?
Blocks: 977593
No longer blocks: 977593
Comment on attachment 8364295 [details] [diff] [review]
0001-Bug-963035-Make-sure-we-close-the-notification-stora.patch

a- - see the blocking bug for more details
Attachment #8364295 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28-
blocking-b2g: 1.3? → backlog
Attachment #8364295 - Flags: approval-mozilla-b2g28-
Comment on attachment 8364295 [details] [diff] [review]
0001-Bug-963035-Make-sure-we-close-the-notification-stora.patch

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: New Notification API broken, as soon as user has pending notifications stored, NotificationDB code will throw and not work.
Testing completed: in master since end of january, fixes a lot of bugs
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

I'm renominating, this fix must be in gecko 28 as we ship the new notification API. Without this fix, it's highly broken.
Attachment #8364295 - Flags: approval-mozilla-b2g28?
If you are asking uplift here, then you need a 1.3+. We don't grant blanket approval anymore without 1.3+.
requesting 1.3? as it breaks Notification API.
blocking-b2g: backlog → 1.3?
Waiting for more information here on what notifications cases are exactly impacted here ? Is it something like missing email notfication, voicemail etc ? Reproducibility frequency etc. Once we have that please renominate it back.
blocking-b2g: 1.3? → -
There is no intermittent part here. All the notifications through the new notification API are impacted.
The 2nd notification will fail or the notification can't be removed by the app.
blocking-b2g: - → 1.3+
Attachment #8364295 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
You need to log in before you can comment on or make changes to this bug.