The default bug view has changed. See this FAQ.

typo in autosync.js

RESOLVED FIXED in Thunderbird 42.0

Status

Thunderbird
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Hugo, Assigned: Hugo)

Tracking

38 Branch
Thunderbird 42.0

Thunderbird Tracking Flags

(thunderbird_esr3840+ fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8632499 [details] [diff] [review]
patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150702232110

Steps to reproduce:

The var _running is defined first as _runnning (with 3 'n') (at line 39 and 154 of the current HEAD). Patch attached

With my account, which have any folders, some of them are not synced after the startup. Is not always reproducible I think, but the patch is so simple that I didn't bother to find a simpler test case.


Actual results:

Folders no synced (but again, the code speaks for itself).
(Assignee)

Updated

2 years ago
Component: Untriaged → General

Updated

2 years ago
Attachment #8632499 - Attachment is patch: true

Updated

2 years ago
Attachment #8632499 - Flags: review?(mconley)
Comment on attachment 8632499 [details] [diff] [review]
patch

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

LGTM - thanks!
Attachment #8632499 - Flags: review?(mconley) → review+
Assignee: nobody → hugo.arregui
Keywords: checkin-needed

Comment 2

2 years ago
url:        https://hg.mozilla.org/comm-central/rev/07d2c884705e65747acadac646afc27c1ea6b5cd
changeset:  07d2c884705e65747acadac646afc27c1ea6b5cd
user:       Hugo <hugo.arregui>
date:       Sat Jul 11 16:41:00 2015 +0200
description:
Bug 1182831 - Typo in autosync.js. r=mconley

Updated

2 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 42.0

Comment 3

2 years ago
Pretty simple fix we should consider for next release.
status-thunderbird_esr38: --- → affected
tracking-thunderbird_esr38: --- → +

Updated

2 years ago
Attachment #8632499 - Flags: approval-comm-esr38?

Comment 4

2 years ago
Comment on attachment 8632499 [details] [diff] [review]
patch

http://hg.mozilla.org/releases/comm-esr38/rev/862df44d911d
Attachment #8632499 - Flags: approval-comm-esr38? → approval-comm-esr38+

Updated

2 years ago
status-thunderbird_esr38: affected → fixed
tracking-thunderbird_esr38: + → 40+
I'm digging around for causes of 
- [Bug 1196662] Thunderbird not checking for mails after hibernation
- [Bug 1195043] Thunderbird 38.2.0 does not retrieve emails
which started in version 38.2.0 - mail check works in 38.1.0. (personnally I've had no problems)

Is it possible this bug had side effects? I don't see anything else obvious in our uplifts to 38.2.0 nor in gecko landings http://mzl.la/1U4btYy that might have caused this.
Flags: needinfo?(rkent)
Flags: needinfo?(mkmelin+mozilla)

Comment 6

2 years ago
Unexpected side affects are possible I suppose, but I'm note likely to have time to investigate any time soon.
Flags: needinfo?(rkent)

Comment 7

2 years ago
Without the patch the code was checking for !this._runnning, which was probably always true, so the process.state was set (in onDownloadCompleted()). Maybe now that the variable sometimes is true (when autosync is doing something), and you hibernate, the action is never completed (some timer is lost) so onStateChanged never sets the variable to true. You debug this theory by logging the value of "running" in onStateChanged.

Updated

2 years ago
Flags: needinfo?(mkmelin+mozilla)
Depends on: 1196662
aryx has determined this is not the cause of bug 1196662. 

However, I have some concerns:
* the patch does not have a test - or does not identify a broken test
* I don't understand the rationale for taking this on esr, even with low risk, expecially given the short bake time on comm-central
* if this is a longstanding bug, why don't we have other bug reports on it?  Or if we do, why haven't they been recogonized? (for lack of a better term)
* given the patch, under what circustances would we expect to see bad autosync behavior, and are any potential unwanted side effects of the change?  (the later sounds like a stupid question, but I don't know the larger code logic

needinfo rkent, mainly regarding item 1 and 2
Flags: needinfo?(rkent)
No longer depends on: 1196662

Comment 9

2 years ago
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #8)
> aryx has determined this is not the cause of bug 1196662. 
> 
> However, I have some concerns:
> * the patch does not have a test - or does not identify a broken test
> * I don't understand the rationale for taking this on esr, even with low
> risk, expecially given the short bake time on comm-central
> * if this is a longstanding bug, why don't we have other bug reports on it? 
> Or if we do, why haven't they been recogonized? (for lack of a better term)
> * given the patch, under what circustances would we expect to see bad
> autosync behavior, and are any potential unwanted side effects of the
> change?  (the later sounds like a stupid question, but I don't know the
> larger code logic

I'm not really sure what you are asking. It sounds like you want to have a discussion about the philosophy of taking patches on our release channel. My philosophy has been to accept low-risk fixes for problems on release, but we can discuss that, but this is probably not the place. The patch seemed very low risk to me.
Flags: needinfo?(rkent)
You need to log in before you can comment on or make changes to this bug.