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


(MailNews Core :: Address Book, defect)

Not set


(Not tracked)

Thunderbird 76.0


(Reporter: darktrojan, Assigned: darktrojan)




(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]

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
Several improvements in address book manager. r=mkmelin

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.