1650925 - "impl is null" errors from returning null in nsIInterfaceRequestor::getInterface()

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Paul Morris [:pmorris]]

We should be throwing instead of returning null when implementing nsIInterfaceRequestor::getInterface (e.g. in InterfaceRequestor_getInterface in calProviderUtils.jsm), and the devtools code is complaining with impl is null errors in the console. I'll upload the fix from Philipp shortly.

Comment 1

[Paul Morris [:pmorris]]

Attachment: throw-when-getting-interfaces-0.diff

These are basically the changes that Philipp shared over chat. I've added a similar change in CalDavSession.jsm. Still needs some more testing, and a try run, but going ahead and putting it up for review.

Comment 2

[Paul Morris [:pmorris]]

try run in progress: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=02bbee4d9f52f900094c4a857eb8a038c1c7e2a7

Comment 3

[Paul Morris [:pmorris]]

Attachment: throw-when-getting-interfaces-1.diff

I realized I uploaded a botched version of the patch. Here's the correct one. The try run is with the correct patch, and looks okay.

Comment 4

[Geoff Lankow (:darktrojan) [OOO, back 7 Apr]]

Comment on attachment 9161913 [details] [diff] [review]
throw-when-getting-interfaces-1.diff

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

::: calendar/base/modules/utils/calProviderUtils.jsm
@@ +52,5 @@
> +      // For example, when there is no calendar associated with a request,
> +      // as in calendar detection.
> +      calendar = aNotificationCallbacks.getInterface(Ci.calICalendar);
> +    } catch (e) {
> +      if (!e.result || e.result != Cr.NS_ERROR_NO_INTERFACE) {

Do we need to check that e.result exists? If it's undefined, that's not equal to Cr.NS_ERROR_NO_INTERFACE.

::: calendar/providers/caldav/modules/CalDavRequest.jsm
@@ +173,5 @@
> +    let iface = tryGetInterface(this.session) || tryGetInterface(this.calendar);
> +    if (!iface) {
> +      throw Components.Exception("", Cr.NS_ERROR_NO_INTERFACE);
> +    }
> +    return iface;

Flip this.

Comment 5

[Paul Morris [:pmorris]]

Attachment: throw-when-getting-interfaces-3.diff

Thanks for review. Comments addressed.

Comment 6

[Paul Morris [:pmorris]]

I'm debugging some networking errors in a patch on top of this one, and I want to get it sorted out and make sure it's not related to this patch before we land this.

Comment 7

[Paul Morris [:pmorris]]

The errors are indeed due to this patch. Synchronizing a caldav calendar fails:

console.log: Lightning: [calCachedCalendar] Doing changelog based sync for calendar https://caldav.fastmail.com/dav/calendars/user/[...]
console.debug: Lightning: 
  CalDAV: send (PROPFIND https://caldav.fastmail.com/dav/calendars/user/[...]/): <?xml version="1.0" encoding="UTF-8"?>
<D:propfind xmlns:D='DAV:' xmlns:C='urn:ietf:params:xml:ns:caldav' xmlns:CS='http://calendarserver.org/ns/'><D:prop><D:resourcetype/><D:owner/><D:current-user-principal/><D:supported-report-set/><C:supported-calendar-component-set/><CS:getctag/></D:prop></D:propfind>
JavaScript error: resource://devtools/server/actors/network-monitor/network-response-listener.js, line 89: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIInterfaceRequestor.getInterface]
JavaScript error: resource://devtools/server/actors/network-monitor/utils/channel-map.js, line 57: TypeError: this.weakMap.get(...) is undefined
JavaScript error: resource://devtools/server/actors/network-monitor/utils/channel-map.js, line 57: TypeError: this.weakMap.get(...) is undefined
console.log: Lightning: CalDAV: Error during initial PROPFIND for calendar FM test cal: [object Object]
console.warn: Lightning: There has been an error reading data for calendar: FM test cal.  However, this error is believed to be minor, so the program will attempt to continue. Error code: DAV_NOT_DAV. Description: The resource at https://caldav.fastmail.com/dav/calendars/user/[...] is either not a DAV collection or not available
console.warn: Lightning: There has been an error reading data for calendar: FM test cal.  However, this error is believed to be minor, so the program will attempt to continue. Error code: READ_FAILED. Description: 
console.error: Lightning: 
  [calCachedCalendar] replay action failed: <unknown>, uri=https://caldav.fastmail.com/dav/calendars/user/[...], result=2147500037, operation=[xpconnect wrapped calIOperation]

Comment 8

[Paul Morris [:pmorris]]

Attachment: throw-when-getting-interfaces-4.diff

Fixed. I had added similar changes to CalDavSession.getInterface and that was causing the new errors. This patch just drops those changes. The remaining changes have been reviewed, and manually tested, so this is ready to land.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4290f6c66a27
Fix console errors due to returning null from getInterface in calendar network code. r=darktrojan

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9162187 [details] [diff] [review]
throw-when-getting-interfaces-4.diff

[Approval Request Comment]
Annoying console error

Comment 11

[Wayne Mery (:wsmwk)]

Comment on attachment 9162187 [details] [diff] [review]
throw-when-getting-interfaces-4.diff

[Triage Comment]
Approved for esr78

Comment 12

[Rob Lemley [:rjl]]

Thunderbird 78.3.2:
https://hg.mozilla.org/releases/comm-esr78/rev/8b38496e9044