Closed Bug 1056034 Opened 7 years ago Closed 7 years ago
Update Timer Manager .js does not properly update last Update Time
8.38 KB, text/plain
658 bytes, text/plain
1.38 KB, text/plain
2.26 KB, patch
|Details | Diff | Splinter Review|
So the call triggering this is at http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateTimerManager.js#239 The lastUpdateTime variable is declared in this while() http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateTimerManager.js#187 but since it's declared with a let, it's scoped only to this block and not the whole function.
This log documents the checking behavior with the current code.
This log documents the behavior of the code after fixing by declaring the variable using 'var' to make sure it's scoped to the whole function instead of the block only with 'let'.
From attachment 8475874 [details] we can see that because of the error, the timer updating logic gets broken and it triggers a check for "user-agent-updates-timer" every two minutes (the value of minimum delay) while we expect this to be done every week. This may have a play in bug 1020000. With attachment 8475876 [details], the only gecko change is just declaring this variable as a 'var' instead of 'let'. This makes sure the variable exists when we want to use it. We can see from the log that we do not have a check every two minutes. In both case, the device was wiped before testing, and left unattented for at least 2h. Update interval was configured to 30 min.
This documents the network activity. This was reproduced with the buggy code. The device was left unattented for about half an hour, with screen turned on to make sure wifi do not drop. It shows that every 120 seconds we are doing a https request against dynamicua.cdn.mozilla.net.
Scope of 'let' variables is limited to the current block, however, there is a later use of this variable that is done outside of the block where it's declared as 'var'. We declare the variable as 'var' and at the top level of the function to make sure that it is a legit one when we want to assign it.
Comment on attachment 8475908 [details] [diff] [review] Avoid unassigned lastUpdateTime use Robert and Brian, you have been recently doing most of the reviews around this code, can I get your point of view on this? We can also declare as 'var' at the same place instead of using 'let', but I feel it's less confusing to declare at top, given the usage of the variable.
Comment on attachment 8475908 [details] [diff] [review] Avoid unassigned lastUpdateTime use Agreed and looks fine.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8) > Comment on attachment 8475908 [details] [diff] [review] > Avoid unassigned lastUpdateTime use > > Agreed and looks fine. Thanks, I forgot to send a try, it's now done: https://tbpl.mozilla.org/?tree=Try&rev=ae82f591b7c3
(In reply to Alexandre LISSY :gerard-majax from comment #9) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #8) > > Comment on attachment 8475908 [details] [diff] [review] > > Avoid unassigned lastUpdateTime use > > > > Agreed and looks fine. > > Thanks, I forgot to send a try, it's now done: > https://tbpl.mozilla.org/?tree=Try&rev=ae82f591b7c3 That looks mostly green :)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
(2.0+, since this blocks bug 1051083)
blocking-b2g: --- → 2.0+
You need to log in before you can comment on or make changes to this bug.