Closed Bug 1383617 Opened 2 years ago Closed 2 years ago

Simplify nsAddrDatabase.cpp by removing "deleted cards" table

Categories

(MailNews Core :: Address Book, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 57.0

People

(Reporter: jorgk, Assigned: jorgk)

Details

(Keywords: privacy)

Attachments

(1 file, 5 obsolete files)

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=foo@bar.com)

I have the impression that this code could be the cause of many "idiosyncrasies" of the address book, listed as blocking bug 758969.
Patch moved over from bug 1329295.
Attachment #8889287 - Flags: feedback?(acelists)
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?
Flags: needinfo?(hgruenberg)
I'd like to help. But what does it involve?
Flags: needinfo?(hgruenberg)
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.
(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 on attachment 8889287 [details] [diff] [review]
1383617-deleted-cards-table.patch (v2).

Maybe a review request gets more attention.
Attachment #8889287 - Flags: feedback?(acelists) → review?(acelists)
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.
Attachment #8889287 - Flags: feedback+
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Keywords: privacy
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
(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.
Yes, but 6 months with a reason (excuse) is better than eternity with no reason :)
So please delete the tables when found.
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.
Attachment #8889287 - Attachment is obsolete: true
Attachment #8889287 - Flags: review?(acelists)
Attachment #8896874 - Flags: feedback?(acelists)
Fixed comment.
Attachment #8896876 - Flags: feedback?(acelists)
(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.
Thanks, please let me know what you find. We can put m_mdbDeletedCardsTable back and and release it.
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 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.
Looks like this deletes the unwanted table eventually. I don't think we can do any better.

Thanks for the hint.
Attachment #8896874 - Attachment is obsolete: true
Attachment #8896876 - Attachment is obsolete: true
Attachment #8897584 - Attachment is obsolete: true
Attachment #8896874 - Flags: feedback?(acelists)
Attachment #8896876 - Flags: feedback?(acelists)
Attachment #8897602 - Flags: review?(acelists)
What is the difference? Only Commit after Release?
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.
Sorry about the noise, but m_mdbStore and m_mdbEnv are checked at the beginning of the function, so no need to check again.
Attachment #8897602 - Attachment is obsolete: true
Attachment #8897602 - Flags: review?(acelists)
Attachment #8897616 - Flags: review?(acelists)
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.
Attachment #8897616 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/06665e6b5cfa
Remove deleted cards table. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
(In reply to :aceman from comment #20)
> Thanks, this seems to work.
Thanks for your help!
Target Milestone: --- → Thunderbird 57.0
You need to log in before you can comment on or make changes to this bug.