Closed Bug 1626167 Opened 4 months ago Closed 4 months ago

Reset mail.collect_addressbook pref when the address book it points to is deleted

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 76.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(1 file)

From bug 1625650 comment 2:

Side note: for this crash to happen the user would have to have an invalid value for mail.collect_addressbook, probably an address book that's been deleted. Should we reset that pref if the directory it points to is deleted?

This patch suffered from scope creep. I fixed the original problem, then I realised that the two built-in directories could be deleted which should never happen. When writing a test for these I remembered that it's possible to throw more user-friendly exceptions than just Cr.NS_ERROR_FAILURE, so I changed some of those.

Attachment #9137672 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9137672 [details] [diff] [review]
1626167-addrbook-manager-improvements-1.diff

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

That is indeed nicer exceptions. r=mkmelin

::: mailnews/addrbook/jsaddrbook/AddrBookManager.jsm
@@ +178,5 @@
>    },
>    getDirectory(uri) {
>      if (uri.startsWith("moz-abdirectory://")) {
> +      throw new Components.Exception(
> +        "The root address book no longer exists",

It think it's nice to also add the uri to the message like

`The address book does not exist; uri=${uri}`

@@ +226,5 @@
>      }
>  
>      if (!dirName) {
> +      throw new Components.Exception(
> +        "Name must be specified",

maybe dirName instead of Name?

@@ +233,1 @@
>      }

I would move this whole if clause up.
Attachment #9137672 - Flags: review?(mkmelin+mozilla) → review+

(In reply to Magnus Melin [:mkmelin] from comment #2)

It think it's nice to also add the uri to the message like

The address book does not exist; uri=${uri}

I don't think that's helpful here, uri would always be either "moz-abdirectory://" or the same with a search query on the end. That is, if we ever hit this code, and I hope we don't.

I would move this whole if clause up.

It's already at the top of the function it's in, logically at least. There's only a function definition above it.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/f30adb6e9f15
Several improvements in address book manager. r=mkmelin

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