Last steps before crash: - remove all google calendar and caldav calendars - disable gdata provider - file/quit --> crash. attaching crash report.

I wonder if we shouldn't just eliminate use of the webdav module, something I'd been thinking to do during the 0.9 cycle anyway. Outside the CalDAV provider it's only used once, in the ICS provider, and that to get an etag - which AFAIK could be done as easily with a HEAD as with a PROPFIND. In the CalDAV provider it's currently used only twice, and that number will remain steady over my next couple of planned patches. It would be straightforward to eliminate it, but for performance reasons I think we'd probably want to do that in conjunction with adding a SAX parser for REPORTs.

(In reply to comment #1) I buy that: +1

propfind isn't supported on all webdav-servers. Would the ics-provider still support webdav-calendars after eliminating the module? Lots of people are using webdav to access their calendars.

Might be related: Our current topcrasher on branch also points to OperationStreamListener::OnStopRequest() in mozilla/extensions/webdav/src/nsOperationStreamListeners.cpp. <http://talkback-public.mozilla.org/reports/sunbird/> <http://talkback-public.mozilla.org/search/start.jsp?search=1&searchby=stacksig&match=contains&searchfor=OperationStreamListener::OnStopRequest&vendor=MozillaOrg&product=Sunbird05&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid> Several of the comments mentioned that the crash happened after closing Sunbird and using CalDAV calendars.

(In reply to comment #1) > it's only used once, in the ICS provider, and that to get an etag - which AFAIK > could be done as easily with a HEAD as with a PROPFIND. I'm not too sure of that. I remember that there were problems with apache+mod_dav and etags back when I wrote that code. I'm not sure anymore what the exact problem was, but it sure needs testing before any code gets removed.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/webdav/src/nsOperationStreamListeners.cpp&rev=&root=/cvsroot&mark=152,160#130 this code is buggy. please handle NS_FAILED(rv) after 152 instead of crashing at 160.

(In reply to comment #5) > (In reply to comment #1) > > could be done as easily with a HEAD as with a PROPFIND. > I'm not too sure of that. I remember that there were problems with > apache+mod_dav and etags back when I wrote that code. I'm not sure anymore what > the exact problem was, but it sure needs testing before any code gets removed. > Yes, clearly we'd need to test it. And I'm not particularly a partisan of using HEAD here: we could also get that etag by setting the method on an nsIHttpChannel to "PROPFIND" and parsing the result, as the CalDAV provider does in several places. This is exactly equivalent to what we're doing now; it takes a few more lines of JS but removes the dependency on the webdav module.

(In reply to comment #3) > propfind isn't supported on all webdav-servers. Would the ics-provider still > support webdav-calendars after eliminating the module? Lots of people are using > webdav to access their calendars. > The ICS provider currently does use a PROPFIND to get that etag, via the webdav service's .getResourceProperties(), so we're not talking about changing the functionality of the ICS provider at all.

This bug has a severity of critical but no 0.8 flags at all, so I'm wondering where it stands wrt the release. I have some draft code for a webdav-module-less calendar tree, but need to figure out priorities with time so short before RC1.

Could somebody with a Mac at hand provide a better symbolic stack so we actually get to know what's called in libwebdav? <http://mxr.mozilla.org/mozilla1.8/source/netwerk/protocol/http/src/nsHttpChannel.cpp#4489>

Don't know if timeless' finding is the cause of this problem, but it seems to be a weakness.

Comment on attachment 302880 [details] [diff] [review] [checked in ] fix for timeless finding error checking is good. r=mvl

Checked in on HEAD and MOZILLA_1_8_BRANCH -> FIXED

Actually, since I don't know if the rest of this is fixed, reopening

Does the crash still happens after the checkin from last week?

I have not seen the crash-on-exit since Daniel's patch was applied, but I continue to think that longer-term we're better off without the webdav extension. This patch eliminates our use of it. I had originally thought to use a SAX parser but found parsing with SAX (at least in JavaScript) was markedly slower, so this patch uses plain old e4x; I don't see a speed difference in comparison to the webdav extension. Since I was eliminating/rewriting reportInternal I went ahead and used calIcsParser.js instead of keeping the old in-provider parser, thus fixing bug 395617 (and saving a review, since that bug was next on my list). That's the main reason that this patch shaves off about 150 overall lines-of-code as well as the webdav extension. I'd be very interested to know if this patch has any impact on bug 327933 (412 response on PUTting from ICS provider to WebDAV): I've been able to reproduce that only sporadically before, and not at all with this patch applied. I should also point out that the patch may well be incomplete: I'm not familiar enough with our updating mechanisms to know if I need to be adding anything to removed-files.in

I'm not sure removing the webdav extension is the right way to go. While I do see that the extension is far from complete and due to the implementation language its also a bit less lucrative to fix, but I like the fact that we have a general extension that can be reused at other places outside of calendar to handle webdav. How much performance difference did you get when using SAX? I think the webdav extension could benefit from using sax or similar since its also written in C++. I did take a short look at webdav recently, to try to implement locking. I'm far from finished though. Regarding a js-webdav client, we might want to look into weave's modules/dav.js. While written in a newer javascript version, it does look fairly simple to use and also seems to implement locking.

The performance of a JS SAX parser, for calendar data at least, was pretty dismal. If I recall correctly, my usual test calendar (about 350 items, real-world data imported nightly from my dayjob GroupWise calendar, so lots of longish DESCRIPTIONs, recurrences, etc) took about 30s to load using the webdav extension and just shy of 200s with the JS SAX parser. I like the *idea* of a webdav extension, but in practice I constantly find myself not using it because I need to do something like setRequestHeader, getResponseHeader, assert a new method like MKCALENDAR, etc, which the current extension won't let me do. And whenever I start thinking about a more flexible API for it ... pretty soon I feel like I'm reinventing nsIHttpChannel. Which is well-tested and well-supported and we don't need another one. So I end up doing DAV stuff with nsIHttpChannel and some wrapper functions. That means e4x for parsing responses from REPORT, PROPFIND, etc. Might be there are some wins to be had there using SAX, probably in C++. Overall, I think we can "do" DAV with code that's as clear, concise, correct, and speedy without the webdav extension as with it, and doing so uses better-tested areas of the Mozilla code and frees us from compiling/shipping libs etc. I'm also not convinced that locking is a real issue, for calendar purposes at least. There was at one point talk of MUSTing it in caldav-sched, but that seems to have gone away, in part at least because in the case of PUT-a-single-file (which is what we're doing in both CalDAV and ICS) it's not clear that the LOCK/PUT/UNLOCK cycle has advantages over the simpler "PUT If-Match ETag".

In an ideal world, we'd have a reusable DAV library written in JS using SAX (or some other async XML parser) under the hood which could even be used from web content. Given the SAX-in-JS constraints that Bruno has seen, and after some discussion with thunder (Dan Mills) about the suitability of XMLHttpRequest for async parsing and error handling, that seems a ways off. Happily, though, we have a C++ SAX parser in the tree which is exposed to JS via XPCOM: see <http://developer.mozilla.org/en/docs/SAX> for details. I suspect that a reasonable way forward might be to use that...

Though, as Bruno points out, it might be possible to use the SAX parser in the existing C++ WebDAV code too. It might well also be worth a discussion in mozilla.dev.platform to see if we can find consensus on a DAV solution that works for a sufficient number of stakeholders that it would end up being well-maintained in-tree.

(In reply to comment #19) > Happily, though, we have a C++ SAX parser in the tree which is exposed to JS > via XPCOM: see <http://developer.mozilla.org/en/docs/SAX> for details. I > suspect that a reasonable way forward might be to use that... > Pardon me for being unclear. The SAX parser I was referring to did use nsISAXContentHandler rather than being written entirely in JavaScript. The JS bits were very much along the lines described on the page cited (unsurprising, since that's what I consulted :) ), with a callback on each <response> element very much like the webdav extension's onOperationDetail. So the XML parsing per se was in C++. I described it as being in JS because, after seeing the results, I assumed that to get the speed I expected I'd have to implement the entire REPORT parser in C++, not just call the SAX stuff from JS. This was the first time I've used nsISAXContentHandler, so I suppose it's possible that I was simply doing so maladroitly.

If the speed tests you did were in a debug build, it's entirely possible that the performance characteristics are different enough to not be meaningful. Whether or not that's the case, it's probably worth some more profiling to understand where exactly your speed problems are coming from. asuth has been doing some work to get dtrace playing nice with calendar and may have some ideas there.

Here is a very bare-bones DAV client in JavaScript which uses XHRs for the actual content transfer: http://hg.mozilla.org/labs/weave/index.cgi/file/6f604a64786a/modules/dav.js I just point it out for reference, I don't think it would really suit your needs. I do plan to make it a bit more complete and develop it semi-independently of Weave by putting it here: http://wiki.mozilla.org/Labs/JS_Modules

(In reply to comment #16) > I should also point out that the patch may well be incomplete: I'm not > familiar enough with our updating mechanisms to know if I need to be adding > anything to removed-files.in Bruno, basically everything that you *remove* from packages-static needs to be *put* into removed-files.in

Comment on attachment 314255 [details] [diff] [review] eliminate use of webdav extension > /** >+ * prepare HTTP channel with standard request headers >+ * and upload data/content-type if needed >+ * >+ * @param arUri channel Uri >+ * @param aUploadData data to be uploaded, if any >+ * @param aContentType value for Content-Type header, if any >+ * @param aCalendar calendar using channel >+ */ please align properly >+ >+function calPrepHttpChannel(aUri, aUploadData, aContentType, aCalendar) { I'd prefer to call the last parameter "aNotificationCallbacks". >+ var ioService = Components.classes["@mozilla.org/network/io-service;1"] >+ .getService(Components.interfaces.nsIIOService); please use getIOService() here. >+ var httpchannel = channel.QueryInterface(Components.interfaces >+ .nsIHttpChannel); please align >+ if (aUploadData) { >+ >+ httpchannel = httpchannel.QueryInterface(Components.interfaces. >+ nsIUploadChannel); please align >+ var converter = >+ Components.classes["@mozilla.org/intl/scriptableunicodeconverter"] >+ .createInstance(Components.interfaces.nsIScriptableUnicodeConverter); dto >+/** >+* calSendHttpRequest; send prepared HTTP request >+* >+* @param aStreamLoader streamLoader for request >+* @param aChannel channel for request >+* @param aListener listener for method completion >+*/ please align >+function calSendHttpRequest(aStreamLoader, aChannel, >+ aListener) { please put on the same line >+ if (appInfo.platformVersion.indexOf("1.8") == 0) { >+ aStreamLoader.init(aChannel, aListener, aChannel); Seems appInfo got lost here (although it's defined globally in some context). Please do a lte-init e.g. if (calSendHttpRequest.mIsBranch === undefined) { ... } >Index: calendar/installer/windows/packages-static ... >-bin\components\webdav.dll >-bin\components\webdav.xpt put these into removed-files as Simon already mentioned. >+ var httpchannel = calPrepHttpChannel(itemUri, aItem.icalString, >+ "text/calendar; charset=utf-8", this); please align >- var httpchannel = this.prepChannel(eventUri, modifiedItemICS, >- "text/calendar; charset=utf-8"); >+ var httpchannel = calPrepHttpChannel(eventUri, modifiedItemICS, >+ "text/calendar; charset=utf-8", this); dto >- var httpchannel2 = thisCalendar.prepChannel(eventUri, null, null); >+ var httpchannel2 = calPrepHttpChannel(eventUri, null, null, >+ thisCalendar); dto >+ etagListener.onStreamComplete = >+ function getUpdatedItems_oSC(aLoader, aContext, aStatus, >+ aResultLength, aResult) { >+ var resultConverter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"] >+ .createInstance(Components >+ .interfaces.nsIScriptableUnicodeConverter); dto >+ var streamLoader = Components.classes["@mozilla.org/network/stream-loader;1"] >+ .createInstance(Components.interfaces >+ .nsIStreamLoader); >+ var streamLoader = Components.classes["@mozilla.org/network/stream-loader;1"] >+ .createInstance(Components.interfaces >+ .nsIStreamLoader); dto >+ if (elementStatus != 200) { >+ LOG("got element status " + elementStatus + " while fetching calendar data"); please prefix the LOGs. >+ var parser = Components.classes["@mozilla.org/calendar/ics-parser;1"] >+ .createInstance(Components >+ .interfaces.calIIcsParser); please align and put Components.interfaces.xyz on the same line >+ var locationPath = decodeURIComponent(resourcePath) >+ .substr(thisCalendar.mLocationPath.length); please put on the same line >+ thisCalendar.mItemInfoCache[item.id].locationPath = >+ locationPath; dto >+ if (item instanceof Components.interfaces.calIEvent) { please use isEvent() >+ } else if (item instanceof Components.interfaces.calITodo) { and isToDo() >+ for (var i = 0; i < items.length; i++) { >+ if (thisCalendar.mItemInfoCache[items[i].id]) { >+ thisCalendar.mMemoryCalendar.modifyItem(items[i], null, >+ aListener); please put on same line >+ var httpchannel = calPrepHttpChannel(calendarDirUri, aQuery, >+ "text/xml; charset=utf-8", this); please align >+ var streamLoader = Components.classes["@mozilla.org/network/stream-loader;1"] >+ .createInstance(Components.interfaces >+ .nsIStreamLoader); >+ var streamLoader = Components.classes["@mozilla.org/network/stream-loader;1"] >+ .createInstance(Components.interfaces >+ .nsIStreamLoader); dto >- var webSvc = Components.classes['@mozilla.org/webdav/service;1'] >- .getService(Components.interfaces.nsIWebDAVService); dto >- var httpchannel = this.prepChannel(this.mUri,queryXml, >- "text/xml; charset=utf-8"); >+ var httpchannel = calPrepHttpChannel(this.mUri,queryXml, >+ "text/xml; charset=utf-8", this); dto >- var httpchannel = this.prepChannel(pns, queryXml, >- "text/xml; charset=utf-8"); >+ var httpchannel = calPrepHttpChannel(pns, queryXml, >+ "text/xml; charset=utf-8", this); dto >- var httpchannel = this.prepChannel(outBoxUri, fbQuery, >- "text/calendar; charset=utf-8"); >+ var httpchannel = calPrepHttpChannel(outBoxUri, fbQuery, >+ "text/calendar; charset=utf-8", this); dto >+ etagListener.onStreamComplete = >+ function ics_etLoSC(aLoader, aContext, aStatus, >+ aResultLength, aResult) { >+ >+ var resultConverter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"] >+ .createInstance(Components >+ .interfaces.nsIScriptableUnicodeConverter); dto >+ var etagChannel = calPrepHttpChannel(aChannel.URI, queryXml, >+ "text/xml; charset=utf-8", >+ this); dto r=dbo

Bruno, please remove webdav extension DIR from lightning/Makefile.in, too.

On linux and mac we can remove libwebdav.dylib/libwebdav.so too?. In that case the entry in removed-files.in should contain components/@DLL_PREFIX@webdav@DLL_SUFFIX@

I'm sorry, Daniel, but I don't understand your "lte-init" comment, and searching both google and mxr for both "lte-init" and "mIsBranch" has failed to enlighten me. Could you run that one by me again, a little slower?

No, I need to be sorry, because an "a" is missing in my comment: It should read "do a late-init...". I wanted you to evaluate the appInfo only once like if (calSendHttpRequest.mIsBranch === undefined) { var appInfo = Components.classes["@mozilla.org/xre/app-info;1"] .getService(Components.interfaces.nsIXULAppInfo); calSendHttpRequest.mIsBranch = (appInfo.platformVersion.indexOf("1.8") == 0); } It may even make sense to outsource that code in calUtils.js, e.g. into a function calIsBranch18().

This patch has problems when the remote calendar is behind a self-signed certificate; works but throws errors on branch, unusable on trunk. Delaying commit accordingly.

I assume this is a problem with the notificationCallbacks not providing an interface for things like nsIBadCertListener or similar. If thats the case, I do wonder why this wasn't happening before?

I have set up my webdav share to use SSLVerifyClient require and will soon test this. Bruno, if you have already un-bitrotted the patch, could you attach it? I now notice I misread the self-signed bit, but I think I can get around that by not importing the CAcert root cetificate.

Just for the record, this bug needs to have the caldav fix for bug 338527 for an r+.

posting update WIP patch per Fallen request. De-bitrotted, some cruft removed from getCalendarData() On branch * still get "JavaScript component does not have method named getInterface" errs on PUT to ICS with my self-signed, domain-mismatch, expired cert. Doubtless some idiocy on my part that I haven't twigged to yet * CalDAV no longer shows this error but periodically re-presents the "cannot verify identity" dialogset on PUT with that cert, even after "accept for this session" Not yet tested on trunk

This seems to do it. No further errors on branch (the extraneous "cannot verify identity" prompts mentioned in comment #34 were seen on a machine under high load and have disappeared under more normal load; likely some kind of race). Also tested on trunk. User has to know to go to preferences to add the exception, but once that's done trunk works fine.

Comment on attachment 330885 [details] [diff] [review] add necessary stubs > } >+ >+ >+/** Too many newlines here. >+ */ >+ >+function calPrepHttpChannel(aUri, aUploadData, aContentType, aNotificationCallbacks) { Also newline not needed. Anyway I'm unsure we should put this into calUtils. I'd opt for a calProviderUtils.js where such functions can go into. >+ var httpchannel = channel.QueryInterface(Components.interfaces >+ .nsIHttpChannel); ... >+ httpchannel = httpchannel.QueryInterface(Components.interfaces >+ .nsIUploadChannel); Either don't wrap, or align the dots of .interfaces and .nsIHtttp... >+ var httpchannel = calPrepHttpChannel(itemUri, aItem.icalString, >+ "text/calendar; charset=utf-8", this); Wrap parameters here and in similar places >+ calSendHttpRequest(streamLoader, httpchannel, addListener); > > return; > } The return here is not needed, please remove while you are at it. This happens more often, please check the file >+ var resultConverter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"] >+ .createInstance(Components >+ .interfaces.nsIScriptableUnicodeConverter); Strange wrapping, please fix >+ try { >+ LOG("CalDAV: Status " + aContext.responseStatus + >+ " on getetag for calendar " + thisCalendar.name); >+ responseStatus = aContext.responseStatus; >+ } catch(ex) { >+ LOG("CalDAV: Error without status on getetag for calendar " + >+ thisCalendar.name); >+ responseStatus = "none"; Indentation is wrong here >+ } >+ var query = thisCalendar.mQueuedQueries.pop(); >+ thisCalendar.mMemoryCalendar.getItems(query[0], query[1], >+ query[2], query[3], >+ query[4]); thisCalendar.mMemoryCalendar.getItems.apply(thisCalendar.mMemoryCalendar, query); > getInterface: function(iid) { If you like do it now, but feel free to do in a different bug. Lots of these if branches do the same thing, you could consolidate that into less, see gdata. >+ // nsIChannelEventSink implementation >+ onChannelRedirect: function(aOldChannel, aNewChannel, aFlags) { >+ }, >+ We don't do anything on redirect? Do we need to (re-)prepare the http channel in >+ // nsIProgressEventSink >+ onProgress: function onProgress(aRequest, aContext, aProgress, aProgressMax) {}, >+ onStatus: function onStatus(aRequest, aContext, aStatus, aStatusArg) {}, Why do we need nsIProgressEventSink if we don't implement it? r=philipp with style fixed and questions answered.

I've applied my checkin comments and fixed some additional style in and around the changed lines.

Next, I'll be taking care of de-bitrotting the sched patch. Checked in on HEAD and MOZILLA_1_8_BRANCH -> FIXED

Philipp, have you also fixed (and tested) the 'Automatic proxy configuration' issue for CalDAV?

(In reply to comment #39) > Philipp, have you also fixed (and tested) the 'Automatic proxy configuration' > issue for CalDAV? > I'm not quite sure its fixed just by adding the method from the interface. I assume that in the onChannelRedirect() function we need to re-prepare the new channel. It would be nice if someone could test if it works and possibly also if thats enough.

Philipp, please fix the following new warnings: Warning: anonymous function does not always return a value Source File: file:///Users/dbo/moz18/sbird-debug_Darwin/dist/xpi-stage/lightning/js/calICSCalendar.js Line: 941, Column: 4 Source Code: } Warning: trailing comma is not legal in ECMA-262 object initializers Source File: file:///Users/dbo/moz18/sbird-debug_Darwin/dist/xpi-stage/lightning/js/calDavCalendar.js Line: 1618 Source Code: }; since the needed changes are trivial, r=me

Writing ics files has regressed, calPrepChannel not defined.

Comment on attachment 331291 [details] [diff] [review] [checked in] regression fix Looks good, sorry I missed that.