1671051 - Events deleted if Calendar window closed before Reminder window

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[void3]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

After upgrading Thunderbird/Lightning from version 68 to version 78, my Calendar events began disappearing.

I discovered that active events are deleted if the Calendar window is closed before the Reminder window.

• How to reproduce the problem:
  Close all instances of Thunderbird.
  Close all other applications (browsers, etc.)
  It is now 2:00pm.
  Open Thunderbird/Lightning/Calendar
  Create a new event for today,
  starting 10 minutes from now (2:10pm),
  ending 15 minutes from now (2:15pm),
  with a reminder 5 minute before it starts (2:05pm):
  New Event:
  Calendar: <default>
  Title: New Event
  Location: <BLANK>
  Category: None
  [UNCHECK] All day Event
  Start: <today's data> 2:10pm
  End: <today's data> 2:15pm
  Repeat: Does not repeat
  Reminder: 5 minutes before
  Description: New event description
  [CHECK] Notify attendees
  [UNCHECK] Separate invitation per attendee
  [UNCHECK] Disallow counter
  Save and Close

  At 2:04pm: (test the starting condition)
  - close Calendar window
  - re-open Calendar
  - "New Event" is still present in Calendar

  At 2:05pm: (Reminder window appears -- ready for 1st test)
  - Reminder window pops up for "New Event"

  At 2:06pm: (Window closing order that DOES NOT loose/delete event)
  SAFE CLOSING ORDER (Reminder followed by Calendar)
  - close Reminder window
  - close Calendar window
  - re-open Calendar window
  - "New Event" is still present in Calendar

  At 2:11pm: (Reminder window re-appears -- ready for 2nd test)
  - Reminder window pops up for "New Event"

  At 2:12pm: (Reverse window closing order that DOES loose/delete event)
  FAIL CLOSING ORDER (Calendar followed by Reminder)
  - close Calendar window <- reversed closing order from 1st test above
  - close Reminder window <- reversed closing order from 1st test above
  - re-open Calendar window
  - "New Event" is lost/deleted from Calendar

So if you close the windows in the wrong order (first Calendar, second Reminder), event(s) will be deleted.

System:
Debian 10 (all upgrades)
Thunderbird version 1:78.3.1-2deb10u2 (stable)
Lightning version 1:78.3.1-2deb10u2 (stable)

Actual results:

Active events are deleted if the Lightning Calendar window is closed before the Reminder window.

Expected results:

Nothing should be deleted.

Comment 1

[Magnus Melin [:mkmelin]]

With calendar window do you mean the main Thunderbird window?

Comment 2

[void3]

(In reply to Magnus Melin [:mkmelin] from comment #1)

With calendar window do you mean the main Thunderbird window?

Yes.
Specifically: Events and Tasks -> [CHECK] Calendar
This sequence creates the Calendar tab.
So when I say "Calendar window", I mean the window within the Calendar tab.

Comment 3

[WaltS48 [:walts48]]

I could not reproduce following the steps as best as I could using 78.3.2 from Thunderbird on Ubuntu 18.04.

When I closed the application after the reminder appeared, the reminder also closed.

The event was still there after I restarted Thunderbird and enabled the Calendar tab.

What desktop are you using with your Debian 10?

I could probably try a LiveUSB version and test it, although it could be a distributions build bug.

Comment 4

[void3]

Thank you for your efforts. Below is some additional information to help you.

I've reproduced this problem on three computers (one laptop and two desktops, two Intel CPUs and one AMD CPU).
All computers are running Debian 10 buster, and
Thunderbird version 1:78.3.1-2deb10u2 (stable)
Lightning version 1:78.3.1-2deb10u2 (stable)

When I downgrade thunderbird/lightning from version 78 to version 68,
the problem disappears (downgrade requires running once: "thunderbird.exe --allow-downgrade").
So the issue is due to the change in version.

To help you to reproduce the problem, below are more details about my Thunderbird/Lightning configuration.

I have separate profiles for my Calendar and Email, so that they can be opened individually.
Here are the contents of "~/.thunderbird/profiles.ini":
[Profile1]
Name=Email
IsRelative=1
Path=<string-1>.Email
Default=1

[Profile0]
Name=Calendar
IsRelative=1
Path=<string-2>.Calendar

[General]
StartWithLastProfile=1
Version=2

[Install<string-3>]
Default=<string-1>.Email

I start the Calendar using a link to "/Desktop/Calendar.desktop".
Here the contents of "/Desktop/Calendar.desktop":
[Desktop Entry]
Name=Calendar
GenericName=thunderbird/Lightning
Exec=thunderbird -no-remote -P "Calendar"
Terminal=false
Type=Application
Icon=office-calendar
Categories=Office;

Please let me know if you need any additional information.

Again, I appreciate your efforts.

Comment 5

[void3]

Hello,
I am using the KDE desktop.
(Sorry, I missed you request for info about my desktop.)

I believe that my instructions are ambiguous on how to re-create the problem.
When I say "close Calendar window", I mean "click the 'x' in the upper right corner of the KDE Thunderbird application window", which closes the Thunderbird application.
When I say "re-open Calendar", I mean "run/restart the Thunderbird application".
When I say "close Reminder window", I mean "click the 'x' in the upper right corner of the KDE Thunderbird Reminder window", which closes the Thunderbird Reminder.

Additional information:
In my "Calendar.desktop" file, I tried removing the "-no-remote" option:
Change
Exec=thunderbird -no-remote -P "Calendar"
to
Exec=thunderbird -P "Calendar"

Unfortunately, the failure still occurs.

Thank you for the help.

Comment 6

[Martin Schröder [:mschroeder]]

This report reminds me of bug 1670455.

Comment 7

[void3]

(In reply to Martin Schröder [:martinschroeder] from comment #6)

This report reminds me of bug 1670455.

Agreed. That's similar to my initial experience (frustrating loss of data -- was this a new "feature"?). I tested for about a week before I found a consistent sequence to cause this failure.

Comment 8

[Bob Wuergler]

Also, concurrent future events (same date/time) do get deleted if

1. the reminder is set to fire, i. e. set reminder < current date,
2. AND some other event setting is changed, e. g. privacy

Example:

Case 1: Create future event 1 and event 2, same date/time, set reminder < current date, change e. g. priority (same change in both events!)
=> both events do get deleted after TB quit and restart

Case 2. Create future event 3 and event 4, same date/time, set reminder < current date, otherwise default or event 3, 4 have different changes)
=> event 3 does not get deleted, event 4 does get deleted

This is likely the same bug, I think. (Not sure if a new bug should be opened, though.)

Version: 1:78.4.2-1~deb10u1 (current TB Calendar in repo of current Debian stable).

Comment 9

[Lasana Murray]

I can't reproduce this. Is it possible for you to produce a screen-recording with fake data void3?

Comment 10

[void3]

Hello Lasana Murray,
I've sent the screen-recording that you requested.

Comment 11

[Egbert Gerber]

I can confirm the issue on Thunderbird 78.6.0 Windows 10 and Thunderbird 78.5.0 on Kubuntu 20.04.1 LTS 64-bit.
It seems to me the event gets deleted after re-opening TB, not upon closing it (the event is briefly visible after starting TB) .

Comment 12

[Lasana Murray]

I'm able to confirm this happens as well.

Comment 13

[Lasana Murray]

This only seems to happen with local calendars so its probably something wrong in CalStorageCalendar.

Edit: Also looks like the item is removed from the sqlite database unlike in bug 1664731.

Comment 14

[Lasana Murray]

Looks like the CalStorageCalendar first deletes events before modifying them, risky.
I hit this error when the event goes away:

JavaScript error: resource:///modules/CalStorageCalendar.jsm, line 266: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [mozIStorageAsyncStatement.newBindingParamsArray]

Comment 15

[Lasana Murray]

On at least one occasion, I was able to cause an error by closing the main window and reminder prompt before initialization completed. This resulted in the event being lost as well. CalStorageCalendar should be using transactions (if supported) before it makes writes to the database, that way abrupt closes would not cause data loss.

Comment 16

[Lasana Murray]

Attachment: bug1671051.patch

This is my patch for this. It's more of a quick fix as data can be lost from this bug.

The alarm dialog attempts to snooze all visible alarms during an "unload" event. While the application is still running, this is not a problem, when this is the last window to be manually closed however, we can hit this bug.

"Snoozing" an alarm involves calling calICalendar.modifyItem(). For the CalStorageCalendar, that underlying operation is async. The CalStorgeCalendar also looks out for the "profile-before-change" event and closes the database connection on receipt. Unfortunately, it does not keep track of whether existing operations are going on and this is where this bug happens.

CalStorageCalendar.modifyItem() first executes some statements to delete the item then executes inserts to create the new version. For some reason that I could not figure out, the last steps to reproduce this bug were enough to allow "profile-before-change" to be observed before modifyItem() is finished, thus closing the connection and causing further statements to fail, and the event is lost.

If you simply create an event with an alarm, wait for it, and close that dialog last, the statements execute before shutdown. It's when you reopen TB and wait for the event alarm, then close, the modifyItem() call is cut short by "profile-before-change".

Normally multiple SQL writes that need to be atomic are done behind transactions to avoid situations like this. So that's one enhancement we should make here if possible. This is the second time I have tracked down a loss of data bug to CalStorageCalendar.

CalStorageCalendar has been difficult to navigate so I'd like to do some other refactoring on it soon.
For now, this should prevent this bug from happening. There may be other bugs lurking related to this but trying to figure them out in one go is tough.

Comment 17

[Geoff Lankow (:darktrojan)]

You can't just not close the database if you don't want to. It still needs to be closed.

An open transaction will be abandoned if the connection is closed, which is good, that's what we want to happen in this case. There definitely should be a transaction wrapped around flushItem. (There was some discussion at the time around whether this was even possible, it appears it is now if it wasn't then.)

Something else we could do here is close the reminder dialog (without attempting to snooze the reminders) automatically when the last main window closes. I've long wanted certain windows to go away when I close the main one, but there's no proper way to do so (yet). SessionStoreManager knows when the last main window is closed, so that could be used to fire an observer service notification.

Comment 18

[Lasana Murray]

You can't just not close the database if you don't want to. It still needs to be closed.

The database is closed, it's just delayed by snoozeAllItems(), only if the alarm dialog appears to be the last to be closed.

(There was some discussion at the time around whether this was even possible, it appears it is now if it wasn't then.)

Is there a bug you can point me to?

Something else we could do here is close the reminder dialog (without attempting to snooze the reminders) automatically when the last main window closes.

I considered this but was not sure what the consequences of that would be.

SessionStoreManager knows when the last main window is closed, so that could be used to fire an observer service notification.

I'm not familiar with this API, what do you suggest?

Comment 19

[Lasana Murray]

Attachment: bug1671051v2.patch

Ok my previous patch won't do. It left a file lock somewhere and I got errors while using the calendar.
This one tries not to snooze all items if no other windows are open, the effect of this change as far as I can tell, is the alarm dialog needing to be explicitly dismissed or it will come up again on restart.

I'm creating another bug to start using transactions in CalStorageCalendar.

Comment 20

[Geoff Lankow (:darktrojan)]

(In reply to Lasana Murray from comment #18)

You can't just not close the database if you don't want to. It still needs to be closed.

The database is closed, it's just delayed by snoozeAllItems(), only if the alarm dialog appears to be the last to be closed.

Sorry, some time passed between reading the patch and writing the comment, I clearly didn't remember all of the details.

What I'm getting at though, is that once you get to profile-before-change, shutdown is happening and won't wait for any reason unless you tell it to. Making it wait is reasonably simple but I don't think we should do it for this. Closing secondary windows when the main window closes is a common behaviour and probably expected by a lot of people.

(There was some discussion at the time around whether this was even possible, it appears it is now if it wasn't then.)

Is there a bug you can point me to?

Bug 501689 is the one in question but I don't think it's a lot of use. Neil and I were working on competing proposals at the same time, it all got a bit messy, and this function got overlooked.

Something else we could do here is close the reminder dialog (without attempting to snooze the reminders) automatically when the last main window closes.

I considered this but was not sure what the consequences of that would be.

Same as your new patch, the reminders come back when you re-open Thunderbird, because they haven't been snoozed. But I think that is something we want to happen.

SessionStoreManager knows when the last main window is closed, so that could be used to fire an observer service notification.

I'm not familiar with this API, what do you suggest?

Actually, forget SessionStoreManager, there is a notification I was unaware of. If you observe mail-unloading-messenger, check if the first argument is the only 3-pane window, and if it is we're quitting. Close the reminders window without snoozing.

Comment 21

[Lasana Murray]

check if the first argument is the only 3-pane window

Not sure I understand this part. I'm still unfamiliar with the intricacies of the 3 pane stuff. Do you mean the window that load messenger.xhtml?
There can be more than one?

Comment 22

[Lasana Murray]

Attachment: bug1671051v3.patch

This seems to do it for now.

Comment 23

[Geoff Lankow (:darktrojan)]

Yes, there can be any number of 3 pane windows (AKA messenger.xhtml). The easiest way to get one is right-click on a folder in the tree and choose Open in New Window.

You should close the window when you the program is exiting. Making the snooze inert but leaving it open is just going to confuse people. Dismiss would still work (which I can kinda see a case for) but choosing a length of time to snooze for would have no effect.

Comment 24

[Lasana Murray]

Attachment: bug1671051v4.patch

Checks whether other mail3pane windows are open before setting the don't snooze flag.

Comment 25

[Geoff Lankow (:darktrojan)]

Comment on attachment 9203754 [details] [diff] [review]
bug1671051v4.patch

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

::: calendar/base/content/dialogs/calendar-alarm-dialog.js
@@ +36,5 @@
> +  {
> +    observe() {
> +      let windows = Array.from(Services.wm.getEnumerator("mail:3pane"));
> +      if (windows.filter(win => !win.closed).length == 0) {
> +        gShutdownDetected = true;

And close the window.

@@ +41,5 @@
> +      }
> +    },
> +  },
> +  "mail-unloading-messenger"
> +);

The observer service needs to hold a weak reference to this observer, otherwise it'll hold onto it (and therefore this window scope and most of the things in it) forever. Hello, memory leak.

Fixing it is easy, add `QueryInterface: ChromeUtils.generateQI(["nsISupportsWeakReference"])` to the observer itself, and `true` as a third argument to `addObserver`.

@@ +171,5 @@
> +    // Avoid snoozing items when the main window has shutdown to prevent
> +    // data loss. See bug 1671051.
> +    if (!gShutdownDetected) {
> +      snoozeAllItems(snoozePref);
> +    }

Just return early at the top of this function. There's nothing else it does that is useful.

Comment 26

[Geoff Lankow (:darktrojan)]

(In reply to Geoff Lankow (:darktrojan) from comment #25)

Fixing it is easy, add QueryInterface: ChromeUtils.generateQI(["nsISupportsWeakReference"]) to the observer
itself, and true as a third argument to addObserver.

Afterthought: something needs to hold a "strong" reference to the observer to prevent it from being garbage collected. Probably the easiest way to do that is to make it a member of the window, ie.:

var mainWindowObserver = { ... };
Services.obs.addObserver(mainWindowObserver, "mail-unloading-messenger");

Comment 27

[Lasana Murray]

Attachment: bug1671051v6.patch

The weak reference approach did not work on the second run. The observer got ignored completely and the original problem happened again.

I used removeObserver() instead to remove the observer before gShutdownDetected is set and in finishWindow().

Comment 28

[Geoff Lankow (:darktrojan)]

Comment on attachment 9204590 [details] [diff] [review]
bug1671051v6.patch

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

Nearly there.

::: calendar/base/content/dialogs/calendar-alarm-dialog.js
@@ +42,5 @@
>  });
>  
> +// XXX: Detect this notification to avoid snoozing items when the main window is
> +// closed. Will close this dialog also. See bug 1671051.
> +Services.obs.addObserver(gShutdownObserver, "mail-unloading-messenger");

This comment should go with the observer to explain what it's for. Remove the XXX and the bug number, we have version control history if we need to find the bug associated with some code.

Adding the observer should go in setupWindow.

@@ +161,5 @@
> +    // Avoid snoozing items when the main window has shutdown to prevent
> +    // data loss. See bug 1671051.
> +    return;
> +  }
> +  Services.obs.removeObserver(gShutdownObserver, "mail-unloading-messenger");

Put this line above the early return and you can remove the same line in the observer.

Comment 29

[Lasana Murray]

Remove the XXX and the bug number, we have version control history if we need to find the bug associated with some code.

Ok but the link searchfox generates for these is quite convenient.

Comment 30

[Lasana Murray]

Attachment: bug1671051v7.patch

Requested changes made.

Comment 31

[Geoff Lankow (:darktrojan)]

Comment on attachment 9204960 [details] [diff] [review]
bug1671051v7.patch

Okay, we got there in the end!

One more minor thing I didn't notice last time (sorry!). We always try to put the QueryInterface member at the top of an object. It's not actually necessary here so you can either move it or remove it.

Comment 32

[Lasana Murray]

Attachment: bug1671051v8.patch

Weird. I was running into an error without the QueryInterface before, seems to have stopped now.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1222ee19839b
Avoid snoozing alarms when the main window has already closed. r=darktrojan

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 9205085 [details] [diff] [review]
bug1671051v8.patch

[Approval Request Comment]
Fix potential dataloss.

Comment 35

[Wayne Mery (:wsmwk)]

Comment on attachment 9205085 [details] [diff] [review]
bug1671051v8.patch

[Triage Comment]
Approved for esr78

Comment 36

[Rob Lemley [:rjl]]

Thunderbird 78.10.2:
https://hg.mozilla.org/releases/comm-esr78/rev/5d2c4714914d