1738867 - Some recurring events missing from agenda on first load

Status: RESOLVED FIXED

(Calendar :: Provider: Local Storage, defect, P1)

Description

[Geoff Lankow (:darktrojan)]

I have some recurring events in one of my calendars that don't appear on the Agenda. I've discovered that this is because of a race condition. We load the events, including recurrence info, into a cache and retrieve them from there, assuming (because we have just done it) the recurrence info is there. Meanwhile another operation comes along and throws away the cache to refill it (I don't really understand why and that code path has the word HACK in a comment, so not a good sign). So when the event retrieval code goes to use the recurrence info, it is missing and we get the wrong result.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1738867 - [CalStorageCalendar] Don't empty recurring item caches while they are being filled. r=mkmelin

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b0477ae0023f
[CalStorageCalendar] Don't empty recurring item caches while they are being filled. r=mkmelin,john.bieling

Comment 3

[Geoff Lankow (:darktrojan)]

95.0b1:
https://hg.mozilla.org/releases/comm-beta/rev/42d08beb6a88

Comment 4

[Geoff Lankow (:darktrojan)]

I'm still experiencing this, although to a lesser extent. Bug 1739270 will solve the problem, but I want to add more to this fix too.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1738867 - [CalStorageCalendar] Call idleDispatch when clearing the recurring item cache. r=mkmelin

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5093a8e2d7e9
[CalStorageCalendar] Call idleDispatch when clearing the recurring item cache. r=mkmelin

Comment 7

[Geoff Lankow (:darktrojan)]

Comment on attachment 9249667 [details]
Bug 1738867 - [CalStorageCalendar] Call idleDispatch when clearing the recurring item cache. r=mkmelin

[Approval Request Comment]
User impact if declined: I'm seeing things missing from the agenda, and I think this fixes it
Testing completed (on c-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): very low

(Getting in early because otherwise I'll forget to do it, and one patch has already gone to beta so the flags are set.)

Comment 8

[Geoff Lankow (:darktrojan)]

Comment on attachment 9249667 [details]
Bug 1738867 - [CalStorageCalendar] Call idleDispatch when clearing the recurring item cache. r=mkmelin

Oh, actually, this patch won't land cleanly on beta. I'll make another one.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1738867-idle-dispatch-beta.diff

Take 2:

[Approval Request Comment]
User impact if declined: I'm seeing things missing from the agenda, and I think this fixes it
Testing completed (on c-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): very low

Comment 10

[Rob Lemley [:rjl]]

Comment on attachment 9249875 [details] [diff] [review]
1738867-idle-dispatch-beta.diff

[Triage Comment]
Follow-up to a bug fix uplifted in b1.

Comment 11

[Rob Lemley [:rjl]]

Thunderbird 95.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/625f7225da7c

Comment 12

[[e-mail]]

Attachment: Proposed patch

I just ran into this in Thunderbird 91 which is very much affected. In fact, I ran in to a more severe version of this where there were three callers racing against each other; at the very least your first fix does not solve that version, and it's unclear to me what your second patch is trying to achieve but I don't think it fixes the double race either. Here's what I was experiencing:

• First call to getItems starts assuring the recurring item caches
• Second call to getItems clobbers the promise, causing the recurring item caches to be built again while the first call is still trying to build them
• Third call to getItems clobbers the second call's promise, causing the recurring item caches to be built again while the first two calls are still trying to build them

Let's see what happens if we only await the promise first.

• First call to getItems starts assuring the recurring item caches
• Second call to getItems waits for the promise, then builds the recurring item caches again
• Third call to getItems waits for the first call's promise, then clobbers the promise from the second call, thus causing the race condition again

I was able to fix the problem completely in Thunderbird 91 by keeping track of which promise was being awaited. In particular, the third call to getItems waits for the first call's promise, then sees that there's a different promise. It recognises that what happened was that the second call waited for the first call's promise and started to rebuild the cache, so it doesn't clobber the promise and simply waits for the second call's promise.

If you think this approach is correct then I'm willing to port it to trunk for you.

Comment 13

[Geoff Lankow (:darktrojan)]

I think this makes sense, but I would rather have it on trunk first, especially given we no longer have a test audience for the 91 code. And let's do it a new bug to avoid further confusing the issue.

Comment 14

[Geoff Lankow (:darktrojan)]

Comment on attachment 9274291 [details] [diff] [review]
Proposed patch

Just clearing the flag. See my previous comment.