717067 - Events show twice/duplicate in the unifinder for cached ics/full sync providers

Status: RESOLVED WORKSFORME

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I noticed this while trying to patch bug 710351 and sfleiter has confirmed. STR:

1. subscribe to an ics calendar with N events, be sure to enable the cache
2. (optional) restart lightning to get a fresh start

The unifinder now shows N events

3. Reload remote calendars

The unfinder now shows 2N events

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Here is what happens:

On a reload, the unfinder triggers a getItems call to refill the unifinder with all items. The cache layer then goes about refreshing and starts adding the items to the storage calendar and notifies the unifinder when this is complete, which adds the first batch of all items to the unifinder. Until now everything is ok.

The storage calendar then starts triggering a lot of onAddItem calls, because the items are being added from the cache layer. These calls are passed on to the unifinder and it starts adding those items in again.

Now how to fix this. The presumably easy way is a 3-line hack in the unifinder that makes sure that items that are already in it are not added again. This works fine on the UI side, although I have not tested what happens if a delete is done before the onAddItem call fires. I'd guess its wrongly added back in.

The better way would be quite tricky, since it involves instructing the storage calendar to add an item without triggering an onAddItem call. This breaks the calICalendar interface since we would specialcase just for the storage calendar.

The intermediate solution I briefly tried would be to suppress the onAddItem calls during the full sync. Whoever ends up fixing this bug should try that again, when I did it seems that mPendingSync was always null in the onAddItem call. I'm open to other solutions too.

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Mohit, do you think you could look into this bug? I don't remember this happening before, so I assume its a regression from the offline cache. If you need any help, you know where I am :-)

Comment 3

[Mohit Kanwal [:redDragon]]

Ok cool, I can look it up and see if I can come up with something :) meantime we can have a entire workflow or some documentation of some form regarding the cache, would make things and the entire workflow so much easier to understand :) !

Am assigning it to myself in the meantime.

Comment 4

[Mohit Kanwal [:redDragon]]

Does this still happen?

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I haven't tried to reproduce it lately, but I don't recall any code changes that might have fixed it.

Comment 6

[Reid Anderson]

Attachment: Patchv1

Comment 7

[Mohit Kanwal [:redDragon]]

Comment on attachment 8461381 [details] [diff] [review]
Patchv1

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

Just wondering if this can be fixed without introducing an extra variable

::: calendar/base/src/calCachedCalendar.js
@@ +80,5 @@
>          }
>      },
>  
>      onAddItem: function(aItem) {
> +        if (this.isCachedObserver && !this.home.mIsSync) {

Can check for mPendingSync == null?

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8461381 [details] [diff] [review]
Patchv1

Unfortunately this patch doesn't fix it for me. These are my exact STR:

1. Subscribe to an ics calendar using a file:// URL
2. Disable all other calendars in the profile
3. Restart
4. Synchronize*
5. Sort by title
6. Synchronze again

Now all events are there twice for me.



*: I observed a possibly different bug here too, it seems the initial sync doesn't work correctly. When I started Lightning, there was only one event in the unifinder. Then I synchronized and the remaining events showed up. The one event was there twice.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This issue could take a long time to fix and requires a lot of knowledge about the calendar codebase. I'm not sure if this is the right bug to fix for the GSoC.

The ics provider does some additional caching with the memory storage provider. Maybe it makes sense to postpone this bug and work on the cache-only ICS provider instead. Once you have the code for that working, revisit this bug and make sure the problem is gone. If not, then we can still figure out a solution.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I realize the first statement in my previous comment might need some explanation. I've been going through this the whole evening and I haven't yet understood all the interactions that lead to this bug and the new bugs it exposes. Maybe someone other than me will find a smart way to fix this, but this leads me to believe it might take a long time to find a solution. Rather than Reid spending a lot of time on this I'd rather get some other bugs fixed. Ideally, moving to cache-only mode will let us get rid of the full sync mode of the cached calendar anyway.

I'm recording some experiments I've done for future reference:

If I set mIsSync to true just after the emptyQueue function, and set it to false inside the emptyQueue function, then the events are not added to the unifinder twice. Doing that does break the refreshing though and I believe it will also break changelog based sync. I then tried to always refresh the event tree in the unifinder's onLoad function, but even with that I run into trouble.

I then tried using a Map to suppress the onAddItem call just when adding the item to the cached calendar during a full sync. It required using setTimeout to postpone deleting the item from the map, because the onAddItem fires after the onOperationComplete in the storage calendar. While I thought this would be the solution, refreshing doesn't work yet. Adding the same code to always refresh the tree in the onLoad function gets me yet again one step closer, but when updating the ics file from the outside, it takes two refreshes to update the calendar view and the unifinder never updates at all. Also, doing that introduces the same flicker in the unifinder as in the event view.

Comment 12

[Wayne Mery (:wsmwk)]

Philipp, do you believe this issue stil exists?

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

We've rewritten most parts of the unifinder since, I think we are good.