237311 - crash if "phantom" event is accessed

Status: RESOLVED WORKSFORME

(Calendar :: General, defect)

Description

[Caliga (Daniel St.)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.6) Gecko/20040113

what i call a "phantom" event: an event, that is shown, although the
corresponding calendar is unchecked. (calendar list, upper left)
these phantom events appear if you (for example) create a new event in a hidden
(unchecked) calendar.
accessing them crahes mozilla.

Reproducible: Always
Steps to Reproduce:
1. deactivate your favourite calendar (calendar list in the upper left)
2. create a new event. make sure it will be added to the calendar you just
deactivated (calendar file drop-down menu on the bottom of the new event dialog)
3. all events of the inactive calendar now reapear in the main view.
4. double clicking one of them crashes mozilla.

Actual Results:  
mozilla crashes

Expected Results:  
step 3 should not happen at all,
step 4 should not be possible at all

Comment 1

[Caliga (Daniel St.)]

bug 198633 _may_ be related.

Comment 2

[Jussi Kukkonen]

Confirming. 

RE comment 1:
I don't think so. My guess is that there are two bugs involved in here (neither
has anything to do with broken calendar files or RDF):
1. the events should not be drawn to view.
2. the 'open edit event on double click' code shouldn't crash (no matter what)

Haven't looked at the code yet, will do that.

Comment 3

[Jussi Kukkonen]

I debugged this (js) in day view by unchecking calendar A and then adding an
event (by doubleclicking on hour, although the method doesn't seem to matter) to
calendar A. 

It seems oeIICal.getEventsForDay() returns the correct events _and_ the ones for
calendar A after adding an event to calendar A. 

The call-path is, I believe (not 100% sure, Venkman's not really good with these):
  onAddItem observer
    DayView.prototype.refreshEvents
      CalendarEventDataSource.prototype.getEventsForDay
        oeIICal.getEventsForDay --- returns events for unchecked calendar A 


This will not happen on subsequent calls ( change to tomorrow and back in day
view for example). 

Comment 4

[Jussi Kukkonen]

I have a hunch about this after taking a look at oeICalContainerImpl.cpp.

normally oeICalContainerImpl::AddEvent() just adds the event, but if the
calendar is not 'on', this is done instead:
 1. AddCalendar( server )
 2. AddEvent( icalEvent, retid )
 3. RemoveCalendar( server )

The problem is, onAddItem-observers are notified in phase two when calendar is
temporarily 'on', right? 

Comment 5

[Jussi Kukkonen]

Related/dupe:
Bug 168495 After adding a new event, events for disabled calendars show up.

Comment 6

[Mostafa Hosseini]

(In reply to comment #4)
> The problem is, onAddItem-observers are notified in phase two when calendar is
> temporarily 'on', right? 

Yes that's right. The hardest part is to figure out how to do this properly (
adding an event to a disabled calendar ):

-Preventing adding/modifying events to disabled calendars?
This is limiting the functionality just to avoid the problem. Besides when an
alarm goes off for an event that resides in a disabled calendar the user should
be able to acknowledge it which is modifying it.

-Not call the observers for changes to disabled calendars?
Not good at all. Some observers might need to know about any change even to
disabled calendars. ( e.g. the future sync observer and the future alarm observer ).

-Change the observer so that it doesn't refresh the view if the notification is
about a disabled calendar?
Adds overhead to observer functions but might be worth it.

-Enhance the observer functions to accept more variables explaining the
nature/condition of the change.
(e.g. onAddItem( event, flags ); flags |= IS_DISABLED in this case )

thoughts, comments?

Comment 7

[Jussi Kukkonen]

(In reply to comment #6)
> Yes that's right. The hardest part is to figure out how to do this properly (
> adding an event to a disabled calendar ):
> 

True. It sounds like your third option (change observer) is probably the right
one here - no need to clutter the interfaces unless we know that observer
overhead will be unacceptable.

I'll probably test that in the next week or so, let me know if you think it
shouldn't be done.

Comment 8

[Martin Bagge / brother]

Some progress on this lately?
This a really serious and ugly bug that needs work.


In a vote I'll go for the third option in comment #6 "change the observer"

Comment 9

[gekacheka]

Works for me on Sunbird 20041001 and Calendar 20041008 on FF, both on w2k.

Comment 10

[Joey Minta]

(In reply to comment #9)
> Works for me on Sunbird 20041001 and Calendar 20041008 on FF, both on w2k.
> 

Also works for me.  The events shouldn't show up, but that's bug 168495.  Resolving.

Comment 11

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: General. Be afraid for your sanity!