ICS provider incorrectly uses nsIPipe and nsIUploadStream together



13 years ago
13 years ago


(Reporter: dmose, Assigned: dmose)




(1 attachment, 1 obsolete attachment)



13 years ago
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

13 years ago
Created attachment 199509 [details] [diff] [review]
avoid unnecessary conversions and copies

Seems to work.	Sorry for the original bum review, mvl!
Attachment #199509 - Flags: first-review?(mvl)
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));


>+        // 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...


13 years ago
Attachment #199509 - Flags: first-review?(mvl)

Comment 3

13 years ago
Created attachment 199599 [details] [diff] [review]
patch, v2

Patch, v2.  Addresses mvl's comments, and handles failure more gracefully.
Attachment #199509 - Attachment is obsolete: true
Comment on attachment 199599 [details] [diff] [review]
patch, v2

Attachment #199599 - Flags: first-review+

Comment 5

13 years ago
Fix checked in.
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.