312381 - ICS provider incorrectly uses nsIPipe and nsIUploadStream together

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

After the patch for bug 311405 landed, I started seeing occasional assertions
and failures to upload as documented in comment 5 of that bug.  It turns out
that in an earlier review, I recommended using nsIPipe without reading the
comments in nsIUploadChannel carefully enough.  It turns out that
nsIUploadChannel requires stream implementors that feed it to implement Seek.

Rather than going back to an nsIStorageStream, I think I see a way to avoid this
data going through JS at all, which should save us at least one copy and a
couple of conversions.  With luck this will fix the above-mentioned regression
as well as  improve performance.  Patch forthcoming.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: avoid unnecessary conversions and copies

Seems to work.	Sorry for the original bum review, mvl!

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 199509 [details] [diff] [review]
avoid unnecessary conversions and copies

>Index: base/src/calICSService.cpp
>+    // this copies the string into the input stream that's handed back,
>+    // because we don't really own icalstr; it's one of libical's ring 
>+    // buffers

Can you explain what copies? you use a dependent string, which doesn't copy.
That confused me. I assume the imputstream copies.

>+    return NS_NewCStringInputStream(aStreamResult, 
>+				    nsDependentCString(icalstr));

tabs..

>+        // The return values is calIError match with ical errors,
>+        // so no need for a conversion table or anything.
That sentence has some grammar errors in it. (I think i once wrote it, so it's
very likely to not be correct :) )

>Index: providers/ics/calICSCalendar.js
>-                    this.unmappedProperties.push(prop);
>+		    this.unmappedProperties.push(prop);
More tabs...

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

Patch, v2.  Addresses mvl's comments, and handles failure more gracefully.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 199599 [details] [diff] [review]
patch, v2

r=mvl

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in.