788004 - No email send after invitation creation if offline cache is enabled [Exception... "'TypeError: aItem.calendar.canNotify is not a function' when calling method: [calIOperationListener::onOperationComplete]"

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Jan]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120209155602

Steps to reproduce:

Platform: Windows 7 - with enabled cache options in the Preferences of calendar create event in the network CalDav calendar and invite attendees 


Actual results:

No event invitation is send. Error console shows:

Fehler: [Exception... "'TypeError: aItem.calendar.canNotify is not a function' when calling method: [calIOperationListener::onOperationComplete]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "JS frame :: resource://calendar/modules/calUtils.jsm -> file:///C:/Users/jlange/AppData/Roaming/Thunderbird/Profiles/8umop2ul.default/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calCachedCalendar.js :: <TOP_LEVEL> :: line 711"  data: no] 

Workarround: disable the cache option and the email is send. But then you can't use the offline version of the calendar.


Expected results:

after creating the event the Outlook compatibility dialog is shown and afterwards the email is send.

Comment 1

[alo]

Same on :
ubuntu 12.04
TB 15.0
Lightning 1.7 

The workaround... works!

Comment 2

[Jan Ole Kastens]

I have the same bug with the same error message on
3.2.0-30-generic #48-Ubuntu SMP Fri Aug 24 16:52:48 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux
TB 15.0
Lightning 1.7
The workaround works for me too

Comment 3

[Stefan Sitter [:ssitter]]

Confirming per duplicated reports.

Comment 5

[Stefan Sitter [:ssitter]]

The cached calendar provider probably needs a similar fix as the storage provider in Bug 762854, i.e. it needs to implement and announce Components.interfaces.calISchedulingSupport.

Mohit, would you have time to take a look at this as you were involved in the latest changes to the cached calendar provider?

Comment 6

[Mohit Kanwal [:redDragon]]

Hi Stefan, I will take a look into this. Do I need to just announce the calISchedulingSupport in calCachedCalendar.js? 

CalDAV does seem to implement the calISchedulingSupport Interface [1]

[1] http://mxr.mozilla.org/comm-central/source/calendar/providers/caldav/calDavCalendar.js

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

You possibly need to forward the methods to the inner calendar, see the forward methods at the bottom of calCachedCalendar.js

While you are at it, could you check all other providers that implement the interface to make sure they are properly advertising the interface?

Comment 10

[Mohit Kanwal [:redDragon]]

Attachment: [HACK] Patch to cleanly apply on beta

This patch takes care of having a hackish fix for the beta branch. The wrappedJSObject apparently is not being cast into a calISchedulingSupport object. For example,
// works:

let foo = aItem.calendar;
(foo instanceof Ci.calISchedulingSupport);
// works:
let foo = aItem.calendar;
(foo instanceof Ci.calISchedulingSupport);
cal.calInstanceOf(aItem.calendar, Ci.calISchedulingSupport) && aItem.calendar.canNotify();

// fails:
cal.calInstanceOf(aItem.calendar, Ci.calISchedulingSupport) && aItem.calendar.canNotify();

Will upload another patch to fix all the calInstanceOf calls soon.

Comment 11

[Mohit Kanwal [:redDragon]]

Attachment: Patch to cleanly apply on beta [HACK]

Oops! uploaded a wrong copy.

Comment 12

[info]

I introduced the patch and still no invites are sent out.
Maybe it's because I used the stable v.17 and not the beta.

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

hbarel, are you getting any error console messages?

Comment 14

[info]

Warnings and errors included:

Timestamp: 02/10/12 14:04:37
Warning: XUL box for _moz_generated_content_before element contained an inline #text child, forcing all its children to be wrapped in a block.
Source File: chrome://messenger/content/messenger.xul
Line: 0

Timestamp: 02/10/12 14:04:38
Warning: XUL box for _moz_generated_content_before element contained an inline #text child, forcing all its children to be wrapped in a block.
Source File: chrome://global/content/bindings/toolbar.xml
Line: 276

Timestamp: 02/10/12 14:04:42
Warning: Use of Mutation Events is deprecated. Use MutationObserver instead.
Source File: chrome://calendar/content/calendar-event-dialog-attendees.js
Line: 29

Timestamp: 02/10/12 14:05:01
Error: calendarURI is null
Source File: file:///l:/thunderbird/profile/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calDavCalendar.js
Line: 1136

Timestamp: 02/10/12 14:05:01
Error: syntax error
Source File: moz-nullprincipal:{5f7d48dd-0040-40b2-aa5a-48ad5fad5e05}
Line: 1, Column: 1
Source Code:
Exception [0] Dodgy looking namespace from 'CALDAV:schedule-send-invite'!

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Could you try this again with the beta and also enable calendar.debug.log and calendar.debug.log.verbose? This looks like a server issue with the namespaces.

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central changeset b8a344f591b3

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-aurora changeset dc19a6076430

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-beta changeset df78399be401

Comment 19

[Mohit Kanwal [:redDragon]]

Attachment: Patch that removes all calInstanceOf calls

This patch takes care of removing all the calInstanceOf calls which was the source of issue for the bug. The instanceof method can be cryptic at times (as it was not casting calISchedulingSupport objects) and as such the function calInstanceOf has been removed in this patch. Instead a new function cal.wrapInstance has been added inside of calUtils module which only does QueryInterface and returns the wrapped object. Help me check if this does not break anything.

Comment 20

[info]

Not on beta yet, but with verbose logging I get the following errors:

Timestamp: 02/10/12 17:08:58
Error: calendarURI is null
Source File: file:///l:/thunderbird/profile/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calDavCalendar.js
Line: 1136

CalDAV: Sending iTIP failed with status 500 for Dcal

Timestamp: 02/10/12 17:08:58
Error: syntax error
Source File: moz-nullprincipal:{0af0ad2c-83b0-400d-ac95-0a4661f609b4}
Line: 1, Column: 1
Source Code:
Exception [0] Dodgy looking namespace from 'CALDAV:schedule-send-invite'!

The CalDAV server is mine, and it has not changed since the previous version of lightning when it still worked, so I don't think it's a server issue.

Comment 21

[info]

Found the source of the problem. It was not the server but another, unrelated bug which caused the calendar cache data to be corrupt. Completely unrelated to this issue. Sorry.

Comment 23

[Mohit Kanwal [:redDragon]]

So it works now after introducing the patch?

Comment 24

[psmith]

Regarding bug 796226 that was marked as a duplicate of this bug, I disabled cache and now Lightning sends two meeting invites to attendees.  One has the reminder set and one does not.  It is a problem if attendee only accepts the invite with no reminder set.

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

This bug was only recently fixed, please retest with the release builds next week.

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 667002 [details] [diff] [review]
Patch that removes all calInstanceOf calls

Review of attachment 667002 [details] [diff] [review]:
-----------------------------------------------------------------

r=philipp with the following changes. Also, please push this to comm-aurora. Depending on when our beta builds run we might want to consider doing this on beta too, but lets see.

::: calendar/base/content/dialogs/calendar-dialog-utils.js
@@ +440,4 @@
>  
>          // Only show if its either an internal protcol handler, or its external
>          // and there is an external app for the scheme
> +        let handlerWrappedInstance = cal.wrapInstance(handler, Components.interfaces.nsIExternalProtocolHandler);  

Looks like there are some extra whitespaces here.

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +50,1 @@
>                      rule = rules[0];

You could just do this:

rule = calWrapInstance(rules[0], Components.interfaces.calIRecurrenceRule);

::: calendar/base/content/dialogs/calendar-invitations-list.xml
@@ +201,4 @@
>          <parameter name="aItem"/>
>          <parameter name="aStatus"/>
>          <body><![CDATA[
> +          aItem.calendar = cal.wrapInstance(aItem.calendar, Components.interfaces.calISchedulingSupport);

I'd avoid re-assigning the calendar here, but instead do 

let calendar = cal.wrapInstance(...);

if (calendar) {
  // and change all other aitem.calendar things here.

::: calendar/providers/composite/calCompositeCalendar.js
@@ +319,4 @@
>      mCompositeObservers: null,
>      mObservers: null,
>      addObserver: function (aObserver) {
> +        let aCompositeObserver = cal.wrapInstance(aObserver, Components.interfaces.calICompositeObserver);

the "a" prefix is usually meant for arguments. I'd suggest naming it compositeObserver

@@ +327,5 @@
>      },
>  
>      // void removeObserver( in calIObserver observer );
>      removeObserver: function (aObserver) {
> +        let aCompositeObserver = cal.wrapInstance(aObserver, Components.interfaces.calICompositeObserver);

Same here.

::: calendar/providers/storage/calStorageCalendar.js
@@ +2116,4 @@
>              this.prepareStatement(this.mInsertProperty);
>              var pp = this.mInsertProperty.params;
>              pp.key = propName;
> +            let propDateTimeVal = cal.wrapInstance(propValue, Components.interfaces.calIDateTime); 

Extra whitespace here.

::: calendar/providers/wcap/calWcapCalendarItems.js
@@ +502,4 @@
>              if (attachments) {
>                  var strings = [];
>                  for each (var att in attachements) {
> +                    let aAtt = cal.wrapInstance(att, Components.interfaces.calIAttachment); 

Extra whitespace here, also same comment about the "a" prefix. Use something like attendeeAtt

@@ +977,5 @@
>              for each (let recItem in recItems) {
>                  // cs bug: workaround missing COUNT
> +                if (cal.wrapInstance(recItem, Components.interfaces.calIRecurrenceRule)) {
> +                    // Possible performance issue double QueryInterface calls
> +                    recItem = recItem.QueryInterface(Components.interfaces.calIRecurrenceRule); 

Extra whitespace

Comment 28

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 666578 [details] [diff] [review]
Patch to cleanly apply on beta [HACK]

This was already checked in.

Comment 29

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 667002 [details] [diff] [review]
Patch that removes all calInstanceOf calls

Mohit, I think this patch was never actually checked in. Could you de-bitrot it and push it to comm-central and comm-aurora?

Comment 30

[Philipp Kewisch [:Fallen][📱📆]]

Mohit, do you think you could take care of comment 29?

Comment 31

[Mohit Kanwal [:redDragon]]

Attachment: Clean Patch for removing calInstance calls

Sorry about the oversight,I have updated the patch to apply cleanly on trunk. Phillip help me push this through, I think I don't have pushing rights =)

Comment 32

[Mohit Kanwal [:redDragon]]

Attachment: Warning message for calInstanceOf function

Since the function calInstanceOf is being unsupported from next release onwards therefore adding a short Warning message for developers to see in the console, informing them about the change.

Comment 33

[Mohit Kanwal [:redDragon]]

Attachment: Cleaner Patch for removal of calInstanceOf calls

The previous patch a bit of whitespaces around which I had ignored. Cleaned the patch to apply cleanly on current tree.

Comment 34

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 700524 [details] [diff] [review]
Cleaner Patch for removal of calInstanceOf calls

Patch looks fine, r=philipp

Comment 35

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 700423 [details] [diff] [review]
Warning message for calInstanceOf function

I was thinking that we should introduce the new function wrapInstance (so push the other patch), but add in a function calInstanceOf that shows the warning (once) and then call wrapInstance.

This way we can get rid of the function early in our code, but still allow extensions to transition to the new function. I'll upload a new patch to show what I mean.

Comment 36

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Warning message for calInstanceOf function - v2

Comment 37

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Warning message for calInstanceOf function - v2

Comment 38

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central changeset f339d89a90f8

Comment 39

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central changeset 905498205868

Comment 40

[Philipp Kewisch [:Fallen][📱📆]]

Sorry, there are a few errors with this patch, we will need to either fix it within the next few days or back it out. Just a few problems I've found:

* Open a readonly event in the summary dialog (no details visible, cannot close)
* Cannot accept email event invitation

Mohit, can you take care of this and doublecheck that all touched code locations still function?

Comment 41

[Mohit Kanwal [:redDragon]]

Ah gross oversight on my part :-(

I put rule = calWrapInstance(rules[0], Components.interfaces.calIRecurrenceRule); instead of cal.WrapInstance ... for calendar-event-dialog-recurrence.js

I will double check that this does not break anything and upload a new one to apply on trunk by tonight :-)

Comment 42

[Mohit Kanwal [:redDragon]]

I realised doing this was wrong:

> aCalendar = cal.wrapInstance(aItem.calendar, Components.interfaces.calISchedulingSupport);
> if (aCalendar) {
> ...

coz if the wrapInstance function fails the aCalendar variable will have null value while the code below the above line might require aCalendar to not be null but perhaps a different object type. wrapInstance returns a wrappedInstance after QueryInterface otherwise null.

Uploading a cleaner patch with such instances removed.

Comment 43

[Mohit Kanwal [:redDragon]]

Attachment: Correct the previous patch's behavior

Applies on top of the previously pushed patch.

I have tested through the scenarios as pointed out in comment 40 and found it to work properly without any errors now. Will appreciate a bit of testing so that this does not cause any new errors :-)

Comment 44

[Stefan Sitter [:ssitter]]

Could this patch be responsible for Bug 842150 regression too? If yes: Will it be fixed before the branch uplift or will the patch that broke things be backed out?

The error in Bug 842150 points to a line |return (cal.wrapInstance(aObject, Components.interfaces.calIEvent) != null);|

Comment 45

[Mohit Kanwal [:redDragon]]

Yes it is definitely caused by this patch. comment 42 has the reason why.

Can try to apply patch attachment 713857 [details] [diff] [review] and see if it solves the issue.

The latest patch needs to be checked in to prevent a number of such errors from popping out.

Comment 46

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 713857 [details] [diff] [review]
Correct the previous patch's behavior

Review of attachment 713857 [details] [diff] [review]:
-----------------------------------------------------------------

Since the merge is tomorrow, I'm going to back out the original change for now. We can then push both patches at once when all issues are out of the way.

I'm going to have to r- for now, I think this is not quite correct:

::: calendar/base/content/agenda-listbox.js
@@ +602,4 @@
>  function refreshCalendarQuery(aStart, aEnd, aCalendar) {
>      let pendingRefresh = this.pendingRefresh;
>      if (pendingRefresh) {
> +        if (cal.wrapInstance(pendingRefresh, Components.interfaces.calIOperation)) {

Hmm isn't this exactly what we were trying to avoid by removing calInstanceOf?

There were circumstances where:

if (calInstanceOf(pendingRefresh, Ci.calIOperation)) {
  pendingRefresh.cancel(null);
}

didn't work because the pendingRefresh is not successfully QI'd to calIOperation. We wanted to work around this by replacing it with the method wrapInstance, that returns the QI'd instance, which can then be used correctly.

I think rather than changing all those instances, you should call it like so:

let pendingRefresh = cal.wrapInstance(this.pendingRefresh, Components.interfaces.calIOperation);
if (pendingRefresh) {
   ...
}

To make this work in all situations, there are two additional things you can do:

* Add documentation to cal.wrapInstance as to how it should be used
* Add a check to wrapInstance to return null if the passed object is null.

Comment 47

[Philipp Kewisch [:Fallen][📱📆]]

Backed out attachment 700524 [details] [diff] [review] https://hg.mozilla.org/comm-central/rev/c1d9ed13f285

Comment 48

[Philipp Kewisch [:Fallen][📱📆]]

Mohit, could you fix these issues?

Comment 49

[Mohit Kanwal [:redDragon]]

Attachment: Fix for removal of calInstanceOf calls

Updated patch. So far no issues upon applying it to the trunk.

Comment 50

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 756468 [details] [diff] [review]
Fix for removal of calInstanceOf calls

Review of attachment 756468 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/modules/calUtils.jsm
@@ +637,5 @@
> +     *   ...
> +     *   pendingRefresh.onResult(...) // Assured of pendingRefresh to be of calIOperation Type
> +     * }
> +     * 
> +     * 

Some trailing whitespace issues here.

Also, maybe you could add a note on how to NOT use it. Consider this as a replacement for the current usage text:

Use this function to QueryInterface the object to a particular interface. You may only expect the return value to be wrapped, not the original passed object. For example: 

// BAD USAGE:
if (cal.wrapInstance(foo, Ci.nsIBar)) {
   foo.barMethod();
}
// GOOD USAGE:
foo = cal.wrapInstance(foo, Ci.nsIBar);
if (foo) {
  foo.barMethod();
}

@@ +648,5 @@
> +        } catch (e) {
> +            return null;
> +        }
> +    },
> +    

Extra newline and trailing whitespaces

::: calendar/providers/gdata/components/calGoogleUtils.js
@@ +589,4 @@
>                      // EXDATES require special casing, since they might contain
>                      // a TZID. To avoid the need for conversion of TZID strings,
>                      // convert to UTC before serialization.
> +                    prop.valueAsDatetime = wrappedRItem.date.getInTimezone(UTC());

Mind changing this to cal.UTC() (optional)

::: calendar/providers/storage/calStorageCalendar.js
@@ +2099,5 @@
>              this.prepareStatement(this.mInsertProperty);
>              var pp = this.mInsertProperty.params;
>              pp.key = propName;
> +            let wPropValue = cal.wrapInstance(propValue, Components.interfaces.calIDateTime);
> +            if (wPropValue) {

I guess its fine like this, but theoretically you could just do:

propValue = cal.wrapInstance(propValue, Components.interfaces.calIDateTime);

Might also be the case some other times (but not always!). This is an optional review comment.

Comment 51

[Mohit Kanwal [:redDragon]]

Comment on attachment 756468 [details] [diff] [review]
Fix for removal of calInstanceOf calls

Review of attachment 756468 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/providers/storage/calStorageCalendar.js
@@ +2099,5 @@
>              this.prepareStatement(this.mInsertProperty);
>              var pp = this.mInsertProperty.params;
>              pp.key = propName;
> +            let wPropValue = cal.wrapInstance(propValue, Components.interfaces.calIDateTime);
> +            if (wPropValue) {

I thought about it, but in some cases where there is an additional logic (if-else) and let's say if wrapInstance fails, then we don't end up changing the value of propValue to null. So I thought just to create an additional variable for the wrapped purpose, although I agree that it creates some confusion.

Comment 52

[Mohit Kanwal [:redDragon]]

Attachment: Updated Fix-2

Updated Patch with the above review taken into account.

Comment 53

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 757017 [details] [diff] [review]
Updated Fix-2

Review of attachment 757017 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=philipp. You have commit access, right?

Comment 54

[Mohit Kanwal [:redDragon]]

Nope don't have commit access yet.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

This patch has no commit message and the bug history makes it rather unclear as to what it should be. If you're going to request checkin, please ensure that the patch is suitable first.

Comment 56

[Mohit Kanwal [:redDragon]]

Attachment: Patch for checkin

Apologies on my oversight. Corrected the patch with the necessary information.

Comment 57

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 780707 [details] [diff] [review]
Patch for checkin

approval for aurora, and also beta depending on if the tree opens in time before the merge today.

Comment 58

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 780707 [details] [diff] [review]
Patch for checkin

Also approving for beta since the merge has happened.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/b532ed24389a

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/comm-aurora/rev/a8c46631e0dc
https://hg.mozilla.org/releases/comm-beta/rev/ef99175a4e60

Comment 61

[Stefan Sitter [:ssitter]]

Seems this checkin regressed Bug 902916.