Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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]"

RESOLVED FIXED in 2.6

Status

Calendar
Provider: CalDAV
--
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Jan, Assigned: redDragon)

Tracking

(Blocks: 1 bug, {regression})

Lightning 1.7
regression
Dependency tree / graph

Details

Attachments

(1 attachment, 11 obsolete attachments)

45.10 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
OS: All → Windows 7
Hardware: All → x86_64
(Reporter)

Updated

5 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
(Reporter)

Updated

5 years ago
Severity: normal → major
(Reporter)

Updated

5 years ago
Severity: major → critical

Comment 1

5 years ago
Same on :
ubuntu 12.04
TB 15.0
Lightning 1.7 

The workaround... works!

Comment 2

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

5 years ago
Confirming per duplicated reports.
Blocks: 462277
Severity: critical → major
Status: UNCONFIRMED → NEW
Component: Lightning Only → Provider: CalDAV
Ever confirmed: true
Summary: No email send after invitation creation [Exception... "'TypeError: aItem.calendar.canNotify is not a function' when calling method: [calIOperationListener::onOperationComplete]" → 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]"

Updated

5 years ago
Duplicate of this bug: 787350

Comment 5

5 years ago
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?
Keywords: regression
(Assignee)

Comment 6

5 years ago
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
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?

Updated

5 years ago
Duplicate of this bug: 792244

Updated

5 years ago
Duplicate of this bug: 793993
Assignee: nobody → mohit.kanwal
(Assignee)

Comment 10

5 years ago
Created attachment 666576 [details] [diff] [review]
[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.
Attachment #666576 - Flags: review?(philipp)
(Assignee)

Comment 11

5 years ago
Created attachment 666578 [details] [diff] [review]
Patch to cleanly apply on beta [HACK]

Oops! uploaded a wrong copy.
Attachment #666576 - Attachment is obsolete: true
Attachment #666576 - Flags: review?(philipp)
Attachment #666578 - Flags: review?(philipp)

Comment 12

5 years ago
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.
hbarel, are you getting any error console messages?

Comment 14

5 years ago
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'!
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.
Pushed to comm-central changeset b8a344f591b3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0
Backported to releases/comm-aurora changeset dc19a6076430
Target Milestone: 2.0 → 1.9
Backported to releases/comm-beta changeset df78399be401
Target Milestone: 1.9 → 1.8
(Assignee)

Comment 19

5 years ago
Created attachment 667002 [details] [diff] [review]
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.
Attachment #667002 - Flags: review?(philipp)

Comment 20

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

5 years ago
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.

Updated

5 years ago
Duplicate of this bug: 796226
(Assignee)

Comment 23

5 years ago
So it works now after introducing the patch?

Comment 24

5 years ago
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.
This bug was only recently fixed, please retest with the release builds next week.

Updated

5 years ago
Duplicate of this bug: 799055
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
Attachment #667002 - Flags: review?(philipp)
Attachment #667002 - Flags: review+
Attachment #667002 - Flags: approval-calendar-aurora+
Comment on attachment 666578 [details] [diff] [review]
Patch to cleanly apply on beta [HACK]

This was already checked in.
Attachment #666578 - Flags: review?(philipp) → review+
Attachment #666578 - Flags: checkin+
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?
Mohit, do you think you could take care of comment 29?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 31

5 years ago
Created attachment 692752 [details] [diff] [review]
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 =)
Attachment #667002 - Attachment is obsolete: true
Attachment #692752 - Flags: review?(philipp)
(Assignee)

Comment 32

5 years ago
Created attachment 700423 [details] [diff] [review]
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.
Attachment #700423 - Flags: review?(philipp)
(Assignee)

Comment 33

5 years ago
Created attachment 700524 [details] [diff] [review]
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.
Attachment #692752 - Attachment is obsolete: true
Attachment #692752 - Flags: review?(philipp)
Attachment #700524 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Whiteboard: [needs-checkin]
Comment on attachment 700524 [details] [diff] [review]
Cleaner Patch for removal of calInstanceOf calls

Patch looks fine, r=philipp
Attachment #700524 - Flags: review?(philipp) → review+
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.
Attachment #700423 - Flags: review?(philipp) → review-
Created attachment 712373 [details] [diff] [review]
Warning message for calInstanceOf function - v2
Attachment #700423 - Attachment is obsolete: true
Attachment #712373 - Flags: review+
Created attachment 712374 [details] [diff] [review]
Warning message for calInstanceOf function - v2
Attachment #712373 - Attachment is obsolete: true
Attachment #712374 - Flags: review+
Pushed to comm-central changeset f339d89a90f8
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: 1.8 → 2.3
Pushed to comm-central changeset 905498205868
Whiteboard: [needs-checkin]
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 41

5 years ago
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 :-)
(Assignee)

Comment 42

5 years ago
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.
(Assignee)

Comment 43

5 years ago
Created attachment 713857 [details] [diff] [review]
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 :-)
Attachment #713857 - Flags: review?(philipp)

Comment 44

5 years ago
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);|
(Assignee)

Comment 45

5 years ago
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 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.
Attachment #713857 - Flags: review?(philipp) → review-
Backed out attachment 700524 [details] [diff] [review] https://hg.mozilla.org/comm-central/rev/c1d9ed13f285
Target Milestone: 2.3 → ---
Mohit, could you fix these issues?
(Assignee)

Comment 49

4 years ago
Created attachment 756468 [details] [diff] [review]
Fix for removal of calInstanceOf calls

Updated patch. So far no issues upon applying it to the trunk.
Attachment #700524 - Attachment is obsolete: true
Attachment #712374 - Attachment is obsolete: true
Attachment #713857 - Attachment is obsolete: true
Attachment #756468 - Flags: review?(philipp)
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.
Attachment #756468 - Flags: review?(philipp) → review+
(Assignee)

Comment 51

4 years ago
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.
(Assignee)

Comment 52

4 years ago
Created attachment 757017 [details] [diff] [review]
Updated Fix-2

Updated Patch with the above review taken into account.
Attachment #757017 - Flags: review?(philipp)
Status: REOPENED → ASSIGNED
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?
Attachment #757017 - Flags: review?(philipp) → review+
(Assignee)

Comment 54

4 years ago
Nope don't have commit access yet.
Keywords: checkin-needed
Attachment #756468 - Attachment is obsolete: true
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.
Keywords: checkin-needed
(Assignee)

Comment 56

4 years ago
Created attachment 780707 [details] [diff] [review]
Patch for checkin

Apologies on my oversight. Corrected the patch with the necessary information.
Attachment #757017 - Attachment is obsolete: true
Attachment #780707 - Flags: review?(philipp)
Attachment #780707 - Flags: review?(philipp) → review+
Keywords: checkin-needed
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.
Attachment #780707 - Flags: approval-calendar-aurora+
Comment on attachment 780707 [details] [diff] [review]
Patch for checkin

Also approving for beta since the merge has happened.
Attachment #780707 - Flags: approval-calendar-beta+
Attachment #666578 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/b532ed24389a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
https://hg.mozilla.org/releases/comm-aurora/rev/a8c46631e0dc
https://hg.mozilla.org/releases/comm-beta/rev/ef99175a4e60
Target Milestone: --- → 2.6

Updated

4 years ago
Depends on: 902916

Comment 61

4 years ago
Seems this checkin regressed Bug 902916.

Updated

4 years ago
Depends on: 938455
You need to log in before you can comment on or make changes to this bug.