963035 - NotificationDB does not close file

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[:gerard-majax]

When we create the notification store, we do not close the file.

Comment 1

[:gerard-majax]

Attachment: 0001-Bug-963035-Make-sure-we-close-the-notification-stora.patch

Please find attached a trivial patch that fixes the issue.

Try build at: https://tbpl.mozilla.org/?tree=Try&rev=440fa5b79643

Comment 2

[Michael Henretty [:mikehenrty][: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?

Comment 3

[:gerard-majax]

(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 :)

Comment 4

[:gerard-majax]

New Try: https://tbpl.mozilla.org/?tree=Try&rev=d07688b68fa8

Comment 5

[:gerard-majax]

Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=d07688b68fa8

Comment 6

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8960899f23d0

Comment 7

[:gerard-majax]

Since I applied this patch I have not been able to reproduce the issue :)

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

Inbound! Thanks for fixing this Alexandre.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8960899f23d0

Comment 10

[:gerard-majax]

While starting to investigate on bug 977593, I noticed that this one is not in 1.3. We need it.

Comment 11

[:gerard-majax]

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

Comment 12

[Jason Smith [:jsmith]]

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

Comment 13

[:gerard-majax]

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.

Comment 14

[Jason Smith [:jsmith]]

If you are asking uplift here, then you need a 1.3+. We don't grant blanket approval anymore without 1.3+.

Comment 15

[:gerard-majax]

requesting 1.3? as it breaks Notification API.

Comment 16

[bhavana bajaj [:bajaj]]

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.

Comment 17

[Gregor Wagner [:gwagner]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a7d130fd633e