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

VERIFIED FIXED in 1.0b1

Status

Calendar
Calendar Views
--
major
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: Stefan Sitter, Assigned: dbo)

Tracking

({regression})

Trunk
1.0b1
regression
Bug Flags:
blocking-calendar1.0 +

Details

(Whiteboard: [needed beta][no l10n impact])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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.
(Reporter)

Comment 1

9 years ago
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.
Flags: blocking-calendar1.0?
Flags: blocking-calendar1.0? → blocking-calendar1.0+
(Reporter)

Comment 2

9 years ago
Created attachment 379579 [details]
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.
(Assignee)

Comment 3

9 years ago
Created attachment 379604 [details] [diff] [review]
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.
Attachment #379604 - Flags: review?(ssitter)
Assignee: nobody → dbo.moz
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: x86 → All
(Reporter)

Comment 4

9 years ago
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?
Attachment #379604 - Flags: review?(ssitter) → review?(philipp)
(Reporter)

Updated

9 years ago
Blocks: 490309
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?
Attachment #379604 - Flags: review?(philipp) → review+
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?
(Reporter)

Comment 7

9 years ago
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, ...
Summary: Switching Month View doesn't update view content and displayed events → Switching view range or editing events doesn't update view content and displayed events
(Assignee)

Comment 8

9 years ago
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...
Attachment #379604 - Attachment description: fixing threadsafe warning → fixing threadsafe warning [checked in]
(Assignee)

Comment 9

9 years ago
(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.
Target Milestone: --- → 1.0
(Reporter)

Comment 10

9 years ago
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)
(Assignee)

Comment 11

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

Comment 12

9 years ago
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.
(Reporter)

Updated

9 years ago
Duplicate of this bug: 495014
(Reporter)

Comment 14

9 years ago
(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.
(Assignee)

Comment 15

9 years ago
Could you reproduce the issue on Lightning (Windows)?
We should definitely fix this or alternatively disable threading for beta1.
Whiteboard: [needed beta][no l10n impact]

Comment 17

9 years ago
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.
(Reporter)

Comment 18

9 years ago
(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

9 years ago
(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.
(Reporter)

Comment 20

9 years ago
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.
(Reporter)

Comment 21

9 years ago
Created attachment 386357 [details] [diff] [review]
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.
Attachment #386357 - Flags: review?(dbo.moz)
(Assignee)

Comment 22

9 years ago
(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?
(Assignee)

Comment 23

9 years ago
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?
Stefan, any updates on this bug?
(Reporter)

Comment 25

9 years ago
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).
(Assignee)

Comment 26

9 years ago
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.
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?
(Reporter)

Updated

8 years ago
Attachment #386357 - Flags: review?(dbo.moz)
Duplicate of this bug: 512986
(Assignee)

Comment 29

8 years ago
Created attachment 398142 [details] [diff] [review]
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.
Attachment #386357 - Attachment is obsolete: true
Attachment #398142 - Flags: review?(ssitter)
Whiteboard: [needed beta][no l10n impact] → [needed beta][no l10n impact][needs review]
(Reporter)

Comment 30

8 years ago
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.
Attachment #398142 - Flags: review?(ssitter)
Attachment #398142 - Flags: review?(philipp)
Attachment #398142 - Flags: review+
Attachment #398142 - Flags: review?(philipp) → review+
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
Keywords: checkin-needed
Whiteboard: [needed beta][no l10n impact][needs review] → [needed beta][no l10n impact][needs checkin]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needed beta][no l10n impact][needs checkin] → [needed beta][no l10n impact]
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.
Keywords: relnote

Comment 34

8 years ago
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 ?
(Reporter)

Comment 35

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

Comment 36

8 years ago
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.
Duplicate of this bug: 515056
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).
(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!
Status: RESOLVED → VERIFIED

Comment 40

8 years ago
ref: #34 + #35

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

Comment 41

8 years ago
(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.

Updated

8 years ago
Keywords: relnote
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".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.