Last Comment Bug 237311 - crash if "phantom" event is accessed
: crash if "phantom" event is accessed
Status: RESOLVED WORKSFORME
:
Product: Calendar
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Windows XP
: -- critical with 2 votes (vote)
: ---
Assigned To: Mostafa Hosseini
:
Mentors:
Depends on: 198633
Blocks:
  Show dependency treegraph
 
Reported: 2004-03-12 15:09 PST by Caliga (Daniel St.)
Modified: 2006-07-17 22:24 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments

Description Caliga (Daniel St.) 2004-03-12 15:09:46 PST
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.) 2004-03-12 15:24:30 PST
bug 198633 _may_ be related.
Comment 2 Jussi Kukkonen 2004-03-13 03:22:07 PST
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 2004-03-13 05:34:27 PST
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 2004-03-14 08:39:39 PST
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 2004-03-14 09:50:28 PST
Related/dupe:
Bug 168495 After adding a new event, events for disabled calendars show up.

Comment 6 Mostafa Hosseini 2004-03-16 07:04:51 PST
(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 2004-03-23 23:43:00 PST
(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 2004-10-22 20:38:23 PDT
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 2004-10-23 07:58:41 PDT
Works for me on Sunbird 20041001 and Calendar 20041008 on FF, both on w2k.
Comment 10 Joey Minta 2005-04-23 03:06:05 PDT
(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] (use needinfo?) 2006-07-17 22:24:44 PDT
The bugspam monkeys have been set free and are feeding on Calendar :: General. Be afraid for your sanity!

Note You need to log in before you can comment on or make changes to this bug.