Closed Bug 1576365 Opened 5 years ago Closed 5 years ago

NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIAbManager.getDirectory]

Categories

(MailNews Core :: LDAP Integration, defect, P1)

Product:
MailNews Core
Component:
LDAP Integration
Type:
defect

Tracking

(thunderbird_esr6869+ fixed, thunderbird70 fixed, thunderbird71 fixed)

Status:
VERIFIED FIXED
Milestone:
Thunderbird 71.0
Tracking Flags:
Tracking Status
thunderbird_esr68 69+ fixed
thunderbird70 --- fixed
thunderbird71 --- fixed

People

(Reporter: jorgk-bmo, Assigned: aleca)

References

Details

(Keywords: regression)

Attachments

(1 file, 9 obsolete files)

1576365-richlistbox-bug.patch
3.03 KB, patch
jorgk-bmo
: review+
jorgk-bmo
: approval-comm-beta+
jorgk-bmo
: approval-comm-esr68+
Details | Diff | Splinter Review

NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIAbManager.getDirectory]

NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIAbManager.getDirectory] 6 pref-editdirectories.js:119
editDirectory chrome://messenger/content/addressbook/pref-editdirectories.js:119
oncommand chrome://messenger/content/addressbook/pref-editdirectories.xul:1

Found when repeatedly trying to edit the Adams server I set up according to bug 1574712 comment #0.

This is the line that fails let ab = MailServices.ab.getDirectory(abURI); in function editDirectory().

Flags: needinfo?(alessandro)

In fact, you can edit once, not a second time when you have the list of servers up. Works when you dismiss the list first and start again.

Oh, I see what wrong, Edit, OK on the edit panel, loses the selection in the list. If you click Edit again, you get the error.

I observed this behaviour as well. I think in TB60 the AB UI made sure that there is always a directoy selected and this no longer works with TB68. I will confirm this later this weekend.

Having no directory selected causes issues for everyone who uses GetSelectedDirectory().

This LDAP integration seems to not like me at all.
I'll get on it right away and fix this.
Sorry again for the issue.

Flags: needinfo?(alessandro)
Assignee: nobody → alessandro
Priority: -- → P1

A little bit of an update.
I tested this on trunk and the issue doesn't seem to present.
The first directory is always selected, no matter how many times I edit an entry and re-edit it again, the selection for the richlistbox persists.

Also, I don't understand why the EDIT and DELETE buttons don't get disabled if no listitem is selected, since this method should kick in:
https://searchfox.org/comm-central/rev/753e6d9df9d7b9a989d735b01c8b280eef388bab/mailnews/addrbook/prefs/content/pref-editdirectories.js#84

I'll keep investigating.

Alice, by now, you're an expert on LDAP.

Can you please configure the Adam server and then hit "Edit" on the list of servers multiple times. Click "OK" on the edit panel.

On TB 68 it only works the first time. On trunk, the first and all subsequent "Edit" evocations work.

Can you find what fixed the issue please. We might need to uplift/backport that to TB 68. Many thanks in advance.

Flags: needinfo?(alice0775)

Thank you so much Alice, this was super helpful.

The changes in comm-central seem completely unrelated, so I suspect it might be this that fixed the issue on trunk: https://hg.mozilla.org/mozilla-central/rev/e09471d136599b68f40f6bb9ec7fcc419732948e

Thanks again, Alice.

I'll see what happens if we move that change from 10 days into the 69 cycle to 68. Hmm, sort of risky.

Flags: needinfo?(jorgk)

On TB60ESR,
After hit "OK" in "Adams Property" dialog, the "Edit"(and "Delete") button is disabled. Because the list is deselected.

On TB68 and TB70,
After hit "OK" in "Adams Property" dialog, the "Edit"(and "Delete") button is still enabled. --- this is BUG I think.

Attached patch 1576365-ldap-regression.patch (obsolete) — — Splinter Review

Here's a hacky patch as a temporary workaround for the richlistitem losing focus on 68.
It's ugly and I'm not sure we want this.

Attachment #9088319 - Flags: review?(jorgk)

I think if it works we might as well take it and be done with it. Uplifting richlistitem de-xbl isn't an option.

Uplifting richlistitem de-xbl isn't an option.

I was going to try this since it landed ten days into the 69 cycle and all the bugs it depended on are already in 68 (or earlier). Is is a bad idea?

I'm wondering whether other we have other spots where such malfunction occurs ... and looking for one, I found that the incoming attachment list is totally hosed :-(

What about comment #10? Shouldn't it behave like TB 60?

Flags: needinfo?(jorgk) → needinfo?(mkmelin+mozilla)

Can we decide on comment #10 before I start messing around with backporting richlistitem de-xbl to TB 68? And preferably also understand why this might be a bad idea.

For comment 10, interesting that it loses the selection after edit by button, but not when edit through double click. I'd suspect a bug in https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/richlistbox.js and that's probably the best place to start debugging.

Flags: needinfo?(mkmelin+mozilla)

Magnus, you didn't answer any of my questions:

  1. Shall we restore the state of TB 60, that is: After Edit button: Deselect the list, disable the buttons?
  2. Why isn't backporting richlistitem de-XBL an option. See comment your comment #12.

And please don't paste any https://searchfox.org/comm-central/source/mozilla/ links as they don't fully work :-(

Flags: needinfo?(mkmelin+mozilla)

I found this: Options, Composition, Keywords. That comes up with a list and "Add" / "Edit". There you can hit "Edit" multiple times in TB 68. But not so in TB 60. There is also loses the selection and the button is disabled.

There is also the spelling button. That launches a panel where you can edit the personal dictionary. It displays a list of words at the bottom. If you exercise a button, like "Remove", you don't lose selection in the list.

So I think the way to go is to maintain some visible selection and make the buttons work.

Thanks for reporting these, please keep adding messages if you stumble upon other sections with the same behaviour.
I'll work on a patch later this afternoon to improve what I uploaded.
The goal is to force the focus back tot he list and refocus the originally selected listitem.

Instead of doing a complicated try build based on TB 68 with changes to mozilla-esr68 code, I simply unpacked omni.ja and made the changes for the richlistitem from https://hg.mozilla.org/mozilla-central/rev/e09471d136599b68f40f6bb9ec7fcc419732948e locally. Yes, that fixes the issue.

So I'd really like to hear from Magnus whether this is a possible way forward.

(In reply to Jorg K (GMT+2) from comment #16)

  1. Shall we restore the state of TB 60, that is: After Edit button: Deselect the list, disable the buttons?

I think the (trunk) behaviour you get when double-clicking to edit is good: the selection remains and therefore the buttons are still enabled.
I notice now, at least on trunk maybe it's a just a visual bug. Editing multiple times works, it's just that the visual selection indication in the list is not shown. So that would be the bug to fix.

  1. Why isn't backporting richlistitem de-XBL an option. See comment your comment #12.

Hmm, going back in time to check what we had that extended richlistitem at that time is somewhat complicated. It seems at least filterlistitem, ruleaction (so most of the filter and search dialog widgets), and calendar-subscriptions-richlistitem. I may have missed some since the dependencies are not obvious + we had the forked richlistitem at some point.

So all in all, richlistitem is used all over the place and looks like there were c-c places not converted over at the time that patch landed. Difficult to say what it would break.

And please don't paste any https://searchfox.org/comm-central/source/mozilla/ links as they don't fully work :-(

Non-versioned links work just fine.

Flags: needinfo?(mkmelin+mozilla)

Oh, I read that slightly wrong. Actually I realize the patch just removed hooking up <richlistitem> to xbl, and hooking it up to a custom element instead. So backporting could cause regressions to every place we use richlistitem.

Hmm, I looked at filter and search dialogue widgets and event attendees. I can't try Calendar subscription since I don't have a CalDAV provider. What I saw, looked OK. But if you think it's too risky, I won't do it and we use a hack like in attachment 9088319 [details] [diff] [review] instead.

Comment on attachment 9088319 [details] [diff] [review]
1576365-ldap-regression.patch

Given the discussion about backporting the de-XBL fix, I think we'll take this. However, after adding a server Edit and Remove buttons are enabled but clicking them doesn't do anything.

Also, could you check the CSS to make sure that the selected row is somewhat visible, like for the keywords I mentioned in comment #17.
Attachment #9088319 - Flags: review?(jorgk)

Sounds good, thanks for defining a clear direction.
I will take care of this most likely tomorrow, once the new website is out of the way.

Attached patch 1576365-richlistbox-bug.patch (obsolete) — — Splinter Review

With this patch the focus goes back to the list and avoids the error.
This is not an optimal solution tho, because it resets the selected listitem to the first one on the list, instead of the one edited by the user.

I'm trying to find a workaround to keep the selection on the original item, meanwhile, let me know if this solution works for you.

Attachment #9088319 - Attachment is obsolete: true
Attachment #9089896 - Flags: review?(jorgk)
Attached patch 1576365-richlistbox-bug.patch (obsolete) — — Splinter Review

I did it!
Now the list gets the focus back and the selection persists.

Attachment #9089896 - Attachment is obsolete: true
Attachment #9089896 - Flags: review?(jorgk)
Attachment #9089907 - Flags: review?(mkmelin+mozilla)
Attachment #9089907 - Flags: review?(jorgk)

Can someone delete this comment?

Status: NEW → ASSIGNED
Comment on attachment 9089907 [details] [diff] [review]
1576365-richlistbox-bug.patch

I'll take it as a 68 bad aid since we decided not to backport bug 1512432. I'l fix the comments and remove the boy-scout stuff adding braces since we're on 68 here which will be reformatted anyway.
Attachment #9089907 - Flags: review?(mkmelin+mozilla)
Attachment #9089907 - Flags: review?(jorgk)
Attachment #9089907 - Flags: review+
Target Milestone: --- → Thunderbird 68.0
Version: unspecified → 68
Attachment #9089907 - Flags: approval-comm-esr68+
Comment on attachment 9089907 [details] [diff] [review]
1576365-richlistbox-bug.patch

Changed my mind. That will not work of last item is removed, or BB was selected as first in the list and AA gets added before it.
Attachment #9089907 - Flags: review+
Attachment #9089907 - Flags: approval-comm-esr68+
Attached patch 1576365-richlistbox-bug.patch (obsolete) — — Splinter Review

I stripped it down a bit to remove the unnecessary parts and realised that it won't work in all cases.

So if we remove the last time, we need to select the second last. And if we add, we need to select the added item, or the one previously selected.

Thanks for reviewing this.
Based on our discussion on IRC, this is the workflow we think might work:

  • When a new item is added: select the added one
  • When an item is removed: select the first item on the list
  • When an item is edited: keep the selection on the edited item

I'll upload an updated patch shortly.

Attached patch 1576365-richlistbox-bug.patch (obsolete) — — Splinter Review

This should cover all those cases listed above.

Attachment #9089907 - Attachment is obsolete: true
Attachment #9089916 - Attachment is obsolete: true
Attachment #9089930 - Flags: review?(jorgk)
Comment on attachment 9089930 [details] [diff] [review]
1576365-richlistbox-bug.patch

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

::: mailnews/addrbook/prefs/content/pref-editdirectories.js
@@ +83,5 @@
>  
>      abList.appendChild(item);
>    });
> +
> +  // Forces the focus back on the list and on the first item.

Umm, is this comment right? It doesn't select the first item.

@@ +88,5 @@
> +  // (Special hack for TB 68, see bug 1576365)
> +  abList.focus();
> +
> +  // Forces the focus back on the edited or recently added item.
> +  // (Special hack for TB 68, see bug 1576365)

Please combine with the comment above.

@@ +94,5 @@
> +    if (edited) {
> +      abList.selectedIndex = selected;
> +    } else {
> +      abList.selectedIndex = holdingArray.findIndex((d) => {
> +        return d && d.URI == aItem.URI;

What is assigned here if the item was removed? Or is aItem==null then and we don't get here?

(In reply to Jorg K (GMT+2) from comment #34)

// Forces the focus back on the list and on the first item.

Umm, is this comment right? It doesn't select the first item.

Re setting the focus on the listbox automatically selects the first item.

What is assigned here if the item was removed? Or is aItem==null then and we
don't get here?

If the item is deleted this condition doesn't run, which means the default behavior of the richlistbox of selecting the first item of the list when it gets the focus is valid.
No need to create a condition for that case.

OK, thanks for the clarification, I'll try it later.

We still need to fix trunk somehow. Double-edit works there, but there is nothing visibly selected in the list after the first edit. Edit after New works, but it doesn't edit the new server, it edits the previously selected one. And Delete doesn't work at all, you get:
NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAbManager.deleteAddressBook] 2 abCommon.js:372
AbDeleteDirectory chrome://messenger/content/addressbook/abCommon.js:372
removeDirectory chrome://messenger/content/addressbook/pref-editdirectories.js:148
oncommand chrome://messenger/content/addressbook/pref-editdirectories.xul:1

This works perfectly, I tried it on an "omni.ja unpacked" TB 68.1 to avoid a try build. I tweaked the comment, so this is ready to go.

Please address the issues in trunk in a separate patch. Maybe we just do the same thing without the focus calls. Also, deletion appears to be broken. Maybe that broke later.

Attachment #9089930 - Attachment is obsolete: true
Attachment #9089930 - Flags: review?(jorgk)
Attachment #9089973 - Flags: review+
Attachment #9089973 - Flags: approval-comm-esr68+

Good to know this solution works for 68.
I'm taking care of trunk now.
Do you want me to upload a different patch in this bug?

Applying the same patch to trunk fixes all the issues, except the deletion.
That definitely broke after because i remember testing trunk when this bug was opened and everything worked at the time.
Something broke the MailServices.ab.deleteAddressBook(aURI);

Yes, let's fix it in this bug. I think we don't need the same patch, I guess you can lose the focus calls since in TB 70 they are not needed.

Let's get Alice to find the regression for the Delete.

Alice, can you please find the regression for the deletion of an LDAP address book.

So in options, composition, addressing you bring up the list of LDAP servers and click delete. That worked until very recently. Today you get the error listed in comment #36.

Flags: needinfo?(alice0775)

Here's the patch that fixes the items selection on trunk.

Attachment #9090144 - Flags: review?(jorgk)
Comment on attachment 9090144 [details] [diff] [review]
1576365-richlistbox-bug-TRUNK.patch

Hard to tell whether it works for deletion. It's different in TB 68. There the selection is clearly visible in blue, here it's light grey. So the focus() call matters?

In 68, the focus() method is necessary otherwise it would be impossible to select added or edited items back.
On trunk, the focus stays on the last clicked button, but is not necessary to use the focus() method because the richlistbox retains the selection.

As you noticed, the only difference here is visual, where on trunk the focus remains on the button and the list item is selected by only a grey background, instead of a blue highlight.

That's why I suggested to use the same patch, because forcing the focus back on the list makes more sense also from a keyboard accessibility point of view.

That's why I suggested to use the same patch, ...

OK, I was wrong, not the first time. Let's use the same. With the "hack" comment?

I'm not sure about the "hack" part in the comment. Does it feel like a hack now that it's everywhere and not only on 68?
Up to you if you want to remove it.

Just leave it. I think it's a hack managing focus and selection in this case.

Let's shut up now so Alice finds his question in comment #40 on delete LDAP server.

(In reply to Jorg K (GMT+2) from comment #40)

Alice, can you please find the regression for the deletion of an LDAP address book.

So in options, composition, addressing you bring up the list of LDAP servers and click delete. That worked until very recently. Today you get the error listed in comment #36.

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=8bbeb44f15b366817cab0573ce3807d4fc09fc33&tochange=fa465d49d7bf9f021772b7080340e9856edf71c5
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f6a1009171e521c4917dcaccf69aba9093467e99&tochange=98cbe150f012619bf9e8d44413fa33374e92dbc9

Flags: needinfo?(alice0775)

Thanks, Alice. I'll file a new bug. EDIT: Filed bug 1578523. Let's get the ESR 68 patch landed on trunk to get rid of this bug finally.

Comment on attachment 9090144 [details] [diff] [review]
1576365-richlistbox-bug-TRUNK.patch

I'll take the ESR 68 patch.
Attachment #9090144 - Flags: review?(jorgk)
Attachment #9090144 - Attachment is obsolete: true
Attachment #9090162 - Flags: review+
Attachment #9090162 - Flags: approval-comm-beta+
Attachment #9090162 - Attachment is obsolete: true
Attachment #9090163 - Flags: review+
Attachment #9090163 - Flags: approval-comm-beta+
Target Milestone: Thunderbird 68.0 → Thunderbird 71.0
Comment on attachment 9089973 [details] [diff] [review]
1576365-richlistbox-bug.patch ESR68 patch

Not working if you edit the name and the thing moves into a different position in the list.
Attachment #9089973 - Flags: review+
Attachment #9089973 - Flags: approval-comm-esr68+
Comment on attachment 9090163 [details] [diff] [review]
1576365-richlistbox-bug.patch - TRUNK

Oh, joy!
Flags: needinfo?(alessandro)
Attachment #9090163 - Flags: review+
Attachment #9090163 - Flags: approval-comm-beta+

Argh, all right.
The index changes if the name causes the item to change position.
Another patch is coming soon.

Flags: needinfo?(alessandro)

This should be good and working for both 68 and trunk.

Attachment #9089973 - Attachment is obsolete: true
Attachment #9090163 - Attachment is obsolete: true
Attachment #9090185 - Flags: review?(jorgk)
Comment on attachment 9090185 [details] [diff] [review]
1576365-richlistbox-bug.patch

Yes, that's what I thought. I changed my mind about the "hack" comment, I'll take it out. I'll do the 68 ESR version myself.
Attachment #9090185 - Flags: review?(jorgk)
Attachment #9090185 - Flags: review+
Attachment #9090185 - Flags: approval-comm-esr68+
Attachment #9090185 - Flags: approval-comm-beta+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/34e95735ae2b
Fix richlistbox losing focus when editing listitem. r=jorgk

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/dc8ba56d0d54
Prettier fix. r=me DONTBUILD

Working in TB 68.1.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: