Closed Bug 1581909 Opened 5 years ago Closed 5 years ago

Illegal value 'Illegal value' when calling method: [calIRecurrenceInfo::modifyException]

Categories

(Calendar :: Lightning Only, defect)

Lightning 71
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: aleca, Assigned: darktrojan)

References

(Regression)

Details

Attachments

(2 files, 1 obsolete file)

Multiple console errors show up as soon as Thunderbird gets launched:

console.warn: Lightning: Message: [Exception... "Illegal value'Illegal value' when calling method: [calIRecurrenceInfo::modifyException]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: file:///home/alecaddd/Mozilla/source/obj-x86_64-pc-linux-gnu/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calStorageCalendar.js :: _assureRecurringItemCaches/< :: line 1751" data: no]

Can you provide steps to reproduce? I'm testing with 71.0a1 (Build ID: 20190917085608). I created multiple repeating events and modified them and I'm not seeing this error.

Version: Trunk → Lightning 71

The errors appear on startup, even if the calendar tab is closed.
I have 2 calendars, 1 local and 1 remote from Fastmail.

I just pulled this morning from m-c and c-c.
Do you need any specific info?

Flags: needinfo?(geoff)
Regressed by: 501689

I found a bug that is related, not sure if it's the reported one though.

Assignee: nobody → geoff
Status: NEW → ASSIGNED
Flags: needinfo?(geoff)
Attachment #9093466 - Flags: review?(paul)
Attachment #9093466 - Flags: feedback?(alessandro)
Comment on attachment 9093466 [details] [diff] [review]
1581909-async-recurrenceinfo-1.diff

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

Same problems, but with a slightly bigger error message.

`console.error: Lightning: 
  recurrenceInfo::addException: item with null recurrenceId!
JavaScript error: file:///home/alecaddd/Mozilla/source/obj-x86_64-pc-linux-gnu/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calStorageCalendar.js, line 1754: uncaught exception: 2147942487`

`console.warn: Lightning: Message: [Exception... "Illegal value'Illegal value' when calling method: [calIRecurrenceInfo::modifyException]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///home/alecaddd/Mozilla/source/obj-x86_64-pc-linux-gnu/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calStorageCalendar.js :: _assureRecurringItemCaches/< :: line 1754"  data: no]
`
Attachment #9093466 - Flags: feedback?(alessandro) → feedback-
Attached patch 1581909-async-clearcache-1.diff (obsolete) — — Splinter Review

In hindsight this makes sense, we should empty the cache out before trying to fill it up again. The complaining code is finding something in the cache that shouldn't be there.

I never realised just how many times we call _assureRecurringItemCaches – four at start-up for a calDAV calendar with offline storage. That's got to be bad for performance.

Attachment #9093488 - Flags: review?(paul)
Attachment #9093488 - Flags: feedback?(alessandro)
Comment on attachment 9093488 [details] [diff] [review]
1581909-async-clearcache-1.diff

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

This does the trick for me!
Attachment #9093488 - Flags: feedback?(alessandro) → feedback+
Comment on attachment 9093488 [details] [diff] [review]
1581909-async-clearcache-1.diff

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

With these two patches applied, the errors no longer appear.  I'm not well versed in this part of the code, but the change seems reasonable.
Attachment #9093488 - Flags: review?(paul) → review+
Comment on attachment 9093466 [details] [diff] [review]
1581909-async-recurrenceinfo-1.diff

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

LGTM.
Attachment #9093466 - Flags: review?(paul) → review+

You want those landed?

Flags: needinfo?(geoff)

I think we should get them landed.

Flags: needinfo?(geoff)
Keywords: checkin-needed
Comment on attachment 9093466 [details] [diff] [review]
1581909-async-recurrenceinfo-1.diff

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

::: calendar/providers/storage/calStorageCalendar.js
@@ +1735,5 @@
> +      let recInfo = item.recurrenceInfo;
> +      if (!recInfo) {
> +        recInfo = cal.createRecurrenceInfo(item);
> +        item.recurrenceInfo = recInfo;
> +      }

this would be shorter:

if (!item.recurrenceInfo) {
  item.recurrenceInfo = cal.createRecurrenceInfo(item);
}

I'd prefer Geoff to tell use what he wants to do, maybe even change the snippet you've pointed out.

Flags: needinfo?(geoff)
Keywords: checkin-needed

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/d52c5f098152
Use existing item recurrence info instead of overwriting it; r=pmorris
https://hg.mozilla.org/comm-central/rev/2fd4b75c6950
Clear in-memory item cache before re-filling it; r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: needinfo?(geoff)
Target Milestone: --- → 71

https://hg.mozilla.org/comm-central/rev/2148bb88117e96f83d4c780f2707fc44ed2423f8
Backed out 2 changesets (bug 1581909) for test failures in calendar/test/unit/test_providers.js. a=backout DONTBUILD

Status: RESOLVED → REOPENED
Flags: needinfo?(geoff)
Resolution: FIXED → ---

I've seen so much red/orange in the last few hours that I'm confused what's working and what is not. Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ec7a23f9bcfd63e88fb50a05a8f91c72b7885dc7

Attached patch 1581909-async-clearcache-2.diff — — Splinter Review

IMO it's actually the test that's wrong, but I still should've checked it before pushing. :-/

This is a better fix anyway. It only clears items from mItemCache that are going to be replaced at the end of the function. Also I realised we've never cleared mRecEventCache/mRecEventCacheOfflineFlags/mRecTodoCache/mRecTodoCacheOfflineFlags before filling them which could lead to ghost entries. I've never seen a problem with this before but it should be fixed anyway.

Attachment #9093488 - Attachment is obsolete: true
Flags: needinfo?(geoff)
Attachment #9094051 - Flags: review?(paul)
Comment on attachment 9094051 [details] [diff] [review]
1581909-async-clearcache-2.diff

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

The changes look fine to me.
Attachment #9094051 - Flags: review?(paul) → review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/cf15df787db9
Use existing item recurrence info instead of overwriting it. r=pmorris
https://hg.mozilla.org/comm-central/rev/a652580e14da
Clear in-memory item cache before re-filling it. r=pmorris

Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: