494783 - Switching view range or editing events doesn't update view content and displayed events

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090525 Calendar/1.0pre

Steps to Reproduce:
1. Download http://www.mozilla.org/projects/calendar/caldata/GermanHolidays.ics and http://www.mozilla.org/projects/calendar/caldata/USHolidays.ics to local disc
2. Start Sunbird with new profile
3. Switch to Month View
4. Use File > Open Calendar File and select GermanHolidays.ics
5. Use File > Open Calendar File and select USHolidays.ics
6. Close and restart Sunbird
7. Switch to Month View, change to next or previous month

Actual Results:
The navigation bar (month, calendar week, date range) is updated. But the view area is not updated and still shows the events from the current month.

Comment 1

[Stefan Sitter [:ssitter]]

Regression range: Works in Sunbird 1.0pre (BuildID: 20090519045809)
                  Fails in Sunbird 1.0pre (BuildID: 20090520034504)

Changes during regression range: https://hg.mozilla.org/comm-central/pushloghtml?startdate=2009-05-19+04:58:09&enddate=2009-05-20+03:45:04

Most probably caused by the checkin for Bug 490309.

Comment 2

[Stefan Sitter [:ssitter]]

Attachment: console output of debug build

I get no error message with the official nightly builds. But with my own debug build I get the following errors:

!!!!! XPConnect wrapper thread use error...
  XPConnect WrappedNative is being accessed on multiple threads but the underlying native xpcom object does not have a nsIClassInfo with the 'THREADSAFE' flag set 
  wrapper: [xpconnect wrapped calIIcalComponent @ 0x49dbb38 (native @ 0x5e200e8)]

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "can't convert Call to string" {file: "file:///E:/obj/sb-dbg/mozilla/dist/bin/modules/calUtils.jsm -> file:///E:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calIcsParser.js" line: 82}]' when calling method: [nsIRunnable::run]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]

See log file for full message.

Comment 3

[Daniel Boelzle [:dbo]]

Attachment: fixing threadsafe warning [checked in]

Thanks for catching this. I don't know whether this patch fixes the bug, bug it should fix the "threadsafe" warning.

Comment 4

[Stefan Sitter [:ssitter]]

Comment on attachment 379604 [details] [diff] [review]
fixing threadsafe warning [checked in]

I'm not familiar with the NS_* macros and their usage. Maybe Philipp can take over review?

Comment 5

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

Comment on attachment 379604 [details] [diff] [review]
fixing threadsafe warning [checked in]

You probably know this better than I do, but *are* our calIIcalComponents actually threadsafe?

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

from calIICalComponent:
80     calIIcalComponent getNextSubcomponent(in AUTF8String componentType);
That one worries me. It means that the component has some internal state. If one thread tries to get all the sub components using this method, and some other thread also calls the method, things go wrong.
Or is there magic in the implementation that I'm missing?

Comment 7

[Stefan Sitter [:ssitter]]

Some more information: I can now reproduce with all views. It always fails for the view that is active during Sunbird startup. The other views work fine.

For example: If Day View is active and Sunbird is closed the Day View will fail during the next application session. But Week, Multiweek and Month views work.

In addition I noticed that the view never updates itself, not when switching the view range, not when adding/editing/deleting events, not when enabling/disabling calendars, ...

Comment 8

[Daniel Boelzle [:dbo]]

Comment on attachment 379604 [details] [diff] [review]
fixing threadsafe warning [checked in]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/0b071c184691>

Stefan, does this actually fix the bug? I haven't had any time to do a Sunbird build...

Comment 9

[Daniel Boelzle [:dbo]]

(In reply to comment #5)
> (From update of attachment 379604 [details] [diff] [review])
> You probably know this better than I do, but *are* our calIIcalComponents
> actually threadsafe?

No, they are not. The core objects cannot be used *concurrently* from multiple threads, both w.r.t. how they are coded (e.g. updating internal state), but also w.r.t. to stateful  API design (as mvl also pointed out). However, this is no problem unless multiple threads actually use those objects from multiple threads which they don't do. Just the ical parsing stage and creating the top-level calIIcalComponent is outsourced to a worker thread. Every further processing (iterating and creating calIEvent etc objects) still remain on shared the main thread. What may need further investigation is whether libical's ring buffers are safe (and used on parsing). Reading the code (compiled with HAVE_PTHREAD), it ought to be.

Comment 10

[Stefan Sitter [:ssitter]]

The error message in the debug build is gone. But the reported issue is not fixed. I can still reproduce it using hourly build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090529 Calendar/1.0pre (BuildID: 20090529113112)

Comment 11

[Daniel Boelzle [:dbo]]

Hmm, this WFM using my Mac debug build. Any further information available?

Comment 12

[sp98]

I have same problem. Worked OK on version of 20090518, but failed on the 20090530 update. As reported earlier, it fails to update the view that is active during Sunbird startup. 
Environment: Windows XP, Provider for Google Calendar.

Comment 14

[Stefan Sitter [:ssitter]]

(In reply to comment #11)

What information do you need? For me the issue exists as soon as I add a local ics file (with at least one event) and restart Sunbird.

Comment 15

[Daniel Boelzle [:dbo]]

Could you reproduce the issue on Lightning (Windows)?

Comment 16

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

We should definitely fix this or alternatively disable threading for beta1.

Comment 17

[r.novotny]

I can reproduce this issue on clean install of Thunderbird 3.0b2 when using Lightning nightlies past 2009/05/20 (Windows XP). Downgrading to the 2009/05/19 works fine -- I can scroll through the months without any problems.

I think this affects Task View as well.

Comment 18

[Stefan Sitter [:ssitter]]

(In reply to comment #17)

Due to major changes in Thunderbird itself the current Lightning nightly builds work only correctly in 3.0b3pre but not in 3.0b2. Can you reproduce using Thunderbird 3.0b3pre?

Comment 19

[r.novotny]

(In reply to comment #18)
Ok, I downloaded Thunderbird 3.0b3pre (2009-06-29 nightly), installed Lightning 2009-06-30 nightly. When using the original profile, the issue was still there. 
However, when I exported my calendars to ICS from 3.0b2 profile, started a brand new profile in 3.0b3pre and imported the ICSs, the issue disappears -- I can scroll without any problems.

I think someone should mark the current Lightning nightlies as incompatible with Thunderbird 3.0b2. This should prevent the installation incompatibilities.

Comment 20

[Stefan Sitter [:ssitter]]

Some more information after adding some debug information to calendar-base-view.xml:

If a local ics calendar is enabled the function onEndBatch() is called. But there is no previous call to onStartBatch()! Therefore this.calView.mBatchCount is set to -1 and subsequent calls to other functions like onAddItem() fails.

Comment 21

[Stefan Sitter [:ssitter]]

Attachment: workaround

This simple workaround ensures that this.calView.mBatchCount is never less than Zero. With the patch applied everything works OK again for me.

The better fix is probably to investigate why onEndBatch() is called without previous onStartBatch() call and fix the caller.

Comment 22

[Daniel Boelzle [:dbo]]

(In reply to comment #20)
> If a local ics calendar is enabled the function onEndBatch() is called. But
> there is no previous call to onStartBatch()! Therefore this.calView.mBatchCount
Strange. There's a call right before (async) parsing begins:
<http://mxr.mozilla.org/comm-central/source/calendar/providers/ics/calICSCalendar.js#232>. Could you verify you pass that code (and an observer is notified). Is there a registered observer at all?

Comment 23

[Daniel Boelzle [:dbo]]

I'd prefer we really understand what's going wrong with the batch count. Stefan, since you can reproduce the bug, could you spend some more time on investigation, please?

Comment 24

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

Stefan, any updates on this bug?

Comment 25

[Stefan Sitter [:ssitter]]

I can still reproduce the issue using the STR from comment #0 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3pre) Gecko/20090811 Calendar/1.0pre (BuildID: 20090811051957).

Comment 26

[Daniel Boelzle [:dbo]]

I can imagine how this bug occurs: Before loading the views, e.g. the alarm service loads its events. While loading alarm items the views add a calIObserver (e.g. to composite) and receive an onEndBatch (though they've never revceived an onStartBatch). This is odd and shows that batch counting doesn't work with concurrency. I am wondering why this hasn't happened before with the other asynchronous providers, but anyway.

I can think of the following:
A) We derive a calObserverBag (out of calListenerBag) and use that throughout the code when dealing with calIObservers. That implementation keeps track of the batch count (e.g. increased/decreased on notifyObservers(onStartBatch/onEndBatch). When a new calIObserver is added and the bag's batch count is not zero, we replay the same number of onStartBatch notifications on it (since it'll receive the same number of onEndBatch notifications later on).
B) We dump the batch handling, and look for a different thread-safe alternative.
C) We turn off threading (which seems to cure this).

I can imagine A) works, but to be honest, it looks complicated and hardly maintainable to me. I'd rather go with B) -- though I don't yet have a clue about an alternative.

Comment 27

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

Maybe we can just implement a method getBatchCount() and have the observers just get the batch count when they register?

Regarding B: Batch handling the way we do things seems to be broken anyway to some point. I think this would be a massive change, but maybe we can have some sort of transaction object that implements the get/add/update/delete methods and can play all actions at once? I imagine this being similar to transactions in SQL.

This would also simplify code that deletes occurrences of recurring events, i.e you could start a transaction, do all occurrence deletes with deleteItem(), then commit the transaction, which will then modify the master. We already do this somewhere in our code, but I believe we should be doing it more often.

This is more of a long term solution though. Nothing comes to mind what we can solve short-term.

Daniel, since you have done a substantial amount of work on this I'll leave the final decision to you. What road should we follow short term?

Comment 29

[Daniel Boelzle [:dbo]]

Attachment: patch - v1

This patch introduces a new observer bag and replays the onStartBatch notifications on new observers being added if there's a batch in action.
I hope this fixes the problem, because I cannot reproduce it on my Mac.

Comment 30

[Stefan Sitter [:ssitter]]

Comment on attachment 398142 [details] [diff] [review]
patch - v1

This patch fixes my issue, r=ssitter. But I don't understand in detail the __proto__ magic you are doing. Therefore I'd like Philipp to take an additional look.

Comment 31

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

Comment on attachment 398142 [details] [diff] [review]
patch - v1

>+cal.ObserverBag = function calObserverBag(iid) {
>+    this.init(iid);
>+};
>+cal.ObserverBag.prototype = {
>+    __proto__: cal.calListenerBag.prototype,
This basicly means, that the base class for this object should be cal.calListenerBag, i.e this object inherits all members of calListenerBag.

>+    notify: function calObserverBag_notify(func, args) {
...
>+        return this.__proto__.__proto__.notify.apply(this, arguments);
>+    },
This means, that the base class' prototype's notify function should be called with |this| and the given arguments. The doubling is needed because let p1 = this.__proto__ is the prototype of the current object and p1.__proto__ is the prototype of the base class object.




>+        this.mObservers = new cal.ObserverBag(Components.interfaces.calIObserver);
Daniel, maybe we should call it observerBag to use camel case consistently?


r=philipp

Comment 32

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

Checking this in due to upcoming beta. Daniel, if you have good arguments for cal.ObserverBag go ahead and comment here.

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/3f38957b80a5>

-> FIXED

Comment 33

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

Maybe we can mention in the release notes that if windows users run into problems similar to these, they can disable threading to work around.

Comment 34

[bernhard krumm]

Build Identifier: Calendar/1.0pre

A new event is not shown in the calendar. After close and open it is shown. 

Reproducible: Always

Steps to Reproduce:
1. add new event or change an existing event
2. close sunbird
3. reopen sunbird
Actual Results:  
update is not displayed rightaway, but after close and reopen

Expected Results:  
immediated update / display of new event

Sunbird worked OK until I updated thunbird today:
- Thunbird 2.0.0.22 to 3.0 Beta 3
- added Extensions to Thunderbird 
- - Addblock
- - thunder browse
- - added germanholidays.isc    (like described on top !!!!!!!!!! )

same result after updating sunbird from 1.0pre 20.Jul 2009 to 1.0pre 7 sep
2009
I am using plain Sunbird. Guess it is my local calendar, did not publish
anything. Don't get any error message, new event is just not displayed,
Same for changed event. 
But it must have been updated, as it is shown after close and reopen.

--> NOT FIXED ?

Comment 35

[Stefan Sitter [:ssitter]]

Bernhard, the fix has been pushed just two hours ago. You'll need to wait for tomorrows (20090908) nightly build for retest.

Comment 36

[Daniel Boelzle [:dbo]]

Thanks for taking care, Philipp.

(In reply to comment #31)
> >+        this.mObservers = new cal.ObserverBag(Components.interfaces.calIObserver);
> Daniel, maybe we should call it observerBag to use camel case consistently?

I don't think we should do that since I read |ObserverBag| as kind of class to instantiate and |cal| as its namespace.

Comment 38

[Tony Mechelynck [:tonymec]]

Verifying on Sunbird for Linux, as follows:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4pre) Gecko/20090908 Calendar/1.0pre

This is a "nightly" build, I don't know the exact HHMMSS build ID.

The "grid" view is now updated when clicking left/right arrows at its top. It is also updated when clickikng an arbitrary date in the "little calendar" on the "Date" tab of the sidebar.

New events are now displayed immediately.

Existing events are now displayed at startup, even in the "current" view.

Another bug, which I'm not sure had been reported, has also disappeared: no more "Unknown Calendar" added once per startup (meaning, until yesterday, if I closed and started 5 times I used to get 5 "Unknown Calendar"s below my subscribed ones).

Comment 39

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

(In reply to comment #36)
> Thanks for taking care, Philipp.
> 
> (In reply to comment #31)
> > >+        this.mObservers = new cal.ObserverBag(Components.interfaces.calIObserver);
> > Daniel, maybe we should call it observerBag to use camel case consistently?
> 
> I don't think we should do that since I read |ObserverBag| as kind of class to
> instantiate and |cal| as its namespace.

Case change taken care in changeset fca52a924ddc.

Verified per previous comment, thanks for testing!

Comment 40

[bernhard krumm]

ref: #34 + #35

with built of 2009-09-08 
immediate displays of new, update or delete of event works now,  thanks

Comment 41

[Stefan Sitter [:ssitter]]

(In reply to comment #33)
> Maybe we can mention in the release notes that if windows users run into
> problems similar to these, they can disable threading to work around.

Only nightly testers were affected by this issue. Now it is fixed. I don't see a reason to put this issue in the release notes.

Comment 42

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

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".