Closed Bug 322827 Opened 14 years ago Closed 13 years ago

All X-Components are serialized with BEGIN:X, rather than BEGIN:X-FOO

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Lightning 0.3

People

(Reporter: jminta, Assigned: mvl)

References

()

Details

(Keywords: dataloss, Whiteboard: [libical-upstream?])

Attachments

(3 files, 1 obsolete file)

Something like

BEGIN:X-SOMETHING-COOL
X-AWESOME-THING1:yay

END:X-LOCATION

is disappearing when publishing/editing the calendar.
err, that should be 
END:X-SOMETHING-COOL
No longer blocks: lightning-0.1
Target Milestone: --- → Lightning 0.2
Flags: blocking0.3+
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
Whiteboard: [swag:2d]
Attached file testcase.js
This is a xpcshell test for this bug.  It shows that even though the X-Component is added, it fails to be serialized.
Looks like we have some weird component-conversion going on here.  If I try simply

var xComp = icsServ.parseICS("BEGIN:X-SOMETHING-COOL\nX-AWESOME-THING1:yay\nEND:X-SOMETHING-COOL");
dump(xComp.serializeToICS()+'\n');

it will throw with BADARG, due to http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalcomponent.c#299

The fact that this is saying its ICAL_NO_COMPONENT, rather than ICAL_X_COMPONENT is likely the reason that we're failing to serialize.
http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalcomponent.c#1350
strcmp is giving us garbage here.  |string| is properly X-SOMETHING-COOL, but when  i is 10 (and component_map[i].name is "X") it's returning -45, rather than the 0 that I would expect.  This means we fail to match the right component type.
This at least will parse the x-comp and spit it back out.  The problem is that the name of the x-prop gets lost.  Someone started implementing this here: http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalcomponent.c#54
but a quick glance at http://lxr.mozilla.org/mozilla/search?string=x_name will show you it never got past defining the member.  Someone with more c-foo than me is going to need to take care of actually storing that name here: http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalparser.c#667
and reusing it here: http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalcomponent.c#306
Attachment #233193 - Flags: second-review?(dmose)
Attachment #233193 - Flags: first-review?(mattwillis)
Comment on attachment 233193 [details] [diff] [review]
part one (checked in)

After dusting off my rusty C, this looks right.

We won't try to include the trailing \0 in the comparison.

r1=lilmatt
Attachment #233193 - Flags: first-review?(mattwillis) → first-review+
Comment on attachment 233193 [details] [diff] [review]
part one (checked in)

r=dmose
Attachment #233193 - Flags: second-review?(dmose) → second-review+
Comment on attachment 233193 [details] [diff] [review]
part one (checked in)

patch checked in.
Attachment #233193 - Attachment description: part one → part one (checked in)
Summary: Failure to serialize X-Components → All X-Compnents are serialized with BEGIN:X, rather than BEGIN:X-FOO
Whiteboard: [swag:2d] → [swag:1d]
Summary: All X-Compnents are serialized with BEGIN:X, rather than BEGIN:X-FOO → All X-Components are serialized with BEGIN:X, rather than BEGIN:X-FOO
Attached patch patch libical (obsolete) — Splinter Review
This patch does what jminta described in comment 6.
Sorry about the tab mess, that's just libicals fault.
Attachment #234605 - Flags: first-review?(dmose)
Whiteboard: [swag:1d] → [swag:1d][patch in hand]
Whiteboard: [swag:1d][patch in hand] → [swag:1d][patch in hand][needs review dmose]
Whiteboard: [swag:1d][patch in hand][needs review dmose] → [swag:1d][patch in hand][needs review dmose][no l10n impact]
Attachment #234605 - Flags: second-review?(dmose)
Attachment #234605 - Flags: first-review?(dmose)
Attachment #234605 - Flags: first-review?(daniel.boelzle)
(In reply to comment #10)
> Created an attachment (id=234605) [edit]
> patch libical

Looks good, only minor nits:

> +   icalcomponent* comp = icalcomponent_new_impl(kind);
> +   comp->x_name = icalmemory_strdup(x_name);
> +   return comp;
check for out-of-mem:
if (comp != 0)
    comp->x_name

>     kind_string  = icalcomponent_kind_to_string(kind);
> +   if (kind == ICAL_X_COMPONENT) {
> +       kind_string = impl->x_name;
> +   }
minor opt:
      else
          kind_string  = icalcomponent_kind_to_string(kind);

r1=dbo
Attachment #234605 - Flags: first-review?(daniel.boelzle) → first-review+
Comment on attachment 234605 [details] [diff] [review]
patch libical

>+/** @brief Constructor
>+ */
>+icalcomponent*
>+icalcomponent_new_x (icalcomponent_kind kind, const char* x_name)

Unless I'm misunderstanding something, kind is always going to be ICAL_X_COMPONENT, so there should be no need for the kind argument at all.
r2=dmose with dbo's comments addressed and that fixed.
Attachment #234605 - Flags: second-review?(dmose) → second-review+
Whiteboard: [swag:1d][patch in hand][needs review dmose][no l10n impact] → [swag:1d][patch in hand][needs updated patch][no l10n impact]
Assignee: nobody → mvl
Whiteboard: [swag:1d][patch in hand][needs updated patch][no l10n impact] → [needs updated patch]
Whiteboard: [needs updated patch] → [needs updated patch and checkin]
Attached patch updated patchSplinter Review
Updated patch, ready for checkin
Attachment #234605 - Attachment is obsolete: true
Attachment #238998 - Flags: second-review+
Attachment #238998 - Flags: first-review+
Whiteboard: [needs updated patch and checkin] → [needs checkin mvl]
patch checked in
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs checkin mvl]
Whiteboard: [libical-upstream?]
You need to log in before you can comment on or make changes to this bug.