Closed Bug 1056034 Opened 7 years ago Closed 7 years ago

nsUpdateTimerManager.js does not properly update lastUpdateTime

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, firefox32 wontfix, firefox33 wontfix, firefox34 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S3 (29aug)
blocking-b2g 2.0+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(4 files)

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}]
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.
Blocks: 1020000
Attached file 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.
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.
Attachment #8475908 - Flags: review?(robert.strong.bugs)
Attachment #8475908 - Flags: review?(netzen)
Comment on attachment 8475908 [details] [diff] [review]
Avoid unassigned lastUpdateTime use

Agreed and looks fine.
Attachment #8475908 - Flags: review?(robert.strong.bugs)
Attachment #8475908 - Flags: review?(netzen)
Attachment #8475908 - Flags: review+
(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 :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/88707197f34f
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+
Whiteboard: [systemsfe]
Duplicate of this bug: 1052598
You need to log in before you can comment on or make changes to this bug.