1182831 - typo in autosync.js

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Hugo]

Attachment: 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).

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8632499 [details] [diff] [review]
patch

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

LGTM - thanks!

Comment 2

[aleth [:aleth]]

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

Comment 3

[Kent James (:rkent)]

Pretty simple fix we should consider for next release.

Comment 4

[Kent James (:rkent)]

Comment on attachment 8632499 [details] [diff] [review]
patch

http://hg.mozilla.org/releases/comm-esr38/rev/862df44d911d

Comment 5

[Wayne Mery (:wsmwk)]

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.

Comment 6

[Kent James (:rkent)]

Unexpected side affects are possible I suppose, but I'm note likely to have time to investigate any time soon.

Comment 7

[:aceman]

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.

Comment 8

[Wayne Mery (:wsmwk)]

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

Comment 9

[Kent James (:rkent)]

(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.