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

RESOLVED FIXED in Thunderbird 3.0b3

Status

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: bugzilla.i.sekler, Assigned: standard8)

Tracking

({regression})

Trunk
Thunderbird 3.0b3
regression
Bug Flags:
blocking-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has patch])

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
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.
(Reporter)

Updated

11 years ago
Version: unspecified → Trunk
The same applies to Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081101 SeaMonkey/2.0a2pre
(Reporter)

Comment 2

11 years ago
OS -> All, based on Comment #1.
OS: Linux → All
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?
Component: Address Book → Address Book
Flags: blocking-thunderbird3+
Keywords: regression
Product: Thunderbird → MailNews Core
QA Contact: address-book → addressbook
Hardware: PC → All
Target Milestone: --- → Thunderbird 3.0b2
(Assignee)

Comment 4

11 years ago
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.
(Assignee)

Comment 5

11 years ago
This works for me on today's build. Can anyone else confirm?
(Reporter)

Comment 6

11 years ago
(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
(Assignee)

Comment 7

11 years ago
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?
(Reporter)

Comment 8

11 years ago
(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?
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.
Bumping off the b2 blocker list, we're going to have to fix this soon.
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0rc1
(Assignee)

Updated

10 years ago
Assignee: nobody → bugzilla
Still see this on linux.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b3
(Assignee)

Comment 13

10 years ago
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.
Assignee: bugzilla → Pidgeot18
Summary: "Allow remote images in HTML mail" state not saved → "Allow remote images in HTML mail" state not saved across restarts
Whiteboard: [diagnosed][needs comment Joshua]
(Assignee)

Comment 14

10 years ago
Posted patch Proposed fixSplinter Review
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.
Assignee: Pidgeot18 → bugzilla
Status: NEW → ASSIGNED
Attachment #370199 - Flags: superreview?(neil)
Attachment #370199 - Flags: review?(Pidgeot18)
(Assignee)

Updated

10 years ago
Whiteboard: [diagnosed][needs comment Joshua] → [has patch][needs review Joshua,Neil]

Updated

10 years ago
Attachment #370199 - Flags: superreview?(neil) → superreview+
Comment on attachment 370199 [details] [diff] [review]
Proposed fix

Personally I'd lean to != false
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...
Attachment #370199 - Flags: review?(Pidgeot18) → review+
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review Joshua,Neil] → [has patch]
(Assignee)

Updated

10 years ago
Duplicate of this bug: 502662
You need to log in before you can comment on or make changes to this bug.