nsUpdateTimerManager.js does not properly update lastUpdateTime

RESOLVED FIXED in Firefox 34, Firefox OS v2.0

Status

Firefox OS
Gaia::System
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: gerard, Assigned: gerard)

Tracking

unspecified
2.1 S3 (29aug)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [systemsfe])

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
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}]
(Assignee)

Comment 1

3 years ago
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.
(Assignee)

Comment 2

3 years ago
Created attachment 8475874 [details]
Behavior with the current code

This log documents the checking behavior with the current code.
(Assignee)

Comment 3

3 years ago
Created attachment 8475876 [details]
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'.
(Assignee)

Comment 4

3 years ago
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
(Assignee)

Comment 5

3 years ago
Created attachment 8475905 [details]
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.
(Assignee)

Comment 6

3 years ago
Created attachment 8475908 [details] [diff] [review]
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.
(Assignee)

Comment 7

3 years ago
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+
(Assignee)

Comment 9

3 years ago
(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
(Assignee)

Comment 10

3 years ago
(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 :)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/88707197f34f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/88707197f34f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
(2.0+, since this blocks bug 1051083)
blocking-b2g: --- → 2.0+
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/21925a037ec3
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox32: --- → wontfix
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
Whiteboard: [systemsfe]
Duplicate of this bug: 1052598
You need to log in before you can comment on or make changes to this bug.