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

Status: VERIFIED FIXED

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

Description

[Jorg K (CEST = GMT+2)]

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().

Comment 1

[Jorg K (CEST = GMT+2)]

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.

Comment 2

[Jorg K (CEST = GMT+2)]

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.

Comment 3

[John Bieling (:TbSync)]

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().

Comment 4

[Alessandro Castellani [:aleca]]

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.

Comment 5

[Alessandro Castellani [:aleca]]

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.

Comment 6

[Jorg K (CEST = GMT+2)]

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.

Comment 7

[Alice0775 White]

Fixed range:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=aff89fb3045ea88de39bb83d6006a2387c37cea7&tochange=a6bbb29b55e5b214340877dab10d84bc7c7ade48
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=73c98da145a7c0ef518404493b23a979f328768e&tochange=e09471d136599b68f40f6bb9ec7fcc419732948e

Comment 8

[Alessandro Castellani [:aleca]]

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

Comment 9

[Jorg K (CEST = GMT+2)]

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.

Comment 10

[Alice0775 White]

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.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1576365-ldap-regression.patch

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.

Comment 12

[Magnus Melin [:mkmelin]]

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

Comment 13

[Jorg K (CEST = GMT+2)]

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?

Comment 14

[Jorg K (CEST = GMT+2)]

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.

Comment 15

[Magnus Melin [:mkmelin]]

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.

Comment 16

[Jorg K (CEST = GMT+2)]

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

Comment 17

[Jorg K (CEST = GMT+2)]

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.

Comment 18

[Jorg K (CEST = GMT+2)]

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.

Comment 19

[Alessandro Castellani [:aleca]]

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.

Comment 20

[Jorg K (CEST = GMT+2)]

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.

Comment 21

[Magnus Melin [:mkmelin]]

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

2. 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.

Comment 22

[Magnus Melin [:mkmelin]]

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.

Comment 23

[Jorg K (CEST = GMT+2)]

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 24

[Jorg K (CEST = GMT+2)]

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.

Comment 25

[Alessandro Castellani [:aleca]]

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.

Comment 26

[Alessandro Castellani [:aleca]]

Attachment: 1576365-richlistbox-bug.patch

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.

Comment 27

[Alessandro Castellani [:aleca]]

Attachment: 1576365-richlistbox-bug.patch

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

Comment 28

[Alessandro Castellani [:aleca]]

Can someone delete this comment?

Comment 29

[Jorg K (CEST = GMT+2)]

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.

Comment 30

[Jorg K (CEST = GMT+2)]

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.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: 1576365-richlistbox-bug.patch

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.

Comment 32

[Alessandro Castellani [:aleca]]

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.

Comment 33

[Alessandro Castellani [:aleca]]

Attachment: 1576365-richlistbox-bug.patch

This should cover all those cases listed above.

Comment 34

[Jorg K (CEST = GMT+2)]

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?

Comment 35

[Alessandro Castellani [:aleca]]

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

Comment 36

[Jorg K (CEST = GMT+2)]

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

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: 1576365-richlistbox-bug.patch ESR68 patch

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.

Comment 38

[Alessandro Castellani [:aleca]]

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?

Comment 39

[Alessandro Castellani [:aleca]]

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);

Comment 40

[Jorg K (CEST = GMT+2)]

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.

Comment 41

[Alessandro Castellani [:aleca]]

Attachment: 1576365-richlistbox-bug-TRUNK.patch

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

Comment 42

[Jorg K (CEST = GMT+2)]

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?

Comment 43

[Alessandro Castellani [:aleca]]

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.

Comment 44

[Jorg K (CEST = GMT+2)]

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?

Comment 45

[Alessandro Castellani [:aleca]]

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.

Comment 46

[Jorg K (CEST = GMT+2)]

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.

Comment 47

[Alice0775 White]

(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

Comment 48

[Jorg K (CEST = GMT+2)]

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 49

[Jorg K (CEST = GMT+2)]

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

I'll take the ESR 68 patch.

Comment 50

[Jorg K (CEST = GMT+2)]

Attachment: 1576365-richlistbox-bug.patch - TRUNK

Comment 51

[Jorg K (CEST = GMT+2)]

Attachment: 1576365-richlistbox-bug.patch - TRUNK

Comment 52

[Jorg K (CEST = GMT+2)]

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.

Comment 53

[Jorg K (CEST = GMT+2)]

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

Oh, joy!

Comment 54

[Alessandro Castellani [:aleca]]

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

Comment 55

[Alessandro Castellani [:aleca]]

Attachment: 1576365-richlistbox-bug.patch

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

Comment 56

[Jorg K (CEST = GMT+2)]

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.

Comment 57

[Pulsebot]

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

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dc8ba56d0d54
Prettier fix. r=me DONTBUILD

Comment 59

[Jorg K (CEST = GMT+2)]

TB 68.1 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/d4b9a816410ee2f1338f9ca78f51e7d1ff6a12f7

Comment 60

[Jorg K (CEST = GMT+2)]

Working in TB 68.1.

Comment 61

[Jorg K (CEST = GMT+2)]

TB 70 beta 1:
https://hg.mozilla.org/releases/comm-beta/rev/dae1d52a933add5f700d042f132d78607066a408