Last Comment Bug 1182831 - typo in autosync.js
: typo in autosync.js
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: 38 Branch
: Unspecified Unspecified
-- normal (vote)
: Thunderbird 42.0
Assigned To: Hugo
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-07-11 16:41 PDT by Hugo
Modified: 2015-08-25 10:34 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
40+
fixed


Attachments
patch (686 bytes, patch)
2015-07-11 16:41 PDT, Hugo
mconley: review+
rkent: approval‑comm‑esr38+
Details | Diff | Splinter Review

Description User image Hugo 2015-07-11 16:41:27 PDT
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).
Comment 1 User image Mike Conley (:mconley) 2015-07-14 08:16:46 PDT
Comment on attachment 8632499 [details] [diff] [review]
patch

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

LGTM - thanks!
Comment 2 User image aleth [:aleth] 2015-08-06 14:32:07 PDT
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 User image Kent James (:rkent) 2015-08-06 14:52:10 PDT
Pretty simple fix we should consider for next release.
Comment 5 User image Wayne Mery (:wsmwk, NI for questions) 2015-08-21 12:34:57 PDT
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 User image Kent James (:rkent) 2015-08-21 12:46:29 PDT
Unexpected side affects are possible I suppose, but I'm note likely to have time to investigate any time soon.
Comment 7 User image :aceman 2015-08-21 12:57:57 PDT
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 User image Wayne Mery (:wsmwk, NI for questions) 2015-08-25 06:59:01 PDT
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 User image Kent James (:rkent) 2015-08-25 10:34:38 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.