Closed Bug 1687329 Opened 8 months ago Closed 2 months ago

cal.wrapInstance() throws and swallows errors silently (may hide errors and impact performance)

Categories

(Calendar :: Internal Components, defect)

defect

Tracking

(Not tracked)

RESOLVED FIXED
92 Branch

People

(Reporter: lasana, Assigned: lasana)

References

Details

Attachments

(4 files, 2 obsolete files)

This function was introduced in bug 788004 to replace calInstanceOf:

https://searchfox.org/comm-central/source/calendar/base/modules/calUtils.jsm#320

As far as I can understand, its purpose seems to be to force an object to be used as an idl interface or return null if not. When I log the caught error to console, I see it happens quite frequently.

I have not done any profiling but this may have a negative impact on UX and could potentially be hiding errors that should be addressed.

Places where wrapInstance() is used, some of these may be able to be refactored not to:

https://searchfox.org/comm-esr60/search?q=wrapInstance&path=&case=true&regexp=false

Seems like the caught errors I saw originate from here:
https://searchfox.org/comm-central/source/calendar/base/modules/calUtils.jsm#583

Seeing as wrapInstance() is used to check if a calendar supports a particular interface, maybe we can return null instead of throwing an error?

Flags: needinfo?(philipp)
Flags: needinfo?(geoff)
Flags: needinfo?(philipp)
Flags: needinfo?(geoff)

I'm doing this in parts as some of these may not be easily changed. This one substitutes the use of wrapInstance to nsIURI.schemeIs instead.

Attachment #9197820 - Flags: review?(geoff)
See Also: → 1174397

Comment on attachment 9197820 [details] [diff] [review]
[checked in] bug1687329p1.patch

There's no need to create channel, just use the URI. I'm not sure what problem this little piece of ancient history was trying to solve. Perhaps they were unable to check the URI and see it was HTTP? Weird.

Attachment #9197820 - Flags: review?(geoff) → feedback+

I've had fights with calendar XPCOM stuff before and not always succeeded.

I think in many cases wrapInstance is being used to say "if this is an instance of interface then we want it, otherwise not" in which case instanceof should work.

Status: NEW → ASSIGNED

(In reply to Geoff Lankow (:darktrojan) from comment #4)

Comment on attachment 9197820 [details] [diff] [review]
bug1687329p1.patch

There's no need to create channel, just use the URI. I'm not sure what problem this little piece of ancient history was trying to solve. Perhaps they were unable to check the URI and see it was HTTP? Weird.

This code came about in bug 324849. I don't want to redo logic too much here, just clean up cal.wrapInstance() usage. Calendar is broken in various places already as it is. What I assumed was, creating a channel probably resolved or normalized the uri. I see comments about recognizing ETags in bug 324849. Maybe that's why? I'm not too familiar.

(In reply to Geoff Lankow (:darktrojan) from comment #5)

I've had fights with calendar XPCOM stuff before and not always succeeded.

I think in many cases wrapInstance is being used to say "if this is an instance of interface then we want it, otherwise not" in which case instanceof should work.

Yes this is what I'm seeing, however wrapInstance seemed to come about as a replacement for calInstanceOf which seemed to have come about due to problems using instanceof with calendar code.

Attached patch bug1687329p2.patch (obsolete) — Splinter Review

This one is a little long, it removes the calendar.item.isEvent and calendar.item.isTodo helpers. Instead I made those functions methods on calIItemBase. Note: I keep them as functions to avoid false negatives when the property isn't set.

Attachment #9202171 - Flags: review?(geoff)

Comment on attachment 9197820 [details] [diff] [review]
[checked in] bug1687329p1.patch

Requesting another review based on comment 6.

Attachment #9197820 - Flags: review?(geoff)

The patch in comment 8 causes some test failures. I'm looking into it.

Comment on attachment 9197820 [details] [diff] [review]
[checked in] bug1687329p1.patch

What I assumed was, creating a channel probably resolved or normalized the uri.

I doubt it but whatever. This'll do.

Attachment #9197820 - Flags: review?(geoff) → review+
Target Milestone: --- → 87 Branch

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/6c2303877566
Part 1 - Use schemeIs instead of wrapInstance in CalICSCalender.uri setter. r=darktrojan

Attached patch bug1687329p2v2.patch (obsolete) — Splinter Review

Dealt with some test failures this was causing.

Attachment #9202171 - Attachment is obsolete: true
Attachment #9202171 - Flags: review?(geoff)
Attachment #9203801 - Flags: review?(geoff)
Attachment #9203801 - Attachment is patch: true
Comment on attachment 9203801 [details] [diff] [review]
bug1687329p2v2.patch

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

For the benefit of my sanity I've looked at the main change and skimmed over the bulk of the rest. I'm assuming you used some sort of find/replace mechanism.

::: calendar/base/src/calItemBase.js
@@ +251,5 @@
>    },
>  
> +  isEvent() {
> +    return false;
> +  },

I suggest a comment saying this is overridden by CalEvent (likewise below). I mean, it's obvious, we all have days where we miss the obvious.
Attachment #9203801 - Flags: review?(geoff) → review+

For the benefit of my sanity I've looked at the main change and skimmed over the bulk of the rest. I'm assuming you used some sort of find/replace mechanism.

Where simple enough, yes.

Included the comment requested.

Attachment #9203801 - Attachment is obsolete: true
Attachment #9204246 - Flags: review?(geoff)

Comment on attachment 9204246 [details] [diff] [review]
[checked in] bug1687329p2v3.patch

This already had r+ so you can just carry that review forward.
I'll grab this for landing now.

Attachment #9204246 - Flags: review?(geoff) → review+
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/d5176548418c
Part 2: Add isEvent() and isTodo() to calIItemBase. r=darktrojan

Removing these because they seem dubious: The return value of wrapInstance() is checked to see if it's falsy before the logic continues. The return value however, will be null if the original value was falsy or when conversion to the desired interface failed.

For the latter, I think it's better we find out why the wrong xpcom object is being passed around instead of burying the error.

Attachment #9208173 - Flags: review?(geoff)

Comment on attachment 9208173 [details] [diff] [review]
[checked in] bug1687329p3.patch

Yeah, I don't see any need for these.

Attachment #9208173 - Flags: review?(geoff) → review+
Attachment #9197820 - Attachment filename: bug1687329p1.patch → [checked in] bug1687329p1.patch
Attachment #9197820 - Attachment description: bug1687329p1.patch → [checked in] bug1687329p1.patch
Attachment #9197820 - Attachment filename: [checked in] bug1687329p1.patch → bug1687329p1.patch
Attachment #9204246 - Attachment description: bug1687329p2v3.patch → [checked in] bug1687329p2v3.patch

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/404d98505938
Do not cal.wrapInstance() pending operations. r=darktrojan

Attachment #9208173 - Attachment description: bug1687329p3.patch → [checked in] bug1687329p3.patch

I'm going to tackle the rest of these in separate bugs.

Depends on: 1702066

(In reply to Lasana Murray from comment #23)

I'm going to tackle the rest of these in separate bugs.

Does it mean, this bug does not have to be left open and can be resolved as fixed?

Flags: needinfo?(lasana)

(In reply to Martin Schröder [:mschroeder] from comment #24)

(In reply to Lasana Murray from comment #23)

I'm going to tackle the rest of these in separate bugs.

Does it mean, this bug does not have to be left open and can be resolved as fixed?

Not yet, I may do the rest here if they are small enough.

Flags: needinfo?(lasana)

Last patch to remove some of these, the ones that remain seem either semi-legit or would
require more work than is worth to refactor.

Target Milestone: 87 Branch → 92 Branch

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/3438dd1edb4d
Remove remaining cal.wrapInstance() usages where possible. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.