461660 - "Allow remote images in HTML mail" state not saved across restarts

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Ilja Sekler]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081025 Firefox/3.1b2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081025 Shredder/3.0b1pre

Unchecking the checkbox to allow remote images in HTML mails in an address book card persists only until the address book window remains open. After closing and reopening the address book the checkbox is checked again.

Reproducible: Always

Steps to Reproduce:
1. Open Adress Book
2. Edit a contact, uncheck "Allow remote images in HTML mail" (click "OK")
3. Close the Adress Book window and open it again
4. Open the contact you disallowed to load remote images for editing.
Actual Results:  
"Allow remote images in HTML mail" is checked.

Expected Results:  
"Allow remote images in HTML mail" remains unchecked.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

The same applies to Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081101 SeaMonkey/2.0a2pre

Comment 2

[Ilja Sekler]

OS -> All, based on Comment #1.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

I think the bug is somewhere in nsAbCardProperty.cpp which is backend code shared between TB and SM (if I'm right Product should be changed to MailNews Core).

In abCardOverlay.js (TB and SM version), kNonVcardFields contains "allowRemoteContent" which suggests that this property should not be saved to a VCard. Accordingly nsAbCardProperty::ConvertToEscapedVCard() in nsAbCardProperty.cpp doesn't query and write the value of kAllowRemoteContentProperty. Furthermore this property isn't queried or written anywhere else in the file, only set to false during initialization. If the property is not to be written to a VCard, where should its value be stored (permanently) then?

Comment 4

[Mark Banner (:standard8)]

Joshua, any chance you could take a look at this as I think its a regression from the AB card refactoring?

(In reply to comment #3)
> In abCardOverlay.js (TB and SM version), kNonVcardFields contains
> "allowRemoteContent" which suggests that this property should not be saved to a
> VCard. Accordingly nsAbCardProperty::ConvertToEscapedVCard() in
> nsAbCardProperty.cpp doesn't query and write the value of
> kAllowRemoteContentProperty. Furthermore this property isn't queried or written
> anywhere else in the file, only set to false during initialization. If the
> property is not to be written to a VCard, where should its value be stored
> (permanently) then?

The convert to escaped vcard is only used for saving as a vcard for attaching to outgoing emails (dig around in top-level account preferences), hence you don't need to save it there.

Comment 5

[Mark Banner (:standard8)]

This works for me on today's build. Can anyone else confirm?

Comment 6

[Ilja Sekler]

(In reply to comment #5)
> This works for me on today's build. Can anyone else confirm?

No, still broken.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081118 Thunderbird/3.0b1pre

hg identify
be77b2aa036b tip

Comment 7

[Mark Banner (:standard8)]

Ilja, please can you check the error console for any messages whilst you're editing/saving a contact.

Also, is this in your local address book? Not an LDAP one? (not that it should be enabled in an LDAP book).

Do other changes to your address book work?

Comment 8

[Ilja Sekler]

(In reply to comment #7)
> Ilja, please can you check the error console for any messages whilst you're
> editing/saving a contact.

No messages. The warning

Warning: Timed textboxes are deprecated. Consider using type="search" instead.
Source File: chrome://messenger/content/addressbook/addressbook.xul
Line: 0

just opening the address book is unrelated, I think.

> Also, is this in your local address book?

Yes.

> Not an LDAP one? [...].

No.

> Do other changes to your address book work?

Yes, I haven't found any other that would not get saved, except of this one.

BTW: Is there a special reason to keep this bug UNCONFIRMED?

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Also note that "separate address list for addresses allowed to load remote images for email" (bug 457296) is set for the b2 time frame.  I'm not sure of the blocker status of this assuming the other bug gets some traction.

Comment 10

[Bryan Clark (DevTools PM) [:clarkbw]]

Bumping off the b2 blocker list, we're going to have to fix this soon.

Comment 11

[Magnus Melin [:mkmelin]]

Also see bug 476275.

Comment 12

[Magnus Melin [:mkmelin]]

Still see this on linux.

Comment 13

[Mark Banner (:standard8)]

I now see what is happening here:

1) AllowRemoteContent is intended as a boolean field (see nsIAbCard.idl).
2) When we edit a card, the AllowRemoteContent property is set to true or false as appropriate.
3) When we reload the card in the same session, the true/false is assigned to the .checked attribute on the checkbox and we correctly display the state.
4) When the card is saved to the local address book, it is converted to a string and saved as '0' or '1'.
5) TB is then restarted, and reads the value from the local AB -> '0' or '1', the .checked attribute doesn't know about this and always gets set to checked.

Hence the issue.

We may go away from storing this value within the address book via bug 457296, however this does point to the fact that our current local database implementation can't cope with boolean values correctly.

Assigning to Joshua for comment as he knows what most of that revised code does.

Comment 14

[Mark Banner (:standard8)]

Attachment: Proposed fix

Proposed fix. I don't think we're going to get a sane way to deal with boolean values without a lot of special casing. So I think its best just to ensure the values are boolean when we need them to be.

Comment 15

[[e-mail]]

Comment on attachment 370199 [details] [diff] [review]
Proposed fix

Personally I'd lean to != false

Comment 16

[Joshua Cranmer [:jcranmer]]

Comment on attachment 370199 [details] [diff] [review]
Proposed fix

If we were already using SQLite address books, we could differentiate between strings and integers in storage; I don't see an efficient, backwards-compatible way to do that in mork, so we're stuck as is for right now.

And this textbox is way too small...

Comment 17

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/62b3f089e1b4