540356 - Enforce a sane minimum interval for app.update.timer

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

Attachment: patch rev1

I think 60 seconds should be adequate. The patch also allows xpcshell tests to set a value of 500 ms.

Comment 1

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

Comment on attachment 422140 [details] [diff] [review]
patch rev1

I think I'd prefer to go with test-init instead of xpcshell-test-init. Besides that this should be good to go.

Comment 2

[Justin Dolske [:Dolske]]

Comment on attachment 422140 [details] [diff] [review]
patch rev1

>+    case "xpcshell-test-init":
>+      // For xpcshell tests require a minimum timer interval of 500 ms.
>+      minInterval = 500;
>     case "profile-after-change":

Add a /* fallthru */ comment here so it's clear that you didn't want a |break| here.

>-      this._start();
>+      this._timerInterval = Math.max(getPref("getIntPref", PREF_APP_UPDATE_TIMER, 600000),
>+                                     minInterval);
>+      this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+      this._timer.initWithCallback(this, this._timerInterval,
>+                                   Ci.nsITimer.TYPE_REPEATING_SLACK);

I think you'll end up with 2 timers here after xpcshell-test-init fires. Will any tests get confused if the old timer happens to fire while the test is running?

>-  gUTM.observe(null, "profile-after-change", "");
>+  gUTM.observe(null, "xpcshell-test-init", "");

I'd rename that to something less generic, lest that actually become a real notification xpcshells tests fire. :)

r+ with this stuff addressed.

Comment 3

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

(In reply to comment #2)
>...
> Add a /* fallthru */ comment here so it's clear that you didn't want a |break|
> here.
Done

> I think you'll end up with 2 timers here after xpcshell-test-init fires. Will
> any tests get confused if the old timer happens to fire while the test is
> running?
xpcshell tests don't fire profile-after-change but I might be needing this with mochi tests which do so I'll cancel / reinit the timer.

> >-  gUTM.observe(null, "profile-after-change", "");
> >+  gUTM.observe(null, "xpcshell-test-init", "");
> 
> I'd rename that to something less generic, lest that actually become a real
> notification xpcshells tests fire. :)
Done

I'll attach the updated patch before checkin and thanks for the review!

Comment 4

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

Attachment: patch for checkin

Carrying forward r+

Comment 5

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

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/8d97e5540546

Comment 6

[Johnathan Nightingale [:johnath]]

Comment on attachment 424387 [details] [diff] [review]
patch for checkin

Moving out to 1.9.2.3. This is mostly a housekeeping fix, aiui, saves us from getting into a bad QA situation while verifying update behaviour. I'm okay to take it, but it's not something we'd hold this release for.

Comment 7

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

Comment on attachment 424387 [details] [diff] [review]
patch for checkin

Let's hold off on this until after Lorentz and land it with bug 530872 and bug 538331 if they get approved