1528133 - Calendar functionality failing with objects needing QueryInterface

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Something to do with the component registration changes has broken the calendar startup service.

Comment 1

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/42fae7cba14d
Fix calendar startup by QIing services to calIStartupService; rs=bustage-fix CLOSED TREE

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8ec819a812a7
Add more QueryInterface calls to fix broken calendar views; rs=bustage-fix CLOSED TREE

Comment 3

[Geoff Lankow (:darktrojan)]

Okay, that should fix all the calendar tests except testLocalICS.js – I don't know what's wrong with that.

Comment 4

[Geoff Lankow (:darktrojan)]

But wait there's more‽ I didn't realise the XPCShell tests were busted too.

Comment 5

[Jorg K (CEST = GMT+2)]

Badly, so I switched the whole lot off:
https://hg.mozilla.org/comm-central/rev/9ea7d639422237da1420880a874a8673759387a4

Is this related to bug 1527831 / bug 1526382?

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1528133-calendar-xpcshell-qi-1.diff

These are all the QIs needed to make the XPCShell tests pass. (And yes, I know making the tests pass doesn't fix the problem.) I've commented testOfflineStorage in test_providers.js because it is all kinds of broken.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/333bacc41597
Add necessary QueryInterface calls to calendar; rs=bustage-fix

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1528133-calendar-qi-2.diff

This is the net result of the patches I've landed for this bug.

I've just been reminded that part of test_providers.js is still neutered, so I'm going to reopen the bug. I don't know how to fix it.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9044533 [details] [diff] [review]
1528133-calendar-qi-2.diff

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

I knew this would happen some time. I guess it is time to speed up getting rid of xpcom, good thing Paul is on board now.

::: calendar/base/modules/calRecurrenceUtils.jsm
@@ +446,4 @@
>                      let exceptionIds = recInfo.getExceptionIds({});
>                      for (let exceptionId of exceptionIds) {
>                          let recur = recInfo.getExceptionFor(exceptionId);
> +                        recur.QueryInterface(Ci.calIEvent);

I've had cases where just QI'ing for side-effects didn't work well, see also cal.wrapInstance(). I think you should strap the QI onto the line before.

::: calendar/base/src/calFilter.js
@@ +780,4 @@
>                  if (this.isItemInFilters(next)) {
>                      return next;
>                  }
> +                next.QueryInterface(Ci.calIEvent);

Similar here. Also happens in other places in this patch I'm not going to flag individually.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

We might want to back this out and update https://searchfox.org/comm-central/source/calendar/base/modules/calUtils.jsm#225 to adapt for bug 1526382. It looks like changing getInterfaces to get interfaces() or an array would fix more things at once.

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: 1528133-calendar-classinfo-1.diff

I did a Try run with this change and the others backed out and it worked okay. Does it not make sense to leave the QIs I added in place? XPCOM is a bit of a black box to me.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9045150 [details] [diff] [review]
1528133-calendar-classinfo-1.diff

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

I guess we could leave them in. Eventually classInfo might break completely, and then we've made headway.

For making it less of a black box: Without nsIClassInfo, Javascript XPCOM objects are wrapped so that they provide the methods and properties of one specific interface (in the worst case nsISupports). If nsIClassInfo is there and the interfaces are known, it will make all of the methods and properties of all known interfaces available.

Ideally, we're getting rid of all the xpcom in JS and moving that to pure js classes, for which we'd not need the QI's. That's still a bit down the line though.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e0d601d1f154
Fix calendar's ClassInfo implementation to follow the changes from bug 1526382. r=philipp