Don't try to create folders for unloaded extensions

RESOLVED FIXED in Thunderbird 66.0

Status

enhancement
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: neil, Assigned: neil)

Tracking

unspecified
Thunderbird 66.0

Thunderbird Tracking Flags

(thunderbird_esr6065+ fixed, thunderbird65 fixed, thunderbird66 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

5 months ago
When starting Thunderbird the folder pane tries to restore the last selected folder. However if this belongs to an extension which has become disabled or simply not yet started up then the RDF service is unable to create a folder object, instead returning a plain RDF resource. Unfortunately this resource then pollutes the RDF cache preventing the extension from loading correctly.
Assignee

Comment 1

5 months ago
If the folder scheme has a registration then this means that the extension has loaded and it's safe to attempt to get or create the folder.

At some point the folder lookup service will need to create the folder itself so it's going to need to do something along these lines anyway.

To avoid code duplication I called getOrCreateFolderForURL from getFolderForURL but I can change that if you prefer.
Attachment #9029953 - Flags: review?(acelists)
Assignee

Comment 2

5 months ago
Posted patch ESR 60Splinter Review
(On ESR60 the specific problem that shows up the most is the folder pane trying to restore folders that aren't available yet, thus the extra code to deal with that case.)

Comment 3

5 months ago
Comment on attachment 9029953 [details] [diff] [review]
Proposed patch

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

Thanks.
This may slow down getOrCreateFolderForURL a bit, but we aim to not call it from places where creating the folder is not needed.
Attachment #9029953 - Flags: review?(acelists) → review+

Updated

5 months ago
Status: NEW → ASSIGNED
Component: General → Backend
Assignee

Updated

5 months ago
Keywords: checkin-needed

Comment 4

5 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b8dbd330b2a1
Don't try to create folders for unloaded extensions. r=aceman
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

5 months ago
Target Milestone: --- → Thunderbird 66.0

Updated

5 months ago
Attachment #9029953 - Flags: approval-comm-beta+

Comment 5

5 months ago
Comment on attachment 9029964 [details] [diff] [review]
ESR 60

What's the risk here?
Attachment #9029964 - Flags: approval-comm-esr60?
Assignee

Comment 7

5 months ago
Sorry for the delay, I hadn't got around to testing the port yet because I needed to rebuild my branch tree.

(In reply to Jorg K from comment #5)
> What's the risk here?

This is just a correctness check to avoid creating a plain RDF resource (which is what we would get if we don't have an appropriately registered component) when we don't want one anyway, so the risk should be low. Additionally, lots of branch is still hitting RDF directly, so I'm only patching the minimum number of calls to minimise the issue, rather than trying to port all of kill-RDF. Additionally the code is in the startup path so if there was a problem it would have been very noticeable.

Comment 8

5 months ago
Let's take it for TB 60.4.n, n > 0, or TB 60.5.

Comment 9

5 months ago
Comment on attachment 9029964 [details] [diff] [review]
ESR 60

Good for TB 60.5.
Attachment #9029964 - Flags: approval-comm-esr60? → approval-comm-esr60+

Comment 10

4 months ago

Comment on attachment 9029964 [details] [diff] [review]
ESR 60

Sorry, that patch doesn't apply and worse, the function you're calling, getOrCreateFolderForURL(), doesn't exist in ESR 60.

Flags: needinfo?(neil)
Flags: needinfo?(ben.bucksch)
Attachment #9029964 - Flags: approval-comm-esr60+

Dang. We'll update the patch.

Flags: needinfo?(ben.bucksch)

Comment on attachment 9029964 [details] [diff] [review]
ESR 60

Hey Jörg,
getOrCreateFolderForURL() doesn't appear in the ESR60 patch, but does appear in the trunk patch. I think you simply applied the wrong patch.

To avoid any potential further problems, we'll make a try build as well.

Attachment #9029964 - Flags: approval-comm-esr60?

Comment 13

4 months ago
Flags: needinfo?(neil)
Attachment #9029964 - Flags: approval-comm-esr60? → approval-comm-esr60+
You need to log in before you can comment on or make changes to this bug.