1056034 - nsUpdateTimerManager.js does not properly update lastUpdateTime

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[:gerard-majax]

While investigating bug 1051083, I came accross a JS error reported in this file: [JavaScript Error: "assignment to undeclared variable lastUpdateTime" {file: "jar:file:///system/b2g/omni.ja!/components/nsUpdateTimerManager.js" line: 270}]

Comment 1

[:gerard-majax]

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.

Comment 2

[:gerard-majax]

Attachment: Behavior with the current code

This log documents the checking behavior with the current code.

Comment 3

[:gerard-majax]

Attachment: Behavior changing the let to a var

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

Comment 4

[:gerard-majax]

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.

Comment 5

[:gerard-majax]

Attachment: HTTP/HTTPS requests

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.

Comment 6

[:gerard-majax]

Attachment: Avoid unassigned lastUpdateTime use

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 7

[:gerard-majax]

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 8

[Robert Strong (they/them - no direct email)]

Comment on attachment 8475908 [details] [diff] [review]
Avoid unassigned lastUpdateTime use

Agreed and looks fine.

Comment 9

[:gerard-majax]

(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

Comment 10

[:gerard-majax]

(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 :)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/88707197f34f

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88707197f34f

Comment 13

[Michael Vines [:m1] [:evilmachines]]

(2.0+, since this blocks bug 1051083)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/21925a037ec3