1383617 - Simplify nsAddrDatabase.cpp by removing "deleted cards" table

Status: RESOLVED FIXED

(MailNews Core :: Address Book, enhancement)

Description

[Jorg K (CEST = GMT+2)]

Looking at bug 1329295, I noticed the "deleted cards" table. According to a comment, this was implemented for palm sync, but palm sync is long dead.

From IRC:
[2017-07-23 23:27:28] <jcranmer> jorgk_: it was killed several years ago
[2017-07-23 23:27:49] <jcranmer> Standard8 shoved it out into a separate repository, and we last gave up trying to support it in 2008 or 2009
[2017-07-23 23:28:23] <jcranmer> https://hg.mozilla.org/users/bugzilla_standard8.plus.com/palmsync/

We should simplify nsAddrDatabase.cpp and remove the unnecessary code about which we read:
// see bug #198303
// the addition of the m_mdbDeletedCardsTable table has complicated life in the addressbook
// (it was added for palm sync).  until we fix the underlying problem, we have to jump through hoops
// in order to know if we have a row (think card) for a given column value (think email=[e-mail])

I have the impression that this code could be the cause of many "idiosyncrasies" of the address book, listed as blocking bug 758969.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1383617-deleted-cards-table.patch (v2).

Patch moved over from bug 1329295.

Comment 2

[Jorg K (CEST = GMT+2)]

Reporter of bug 1329295: Would you be interested to run a Daily try build to check whether what I'm proposing here resolves some of the weird problems reported in bug 1329295?

Comment 3

[hgruenberg]

I'd like to help. But what does it involve?

Comment 4

[Jorg K (CEST = GMT+2)]

If you still have reproducible weird cases, you'd need to download and install a "special" Daily version of Thunderbird which you can install parallel with your production version. Then you can check whether the special version is any better.

There have been a lot of reports of "weird" behaviour, see bugs listed as blocking bug 758969. But personally I haven't seen a single reproducible case. And generally people don't want to hand out their address books, so this is really a difficult problem to catch.

Comment 5

[hgruenberg]

(In reply to Jorg K (GMT+2) from comment #4)
> If you still have reproducible weird cases, you'd need to download and
> install a "special" Daily version of Thunderbird which you can install
> parallel with your production version. Then you can check whether the
> special version is any better.
> 
> There have been a lot of reports of "weird" behaviour, see bugs listed as
> blocking bug 758969. But personally I haven't seen a single reproducible
> case. And generally people don't want to hand out their address books, so
> this is really a difficult problem to catch.

Unfortunately there's a problem with that.  When I reported the bug I was president of an organization whose e-mail addresses I used to maintain in a Mailing List. I've since been succeeded and have turned over the Mailing List.  So, I'd have to create a new Mailing List and invent e-mail addresses to put in it.  If you have a better idea, I'm amenable.

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8889287 [details] [diff] [review]
1383617-deleted-cards-table.patch (v2).

Maybe a review request gets more attention.

Comment 7

[:aceman]

Comment on attachment 8889287 [details] [diff] [review]
1383617-deleted-cards-table.patch (v2).

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

Nice removal :) xpcshell and mozmill/addrbook passes.
So by reading the code it seems a deleted card was removed from the main table, but a simplified copy (only some fields) were put into the deletedCards table. What problems did it cause?

I thought the problem was the deleted card was left in the main table just a note was put into the deletedCards table which said the card was deleted. And that some code incorrectly didn't consult this deletedCards table and thus incorrectly thought some cards in the main table were still valid.

Also, if we land this, what about existing users that already have this deletedCards table in their ABs. Do we just silently ignore it? I would think this may be a privacy risk. Users deleted some cards and expect them to be gone. But this table keeps a copy of them, now forever.

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #7)
> What problems did it cause?
Good question. Surely it's overcomplicating the code and it is a maintenance burden. The motivation here is really bug 758969 and the bugs I've added as blocking it recently, bug 1329295, bug 1352079, bug 1371336. I tried to investigate bug 1329295 and found this "beautiful" code. I can't rule out a logic error somewhere when dealing with those deleted cards, so removing this cruft will be the first step in shining some light into the dark.

I'll do a patch what will remove the deleted cards table so there is no privacy issue. BTW, that issue exists now since those phantoms are kept for six months before they get purged.

Comment 9

[:aceman]

Yes, but 6 months with a reason (excuse) is better than eternity with no reason :)
So please delete the tables when found.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1383617-deleted-cards-table.patch (v3).

There doesn't appear to be a function to delete a table, but you can empty it out.

So where before the deleted row was added to the deleted cards table and the table was purged, I now empty it out.

As before, if the user never deletes a card, the deleted cards table will hang around for a long time :-(

Untested since I need to move on to fixing some bustage. Try interdiff.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1383617-deleted-cards-table.patch (v3b).

Fixed comment.

Comment 12

[:aceman]

(In reply to Jorg K (GMT+2) from comment #10)
> Untested since I need to move on to fixing some bustage. Try interdiff.

The code looks good.
But I tested this and the contents of the table is not purged from the .mab file. I'm playing with it to see what is missing.
E.g. the old table had m_mdbDeletedCardsTable->Release() in the object destructor.

Comment 13

[Jorg K (CEST = GMT+2)]

Thanks, please let me know what you find. We can put m_mdbDeletedCardsTable back and and release it.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1383617-deleted-cards-table.patch (v3c).

I can't find any way to really clear out the .mab file. The stuff sticks like glue. This is of course a terrible hack and it also doesn't work. See interdiff with v3b.

Comment 15

[:aceman]

Comment on attachment 8897584 [details] [diff] [review]
1383617-deleted-cards-table.patch (v3c).

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

::: mailnews/addrbook/src/nsAddrDatabase.cpp
@@ +1541,5 @@
> +    {
> +      m_mdbStore->GetTable(m_mdbEnv, &deletedCardsTableOID, &m_mdbDeletedCardsTable);
> +      if (m_mdbDeletedCardsTable) {
> +        m_mdbDeletedCardsTable->CutAllRows(m_mdbEnv);
> +        static_cast<morkTable*>(m_mdbDeletedCardsTable)->SetTableRewrite();

I tried
        deletedCardsTable->CutAllRows(m_mdbEnv);
        Commit(nsAddrDBCommitType::kCompressCommit);
        deletedCardsTable->Release();

here, but it also doesn't always clear the data. Some places also call ->CutAllColumns, but that is for single rows only and I do not understand what it should do.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1383617-deleted-cards-table.patch (v3d).

Looks like this deletes the unwanted table eventually. I don't think we can do any better.

Thanks for the hint.

Comment 17

[:aceman]

What is the difference? Only Commit after Release?

Comment 18

[Jorg K (CEST = GMT+2)]

Yep. I mean, if those names have a natural meaning, you'd release the thing first you want to compact later, no?

To test, I prepare an address book with a "normal version", add and delete cards, check in the editor. Then with the patched version I add and delete a card and check again and the table is gone.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1383617-deleted-cards-table.patch (v3e).

Sorry about the noise, but m_mdbStore and m_mdbEnv are checked at the beginning of the function, so no need to check again.

Comment 20

[:aceman]

Comment on attachment 8897616 [details] [diff] [review]
1383617-deleted-cards-table.patch (v3e).

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

Thanks, this seems to work.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/06665e6b5cfa
Remove deleted cards table. r=aceman

Comment 22

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #20)
> Thanks, this seems to work.
Thanks for your help!