Closed Bug 807848 Opened 7 years ago Closed 7 years ago

Account Mailbox does not appear in Folder Pane unless 'toolkit.telemetry.prompted' set to 'True'

Categories

(Thunderbird :: Folder and Message Lists, defect, major)

16 Branch
defect
Not set
major

Tracking

(thunderbird20 fixed, thunderbird21 fixed, thunderbird-esr1719+ fixed)

RESOLVED FIXED
Thunderbird 21.0
Tracking Status
thunderbird20 --- fixed
thunderbird21 --- fixed
thunderbird-esr17 19+ fixed

People

(Reporter: imogeng, Assigned: aceman)

References

()

Details

(Whiteboard: [gs])

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010144125

Steps to reproduce:

Upgraded to 16.0.2 on 4x Terminal Servers. 


Actual results:

Some end users could not see any account folders at all unless 'user_pref("toolkit.telemetry.prompted", 2);' is changed to 'user_pref("toolkit.telemetry.prompted", True);'  

The built in Error console showed: 
Timestamp: 02/11/12 11:25:24 AM
Error: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.setBoolPref]
Source File: chrome://messenger/content/specialTabs.js
Line: 784

When clicked on the error link above it showed:
 // Set pref to indicate we've shown the notification.
    prefs.setBoolPref(kTelemetryPrompted, true);

Which is odd because this is supposed to be an integer afaik.

We use a log on 'user.js' script which sets this value to '2'

See also: https://getsatisfaction.com/mozilla_messaging/topics/empty_folder_panes_in_thunderbird_16_0_deleting_foldertree_json_doesnt_fix_it




Expected results:

Display account and associated mailboxes in the folder pane.
OS: Windows 7 → Windows Server 2003
several reports in getsatisfaction
Severity: normal → major
Component: Untriaged → Folder and Message Lists
Whiteboard: [gs]
confirming based on multiple reports. will let other sort out the how, why, etc
Status: UNCONFIRMED → NEW
Ever confirmed: true
Seems true, in mozilla-central it is used as an int pref, but in some tests also as bool (sets to 'true'). Maybe the semantics changed recently:

http://mxr.mozilla.org/comm-central/search?string=PREF_TELEMETRY_PROMPTED&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

http://mxr.mozilla.org/comm-central/search?string=toolkit.telemetry.prompted&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

I can confirm the bug, if I set toolkit.telemetry.prompted to 2 via about:config. The question is how it can naturally get that value.
Do we need to just copy the logic used in Firefox at http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/nsBrowserGlue.js#958 ?
I think you mean the logic at lines 924-934, in which case, yes.
But it seems TB does not have the 2nd version labels. As the pref has no default defined in toolkit, it seems it is fine for TB to specify it as bool.

(In reply to Cj from comment #0)
> Which is odd because this is supposed to be an integer afaik.
> 
> We use a log on 'user.js' script which sets this value to '2'

It is an integer in Firefox, but a bool in TB. Why do you set it to 2?
Flags: needinfo?(craigh)
(In reply to :aceman from comment #6)
> But it seems TB does not have the 2nd version labels. As the pref has no
> default defined in toolkit, it seems it is fine for TB to specify it as bool.
> 
> (In reply to Cj from comment #0)
> > Which is odd because this is supposed to be an integer afaik.
> > 
> > We use a log on 'user.js' script which sets this value to '2'
> 
> It is an integer in Firefox, but a bool in TB. Why do you set it to 2?

It was set to '2' historically (I didn't do the coding), maybe based on shared usage between Firefox & TB. See https://bugzilla.mozilla.org/show_bug.cgi?id=697860 It used to work fine until 16.x.x In testing I've found that changing it to 'True' is user.js doesn't overwrite it prefs.js; it's effect seems to be random, only some users seem to affected, including a new user I set up.
Flags: needinfo?(craigh)
But Firefox and Thunderbird never share a profile. But as the pref name is the same I also this we should use it the same (at least the type should be the same).

This needs decision what we want to do here. If we just convert it to int or we also want all the new features as in https://bugzilla.mozilla.org/show_bug.cgi?id=697860#c15 .
Flags: needinfo?(mbanner)
@Cj: Thunderbird has never set the preference to '2'. When Firefox first introduced telemetry it was a boolean for them. Sometime in the last year (iirc) they changed it to an integer.

@aceman: I think the easy short-term improvement would be to try/catch on that preference get. The slower improvement would be to pick up the rest of the changes they've done for the telemetry prompts.
Flags: needinfo?(mbanner)
(In reply to Mark Banner (:standard8) from comment #9)
> @Cj: Thunderbird has never set the preference to '2'. When Firefox first
> introduced telemetry it was a boolean for them. Sometime in the last year
> (iirc) they changed it to an integer.

Noted. Our user.js script is used for both TB & FF (although they could be split) but it has now been changed to 'True'. The odd thing is why then does it only affect some users, and what is going on with the other reports of this error that are not in a enterprise environment?
Summary: Account Mailbox does appear in Folder Pane unless 'telemetry.prompted' set to 'True' → Account Mailbox does not appear in Folder Pane unless 'telemetry.prompted' set to 'True'
I wonder if this doesn't cause problems in Seamonkey. They may share code that handles the pref as int (from FF) and code that handles it as bool (from mailnews).

Standard8, do we also care about showing the new string (do we actually have a new version) or should we just silently bump all profiles to the value 2?
(In reply to :aceman from comment #11)
> Standard8, do we also care about showing the new string (do we actually have
> a new version) or should we just silently bump all profiles to the value 2?

I don't see any harm in adding the new functionality. I believe its just trying to get an actionable result.
Attached patch patch (obsolete) — Splinter Review
This should do this:
if getting toolkit.telemetry.prompted throws, or the returned int is lower than the current prompt revision, show telemetry prompt. After the prompt is closed, toolkit.telemetry.prompted is set to the current revision. Then
1. if it was dismissed via the close button, nothing else is set.
2. if Yes was clicked, toolkit.telemetry.enabled is set to true.
3. if No was clicked, toolkit.telemetry.rejected is set to true. (And toolkit.telemetry.enabled is left at the default of false. I added it to all-thunderbird.js as firefox also has a default for it)

This seems to match the FF implementation in bug 697860 comment 19.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #687882 - Flags: review?(mbanner)
This is not a problem in Seamonkey because they probably use prompt on the browser part and there is no reference to the pref in /mailnews and the /suite specific code.
OS: Windows Server 2003 → All
Hardware: x86 → All
If we also want to hide the close button as FF bug 697860 does, just tell me.
Hi, we recently landed some changes to this prompt in Firefox in bug 699806, and dependent bugs.

You need to update toolkit.telemetry.rejected and toolkit.telemetry.prompted on the pref pane, too. (See bug 737600 for what we do for Firefox)

Having a toolkit.telemetry.rejected pref that is not synced with current toolkit.telemetry.enabled state caused us a lot of troubles in bug 699806 (see the thread...).

If later we want to easily make telemetry opt-out, and, more important respect user privacy (because of that, unfortunately, there is some edge cases we couldn't handle: we couldn't say if the user made a choice or if he was in the default case.).

>user_pref("toolkit.telemetry.prompted", 999);
Also, why are you doing that? (Actually, I don't understand what's the problem with "2")
We still want to be able to reprompt already opted-in users if our policy changes, even on TB.

Sid, from a privacy point of view, am I correct?


(This is not the aim of this bug, but maybe you could be interested for TB about enabling telemetry by default for Daily/Earlybird, I can backport my patches if you want. Just tell me)
Flags: needinfo?(sstamm)
(In reply to Théo Chevalier [:tchevalier] from comment #16)
> We still want to be able to reprompt already opted-in users if our policy
> changes, even on TB.
> 
> Sid, from a privacy point of view, am I correct?

I'm not sure I follow the rest of the bug, but if our policy changes (and we prompted the first time) we should get consent again, yes.
Flags: needinfo?(sstamm)
(In reply to Théo Chevalier [:tchevalier] from comment #16)

> 
> (This is not the aim of this bug, but maybe you could be interested for TB
> about enabling telemetry by default for Daily/Earlybird, I can backport my
> patches if you want. Just tell me)

Protz, following the discussion on TBPlanning, is there anything to add here?
(In reply to Théo Chevalier [:tchevalier] from comment #16)
> Hi, we recently landed some changes to this prompt in Firefox in bug 699806,
> and dependent bugs.
> 
> You need to update toolkit.telemetry.rejected and toolkit.telemetry.prompted
> on the pref pane, too. (See bug 737600 for what we do for Firefox)
Thanks, I will look at those.

> >user_pref("toolkit.telemetry.prompted", 999);
> Also, why are you doing that? (Actually, I don't understand what's the
> problem with "2")
> We still want to be able to reprompt already opted-in users if our policy
> changes, even on TB.
No, this 999 value is set only in tests. We do not want the prompts there so I gave high value to skip any revisions.
(In reply to Mark Banner (:standard8) from comment #9)
> @Cj: Thunderbird has never set the preference to '2'. When Firefox first
> introduced telemetry it was a boolean for them. Sometime in the last year
> (iirc) they changed it to an integer.

Sorry to drag this up again. Looking once more at moving from 10.x ESR to 17 ESR and came across the problem I had the first time I tried 17 - no folder view. I too had the telemetry pref set to 2. It seems quite an arrogant statement to say "Thunderbird has never set the preference to '2'" when so many people have that value and almost certainly didn't set it to that by hand! There *must* have been some version of Thunderbird, perhaps a long time ago, that set this pref to 2 and the developers ought to accept that and deal with it.
Martin, we have a patch ready, standard8 just has to review it.
Comment on attachment 687882 [details] [diff] [review]
patch

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

Sorry for the delay in this, looks good.

::: mail/app/profile/all-thunderbird.js
@@ +361,5 @@
>  
>  pref("view_source.syntax_highlight", false);
>  
>  pref("toolkit.telemetry.infoURL", "http://www.mozilla.org/thunderbird/legal/privacy/#telemetry");
> +pref("toolkit.telemetry.enabled", false);

This is already defined in all.js so we don't need to define it here.
Attachment #687882 - Flags: review?(mbanner) → review+
I just noticed, this breaks the downgrade situation:

WARNING: Trying to set pref toolkit.telemetry.prompted to with the wrong type!: file /Users/moztest/comm/main/src/mozilla/modules/libpref/src/prefapi.cpp, line 754
JavaScript error: chrome://messenger/content/specialTabs.js, line 779: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.setBoolPref]


Could we land a patch on our existing branches to cope with that situation do you think?
Can we land the patch as is on the branches (17+) ? This is reported starting at 16. So migrating 17 <-> 21 in any direction will work?
Or where do we want to downgrade?
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #687882 - Attachment is obsolete: true
Attachment #711458 - Flags: review+
Keywords: checkin-needed
Whiteboard: [gs] → [gs][please leave open for branch checkins]
This doesn't apply cleanly to comm-central.
Keywords: checkin-needed
Yeah, Services conversions were faster :) Update coming in a moment.
Summary: Account Mailbox does not appear in Folder Pane unless 'telemetry.prompted' set to 'True' → Account Mailbox does not appear in Folder Pane unless 'toolkit.telemetry.prompted' set to 'True'
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #711458 - Attachment is obsolete: true
Attachment #711513 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d01165786209

Leaving open per the whiteboard (though I'll point out that we typically close bugs when they hit trunk and use the status flags for tracking uplifts).
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 21.0
It seems I forgot to make the necessary changes to the preferences pane :( Do we back this out?
https://bug737600.bugzilla.mozilla.org/attachment.cgi?id=690021

standard8, do we inherit MOZ_TELEMETRY_DISPLAY_REV from configure.in (in that patch) ro do we need/want to port it to ours?
Flags: needinfo?(mbanner)
Attached patch patch - preferences (obsolete) — Splinter Review
Attachment #711959 - Flags: review?(mbanner)
Since the last checkin here I get a blank folder pane when reverting to a 2 day old build with the same profile. Regression?
Yes, that is unfortunate side effect that standard8 talks about in comment 23.
You can't safely downgrade in nightlies. But when we push it to all branches, you will be able to migrate between the newest versions on each branch (e.g. 17.0.3, beta after the date of checkin, earlybird after the date of checkin, nightly after 2013-02-08). So all supported versions.
Ok, I think there are several options we have here:

1) Back out the patch, land a patch that "copes" with either boolean/int value on all branches. Then when we hit say Gecko 24, re-land this patch for the next ESR.

2) Use a different preference name

3) Land this patch everywhere and ignore the backwards compatibility issue.


I'm currently swaying towards option 1 which would give us at least some overlap for backwards compatibility.

I think 2 would be confusing as the toolkit pref is specified in all.js, so it would appear anyway, and it would be another pref different which isn't good for enterprises.

Option 3 makes it harder for regression testing, although I guess setting the value to True in user.js might work around it (though they would have to know they need to do that, which makes it less attractive as an option).

aceman, thoughts?
Flags: needinfo?(mbanner) → needinfo?(acelists)
(In reply to Mark Banner (:standard8) from comment #34)
> Ok, I think there are several options we have here:
> 
> 1) Back out the patch, land a patch that "copes" with either boolean/int
> value on all branches. Then when we hit say Gecko 24, re-land this patch for
> the next ESR.

The existing patch copes with bool/int if we push it to all branches. My question about backout was doe to the missing changes in preferences dialog. But I have already uploaded it so if you review it, we can land it too and all is OK.
Flags: needinfo?(acelists)
(In reply to Mark Banner (:standard8) from comment #34)
> Ok, I think there are several options we have here:
> 
> 1) Back out the patch, land a patch that "copes" with either boolean/int
> value on all branches. Then when we hit say Gecko 24, re-land this patch for
> the next ESR.
> 
> 2) Use a different preference name
This would not solve any problem. As diverging just the value (not even the pref name) caused this bug in the first place. The docs probably say to set it to 2 for Firefox and users expected that to work on TB too.

> 3) Land this patch everywhere and ignore the backwards compatibility issue.

I am not sure what is the difference in patch for 3) (the current one) and for 1). I think we agreed that the current patch is compatible enough (all newest versions on all branches of 17+) after we check it into all branches. So what is different in option 1)?
Flags: needinfo?(mbanner)
The difference in 1, is that we have about four-six months with no pref value change (or UI etc). When we then do the change later on, there's then those four-six months worth of builds that will cope with testers (and potentially users) switch back and forth.

If we did option three, then we don't get a grace period of allowing users to go back again.
Flags: needinfo?(mbanner)
me being one who bounces around randomly between releases and builds, I think I favor choice #1
OK, so you propose a patch that accepts bool AND int and converts it to bool (so that older nightlies in the current era still work)? That one gets in all branches. Then later we check-in the real patch into all branches, that accepts bool AND int but converts it to int?
Flags: needinfo?(mbanner)
Attachment #714080 - Flags: review?(mbanner)
If I understand it correctly, the idea is to backout patch v3 and immediately check in the transitional patch.
Comment on attachment 711959 [details] [diff] [review]
patch - preferences

So this one can wait for the backout and then merged with patch v3.
Attachment #711959 - Flags: review?(mbanner)
Comment on attachment 714080 [details] [diff] [review]
patch for transitional period

Ok, I've just tested it and it looks good. I'll land it in a moment with the backout from trunk.

Going to land it on aurora and esr17 as well as agreed.
Attachment #714080 - Flags: review?(mbanner)
Attachment #714080 - Flags: review+
Attachment #714080 - Flags: approval-comm-esr17+
Attachment #714080 - Flags: approval-comm-aurora+
Flags: needinfo?(mbanner)
Branch landings:

https://hg.mozilla.org/releases/comm-aurora/rev/ec4828fcca96
https://hg.mozilla.org/releases/comm-esr17/rev/66f7060807f0 (with minor bitrot fix)

aceman: I'd like to track this for landing in 24, but with the branch landings for this patch that is difficult, would you mind moving the combined patch to a new bug, and we'll track it there?
Attachment #711513 - Attachment is obsolete: true
Attachment #711959 - Attachment is obsolete: true
Sure, no problem.
Whiteboard: [gs][please leave open for branch checkins] → [gs]
Blocks: 842073
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Ryan VanderMeulen [:RyanVM][UTC-4] from comment #47)
> https://hg.mozilla.org/comm-central/rev/2870d0f37de0

This was actually bug 842073, sorry.
Flags: in-testsuite+
Still happening for me in Thunderbird 24.2, driving me mad

No mailboxes appear unless I open prefs.js, where I find

user_pref("toolkit.telemetry.prompted", 2);

And change it to 

user_pref("toolkit.telemetry.prompted", false);

Whereupon after restart the telemetry prompt appears _again_ 

Even though this is set

user_pref("toolkit.telemetry.rejected", true);

And next time I exit thunderbird it's set back to:

user_pref("toolkit.telemetry.prompted", 2);

And my Accounts aren't visible any more.

How cam I KILL TELEMETRY for EVER ?
user_pref("toolkit.telemetry.prompted", 2) is the correct value for TB24 (see bug 842073). Please open a new bug if it does not work for you and post contents of Tools -> Error console there.
You need to log in before you can comment on or make changes to this bug.