1494238 - Replace class AdoptUTF8StringEnumerator by using NS_NewAdoptingUTF8StringEnumerator()

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Jorg K (CEST = GMT+2)]

As per IRC:

10:09:26 - darktrojan: darktrojan> jorgk, I don't think the class AdoptUTF8StringEnumerator is needed now
10:09:27 - darktrojan: <darktrojan> there's NS_NewAdoptingUTF8StringEnumerator

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1494238-AdoptUTF8StringEnumerator.patch

IRC:
10:09:27 - darktrojan: <darktrojan> but that's just my uneducated look at it

Now the reviewer can take another educated look at it ;-)

Comment 2

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aaa4fd29eb4d6298ffd1d8e7828085b2706cbbbd

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9012095 [details] [diff] [review]
1494238-AdoptUTF8StringEnumerator.patch

In case I have no better patches to land, perhaps Aceman can review this.

Comment 4

[:aceman]

Comment on attachment 9012095 [details] [diff] [review]
1494238-AdoptUTF8StringEnumerator.patch

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

Interesting. The code in m-c seems to do the same.
Let's hope they do not remove the class soon, there are very few uses in m-c. But then if they migrate to something else, we should be able to do the same.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1494238-AdoptUTF8StringEnumerator.patch - complete removal

How do you feel about removing the whole thing instead? It appears to be dead code.

I also wonder whether it ever worked. Unlike its M-C friend here:
https://searchfox.org/mozilla-central/rev/ffe6eaf2f032e58ec3b0650a87df2c62ae4ca441/xpcom/ds/nsStringEnumerator.cpp#339
https://searchfox.org/mozilla-central/rev/ffe6eaf2f032e58ec3b0650a87df2c62ae4ca441/xpcom/ds/nsStringEnumerator.cpp#284

our own little beauty was missing the ref-counting, see:
https://searchfox.org/comm-central/rev/4064a6764827b3e78cf33ca2342af8520ff4a411/mailnews/imap/src/nsImapMailFolder.cpp#6383

Comment 6

[Jorg K (CEST = GMT+2)]

I've added this in am-server.js where you select an IMAP trash folder:
function folderPickerChange(aEvent)
{
  var folder = aEvent.target._folder;
  let users = folder.QueryInterface(Ci.nsIMsgImapMailFolder).getOtherUsersWithAccess();
  for (u of users) dump(`=== here is a user ${u}\n`);

It prints OK, then crashes. Next I added the missing NS_ADDREF() like so:
  NS_ADDREF(*aResult = new AdoptUTF8StringEnumerator(resultArray));
and it no longer crashes.

So, two options: Use first patch that will call M-C code that has the addref and the function will work, or remove the dead faulty code.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: GetOtherUsers.patch - patch for experimenting

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9012399 [details] [diff] [review]
GetOtherUsers.patch - patch for experimenting

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

I'm usually keen on getting rid of leftovers. But this seems like it would be a very nice addition in the folder properties Sharing tab.

Tried this patch but the only folder that it will show something on is the inbox (testing on fastmail). For the other folders there's no dump. The folderPickerChange is triggered but whatever errors follows are silently swallowed

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 9012240 [details] [diff] [review]
1494238-AdoptUTF8StringEnumerator.patch - complete removal

OK, yes, it could be a useful function to have in the box. Let's fix it with the other patch then. I'll test it again with the experimental patch.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/102bf0b97b0a
Fix nsMsgIMAPFolderACL::GetOtherUsers() (missing addref) by replacing class AdoptUTF8StringEnumerator with using NS_NewAdoptingUTF8StringEnumerator(). r=aceman

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: GetOtherUsers.patch - patch for experimenting - rebased