Closed Bug 1571453 Opened 3 months ago Closed 3 months ago

Startup cache isn't cleared on upgrade of a legacy add-on

Categories

(Thunderbird :: Add-Ons: General, defect)

defect
Not set

Tracking

(thunderbird_esr6868+ fixed, thunderbird69 fixed, thunderbird70 fixed)

RESOLVED FIXED
Thunderbird 70.0
Tracking Status
thunderbird_esr68 68+ fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

(Reporter: standard8, Assigned: darktrojan)

Details

Attachments

(1 file, 2 obsolete files)

I've been debugging a problem in one of my profiles where Thunderbird Conversations wouldn't work correctly, despite various fixes on upgrade.

Today, I eventually realised the issue. Despite the add-on having just upgraded (and restarted Thunderbird, checked the reported version etc), the debug output on the error console was still from an older version of the add-on (probably early 2.15.1, or even 2.14.x).

With that in mind, I poked around the Thunderbird cache, and moved startupCache/startupCache.8.little out of the way so it couldn't be found.

Starting Thunderbird up again, the add-on sprung into life, and started working correctly, with the debug output was correct.

Therefore, I think that when using add-ons with the legacy flag, we need to make sure we're clearing the startup cache on upgrade.

Requesting tracking, as this seems a fundamental issue for add-ons.

One tracking flag will do.

Flags: needinfo?(geoff)

We do invalidate the cache. Unless the uninstall method isn't being called, which would be weird. I suppose it is possible that uninstall throws an error and we don't catch it. We can fix that.

Flags: needinfo?(geoff)
Attached patch 1571453-uninstall-cache-1.diff (obsolete) — Splinter Review

Adds a try/finally block around the uninstall method.

I also added some checks to the existing test to prove we're calling startupcache-invalidate, although not for this particular case as that would cause bigger problems in the tests.

Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #9083170 - Flags: review?(mkmelin+mozilla)

Thanks for this, I hadn't thought about my uninstall throwing causing issues (which it was, but I think I've fixed that in the latest version). Still, having a try/catch is definitely the right thing to do in case of error.

Comment on attachment 9083170 [details] [diff] [review]
1571453-uninstall-cache-1.diff

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

LGTM, r=mkmelin
Attachment #9083170 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
Attachment #9083170 - Flags: approval-comm-esr68?
Attachment #9083170 - Flags: approval-comm-beta?
Comment on attachment 9083170 [details] [diff] [review]
1571453-uninstall-cache-1.diff

How risky is this? Should that ship in TB 68.0 ESR without spending time on beta?
Attachment #9083170 - Flags: approval-comm-beta? → approval-comm-beta+

It's a try/finally block.

Comment on attachment 9083170 [details] [diff] [review]
1571453-uninstall-cache-1.diff

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

::: mail/components/extensions/parent/ext-legacy.js
@@ +309,5 @@
>        install: (...args) => install(...args),
>  
>        uninstall(...args) {
> +        try {
> +          return uninstall(...args);

Could we still log the error in the throw case? Otherwise it'll be silently absorbed and less likely to be noticed.

That is functionally no different from how it was before. The error is still logged.

Comment on attachment 9083170 [details] [diff] [review]
1571453-uninstall-cache-1.diff

OK then, looks like the benefit outweighs the risk, if any.
Attachment #9083170 - Flags: approval-comm-esr68? → approval-comm-esr68+

Hmm, we should really be catching exceptions, logging, and carrying on… not letting them continue up the stack.

Keywords: checkin-needed
Attached patch 1571453-uninstall-cache-1.diff (obsolete) — Splinter Review

Take two.

Attachment #9083170 - Attachment is obsolete: true
Attachment #9083242 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9083242 [details] [diff] [review]
1571453-uninstall-cache-1.diff

Gah, wrong patch!
Attachment #9083242 - Attachment is obsolete: true
Attachment #9083242 - Flags: review?(mkmelin+mozilla)

Take three.

Attachment #9083243 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9083243 [details] [diff] [review]
1571453-exception-handling-1.diff

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

::: mail/components/extensions/parent/ext-legacy.js
@@ +311,5 @@
> +          install(...args);
> +        } catch (ex) {
> +          logger.warn(
> +            `Exception running bootstrap method install on ${extension.id}`,
> +            ex

huh?
DId you mean to rethrow it?

No. We should report the error and carry on.

So what is the stray "ex" about?

Ah, I see now. Part of the log statement...

Attachment #9083243 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed

So, if I understand this correctly, this is only an issue for "misbehaving" add-ons, like (comment #4: "my uninstall throwing causing issues"), so how great is the urge to take this straight to TB 68 ESR? Looks like the changes have become more extensive than just adding a try/catch.

I haven't landed it yet. You can do so yourself if there's a merge around 4-6h GMT, or I'll do it in the morning.

It's still just a try/catch (×4) and it should be pushed to ESR, but if it's 68.1 instead of 68.0 I'm really not that bothered.

Comment on attachment 9083243 [details] [diff] [review]
1571453-exception-handling-1.diff

OK.
Attachment #9083243 - Flags: approval-comm-esr68?
Attachment #9083243 - Flags: approval-comm-beta+
Target Milestone: --- → Thunderbird 70.0

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/501d4e09cffb
Handle exceptions in bootstrapped extension methods better. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Is this really a "blocker" bug?

Given we now know what caused it, and most extensions probably don't throw errors on uninstall, probably not.

Severity: blocker → normal
Attachment #9083170 - Flags: approval-comm-esr68+
Attachment #9083170 - Flags: approval-comm-beta+
Attachment #9083243 - Flags: approval-comm-esr68? → approval-comm-esr68+
You need to log in before you can comment on or make changes to this bug.