Closed Bug 1679723 Opened 4 years ago Closed 4 years ago

Add-on calendars not added to composite after restart

Categories

(Calendar :: Internal Components, defect)

Thunderbird 78
defect

Tracking

(thunderbird_esr78 fixed, thunderbird86 wontfix, thunderbird87 fixed)

RESOLVED FIXED
87 Branch
Tracking Status
thunderbird_esr78 --- fixed
thunderbird86 --- wontfix
thunderbird87 --- fixed

People

(Reporter: public, Assigned: public)

References

Details

Attachments

(4 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1673280 +++

A tainted startup cache can prevent events in add-on-calendars from appearing in the composite calendar, although the calendar remains visible. This is different from bug 1673280, which can cause calendars to become 'properly' invisible on restarts (crossed eye in the UI).

STR:

  1. Clean Profile, Thunderbird 78.5.0
  2. Create a new contact with a display name and a birthday in the near future
  3. Install the Birthday Calendar Add-on 1.0 from https://github.com/rsjtdrjgfuzkfg/thunderbird-birthdaycalendar/releases/download/v1.0/birthdaycalendar.xpi
  4. Observe that two calendars were created (for the default address books), and verify that you can see the birthday
  5. Quit Thunderbird by closing its window.
  6. Delete the startup cache file associated with the Thunderbird profile you're testing with: ~/.cache/Thunderbird/*/startupCache/startupCache.8.little on Linux, %LOCALAPPDATA%\Thunderbird\Profiles\*\startupCache\startupCache.8.little on Windows
  7. Start Thunderbird (you should not see changes, the event is still there).
  8. Quit Thunderbird by closing its window.
  9. Start Thunderbird again.

Expected:

The calendar remains marked as visible (no crossed eye icon) and the birthday is visible.

Actual:

The calendar remains marked as visible but the birthday is not visible. Hiding the calendar and making it visible again fixes the issue for one session, but the birthday will again become hidden on restarts.

Workaround:

Invalidating the startup cache fixes the issue: open the error console (Ctrl+Shift+J) and evaluate the statement Services.obs.notifyObservers(null, "startupcache-invalidate", null);, then restart Thunderbird.

I did quite a bit of debugging on this. I'm still not sure how the startup cache works in detail, but I managed to understand the file format sufficiently to build packing / unpacking tools and compare cache contents.

This bug seems related to a combination of startup cache entries starting with xulcache/resource/gre/chrome/calendar/content/

Removing these entries fixes the error in a clean test profile (but the entries are not regenerated, so there could be a performance penalty). Partially removing entries in that area has mixed results. The workaround (startup cache invalidation) removes much more entries, but has otherwise the same effect (most entries are not re-generated!).

Now here's the weird part: the entries in that 'folder' are binary-identical with the cache from my productive installation of Thunderbird, which is not affected by this bug. There are a few binary differences in other cache entries (from a hex editor they seem to have a different order of class members, no idea why), though, and quite a few additional cache entries on each side. Copying every cache entry from my productive profile does not seem to fix the test profile – maybe due to profile specific data in caches.

During these experiments, I was also able to verify that add-on caching is not the issue: selectively clearing cache entries of the add-on does not influence the bug. Furthermore, I have experienced many inconsistent situations where the bug was triggered with a cache state that did not previously trigger it, or vice versa. That could just be a side-effect of directly messing with internal caches, or point in the vague direction of a caching-related timing problem.

I'd appreciate if somebody with more toolkit experience could have a look at this :)

Attached file unpackstartupcache.py

This a rather dirty script to unpack the startup cache files to a folder (requires lz4 command line tools and a somewhat recent python), if somebody wants to have a shot at this.

Attached file packstartupcache.py

This a rather dirty script to re-pack a folder into a startup cache file (requires lz4 command line tools and a somewhat recent python), if somebody wants to have a shot at this.

Attached patch fix against comm-esr78 (obsolete) — Splinter Review

The issue seems to be that composite calendars are created via cal.view.getCompositeCalendar(window), without any mechanism to keep their calendar lists updated. That task is currently handled by calendar-management.js. So composite calendars can get out of sync if calendars are registered / unregistered before something calls loadCalendarManager() in their associated window.

I have no idea why we have separate instances of the main composite calendar for each window, but assuming that was a intentional decision the best fix I came up with is registering an observer for each composite calendar instance. The attached fix is for 78, I'll attach a rebased version for comm-central in the next minutes.

At least in my tests, I can no longer reproduce the issue once the patch is applied.

Assignee: nobody → public
Status: NEW → ASSIGNED
Attachment #9203691 - Flags: review?(geoff)
Attached patch fix against comm-central (obsolete) — Splinter Review

Rebased fix for comm-central.

Attachment #9203692 - Flags: review?(geoff)

The composite calendar is per-window because it is used to control the list of selected calendars. The idea was that you might have a different set of selected calendars per window.

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #7)

The composite calendar is per-window because it is used to control the list of selected calendars. The idea was that you might have a different set of selected calendars per window.

But they all store the visibility information in the same pref (as it is the main composite calendar), or is that a bug? From the interface design I'd have assumed that any scope that needs a different set of visible calendars either uses anonymous composite calendars, or composite calendars with names ("prefPrefix") other than "calendar-main". Imho it would make more sense to have the calendar manager own all named composite calendars (and both keep them updated and enforce a single instance for each prefPrefix).

Either way, it would be good to get a fix merged and (hopefully) uplifted soon, as this bug affects many non-technical users that struggle to apply the cache clearing workaround.

Comment on attachment 9203692 [details] [diff] [review]
fix against comm-central

Nice find and fix.

Attachment #9203692 - Flags: review?(geoff) → review+

Comment on attachment 9203691 [details] [diff] [review]
fix against comm-esr78

This is identical to the other patch apart from a first line of context, and doesn't really need review.

Attachment #9203691 - Flags: review?(geoff) → review+

Fixed patch to make the linter happy. Sorry about that!

Attachment #9203692 - Attachment is obsolete: true
Attachment #9204102 - Flags: review?(geoff)

Comment on attachment 9204102 [details] [diff] [review]
fix against comm-central, v2

You can carry forward reviews for simple changes like this one.

Attachment #9204102 - Flags: review?(geoff) → review+

Update for the patch against comm-esr with the same linter change

Attachment #9203691 - Attachment is obsolete: true
Attachment #9204105 - Flags: review+

Comment on attachment 9204105 [details] [diff] [review]
fix against comm-esr78, v2

[Approval Request Comment]
User impact if declined: Users with add-on calendars might feel like they lost all events; users that already know the bug need to continue to re-apply the workaround after each update of Thunderbird
Testing completed (on c-c, etc.): tested local builds of comm-central and com-esr78 with the patch applied; bug is gone and the observer seems to get cleanly registered/unregistered. I found the related bug 1693713, though (present independently of applying the patch)...
Risk to taking this patch (and alternatives if risky): seems not really risky to me, as no core logic was changed

Attachment #9204105 - Flags: approval-comm-esr78?

I'm about to land this. Just waiting for m-c.

Target Milestone: --- → 87 Branch

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/9d7dc409eccd
Add observer for each main composite calendar, r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED

Comment on attachment 9204105 [details] [diff] [review]
fix against comm-esr78, v2

[Triage Comment]
Approved for esr78

Attachment #9204105 - Flags: approval-comm-esr78? → approval-comm-esr78+
You need to log in before you can comment on or make changes to this bug.