403241 - automated test fails when iCal string does not contain a start date

Status: RESOLVED FIXED

(Calendar :: Import and Export, defect)

Description

[Andy Lawrence]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.8pre) Gecko/20071018 Calendar/0.7pre

line 138 of mozilla/calendar/test/unit/test_ics_roundtrip.js contains the following ical string:

 event.icalString = "BEGIN:VEVENT\n" +
                       "UID:1\n" +
                       "X-MAGIC:mymagicstring\n" +
                       "END:VEVENT";

The point of the test is to test that x-properties are round-tripped, but it contains no start date. Because of this, mozilla/calendar/base/src/calEvent.js.set icalComponent() fails, as it uses startDate without first checking that it's non-null.

Reproducible: Always

Steps to Reproduce:
1. cd $OBJDIR
2. make -C calendar/test check
3. note failure of test_ics_roundtrip.js (details in log)
Actual Results:  
[Exception... "'[JavaScript Error: "this.startDate has no properties" {file: "file:///Users/andrewlawrence/src/mozilla/obj-i686-apple-darwin8.10.1/dist/bin/js/calEvent.js" line: 230}]'

Expected Results:  
*** PASS ***

I have a patch for this, which I will attach shortly.

Comment 1

[Andy Lawrence]

Attachment: Patch to check for a valid start date before using it.

Comment 2

[cmtalbert]

Hmm...Ok.  My original instructions to Andy were based on the older version of the specification in my head.  THe current draft (http://tools.ietf.org/html/draft-ietf-calsify-rfc2445bis-07) states:

Component Name:  VEVENT

   Purpose:  Provide a grouping of component properties that describe an
      event.

   Format Definition:  A "VEVENT" calendar component is defined by the
      following notation:

        eventc     = "BEGIN" ":" "VEVENT" CRLF
                     eventprop *alarmc
                     "END" ":" "VEVENT" CRLF

        eventprop  = *(

                   ; the following are REQUIRED,
                   ; but MUST NOT occur more than once

                   dtstamp / dtstart / uid /

                   ; the following are OPTIONAL,
                   ; but MUST NOT occur more than once

                   class / created / description / geo /
                   last-mod / location / organizer / priority /
                   seq / status / summary / transp /
                   url / recurid /

                   ; the following is OPTIONAL,
                   ; but SHOULD NOT occur more than once

                   rrule /

                   ; either 'dtend' or 'duration' MAY appear in
                   ; a 'eventprop', but 'dtend' and 'duration'
                   ; MUST NOT occur in the same 'eventprop'

                   dtend / duration /

                   ; the following are OPTIONAL,
                   ; and MAY occur more than once

                   attach / attendee / categories / comment /
                   contact / exdate / rstatus / related /
                   resources / rdate / x-prop / iana-prop

                   )

So, that means that the old "Magic String" event that was in our test suite is actually invalid ICS.

That said, I still think this patch is valid, (since it ensures that the event.startDate does exist and is initialized). 
 

Comment 3

[Stefan Sitter [:ssitter]]

Same error message as in Bug 397559. Resolve as duplicate?

Comment 4

[cmtalbert]

Comment on attachment 288043 [details] [diff] [review]
Patch to check for a valid start date before using it.

Nice catch! Looks good.

Comment 5

[cmtalbert]

(In reply to comment #3)
> Same error message as in Bug 397559. Resolve as duplicate?
> 
I think that bug 397559 has morphed into the larger question of "what should the ICS parser do when encountering non-standard data", so I don't think these are duplicates per se.  This does solve the direct issue that caused the original reported issue in 397559 though. I'll make a note there to that effect.

Bug has a patch that ought to be checked in, going to confirm it as well.

Comment 6

[Philipp Kewisch [:Fallen][☁️📆]]

Digging for bugs here, I've adapted the patch a bit to work with the latest code and:

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/bcb38f072c3e>
Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/60149d8b0df5>

-> FIXED