fix: synchronize failure when refreshing during idle.

[benjunmun]

Main calendar code calCachedCalendar.synchronize does not behave properly when the replayChangesOn onResult callback occurs right away. As a workaround, defer the callback to the next event loop tick.

Manually tested by forcing the idle case to trigger every few refreshes. Without this change, future syncs fail reliably. With this change syncs resume properly after the idle.

Addresses #498, thanks for the suggestion @binki

[peci1]

I've manually installed this fix. You can ping me in a few days to ask if it seems to help.

[peci1]

Just to be sure, the steps to reproduce the issue this PR should fix are:

1. Launch TB and let it running for some time.
2. Create an event in GCal on another device.
3. Press Synchronize in TB and watch the event not appearing.

Right?

[benjunmun]

Just to be sure, the steps to reproduce the issue this PR should fix are:

1. Launch TB and let it running for some time.

2. Create an event in GCal on another device.

3. Press Synchronize in TB and watch the event not appearing.

Right?

This sounds good to me. You will need thunderbird to sit idle long enough for both the idle timeout, and your calendar refresh to happen. If you turn on calendar.debug.log, you should check after you've seen the message "[calGoogleCalendar] Skipping refresh since user is idle"

[binki]

Unfortunately, this does not fix the issue for me. I did not verify the logging which you requested, so I do not know if I am hitting the same condition as you. But, what I can say, from reading https://searchfox.org/comm-central/rev/d37acc38817c31be95401ca3c873672c813ca708/calendar/base/src/CalCachedCalendar.sys.mjs#270 , is that this change should be unnecessary because the Mozilla code already has await on a new Promise before calling replayChangesOn().

From reading the Mozilla code, what seems more likely is that that code lacks any error handling around replayChangesOn(). This may be intentional—if synchronization continues after an error, there is an increased chance in corrupting things. But any error thrown by replayChangesOn() will trigger the condition where mPendingSync will never get cleared.

I personally need to invest some amount of time in debugging what is happening in my client. I am not sure when I will get around to it.

P.S., the replayChangesOn() method uses old-style async where the code must guarantee it calls the callback. Promises make this a lot easier, but the interface appears to be designed pre-Promise. replayChangesOn() probably should be refactored to internally use async/await to make it harder to fail to make the callback.

[benjunmun]

I agree with you about the extra Promise layer complicating things, however, I found in practice that my change -does- reorder events. I think this may be dipping into really spooky interactions within the javascript event scheduler.

I've come up with a reduced test case that demonstrates the same issue in node.js to play with it more easily: https://gist.github.com/benjunmun/2aa67fdaa031f247bb1e6e639fb19634

Edit: The new Promise constructor is actually executed synchronously before hitting the await.

[peci1]

So, my TB has been running for 3 days (with sleeps and idle times in between) and it is still able to sync events from Google calendars!

[kewisch]

This is such great news that it seems to be resolving these longstanding issues. Just for testing purposes, maybe folks could try calling setTimeout() instead of promise-resolving? I believe setTimeout will create a task that actually runs in the next tick, as opposed to a microtask that promises use. setTimeout is provided by Timer.jsm/Timer.sys.mjs

I'll take a closer look in the next few days.

[benjunmun]

For anyone testing there's a version with setTimeout in the linked issue. Let me know if you would like me to reformulate this pull request using setTimeout.

[kewisch]

Let's go with the setTimeout version to be on the safe side. I think you can condense it a bit by calling aListener.onResult directly in the setTimeout() callback. Thank you!

[benjunmun]

Let's go with the setTimeout version to be on the safe side. I think you can condense it a bit by calling aListener.onResult directly in the setTimeout() callback. Thank you!

Updated with setTimeout. I had written it this way so that the returned promise finishes after 'onResult' has been called, trying to stay parallel with the other return path. Current Thunderbird code doesn't seem to care about this returned promise though...