Fetch and execute recipes once every 24 hours if no restart

[Osmose]

We should be running recipes once every 24 hours for users who haven't restarted the browser in that time. Executing recipes should reset the time, so that a user with the browser open for an entire week should fetch and execute new recipes 7 times (unless their computer is asleep or some other state that stops Firefox from running; we should default to whatever behavior the updater uses in that case).

[mythmon]

I propose that we use this opportunity to decouple our execution from the browser start. Instead I suggest that we keep a record of the last time we have run. If it has been more than 24 hours since the last run, run again. We would check at browser start up, and then again periodically. In pseudo code:

on browser start, and every 30 minutes
    if last_run is more than 24 hours ago
        run()
        last_run = now

@Osmose @gregglind @MattGrimes What do you think about this?

[gregglind]

Perhaps obvious, but I would prefer a system that is robust to client clock. Otherwise, sounds fine to me. Every startup (NECESARY TO handles experiment tagging for ongoing At least once per day for ongoing sessions. Derail: Is there some etag/caching magic to allow more often than once per day cheaply?

…

On Fri, Mar 24, 2017 at 3:48 PM Mike Cooper ***@***.***> wrote: I propose that we use this opportunity to decouple our execution from the browser start. Instead I suggest that we keep a record of the last time we have run. If it has been more than 24 hours since the last run, run again. We would check at browser start up, and then again periodically. In pseudo code: on browser start, and every 30 minutes if last_run is more than 24 hours ago run() last_run = now @Osmose <https://github.com/Osmose> @gregglind <https://github.com/gregglind> @MattGrimes <https://github.com/MattGrimes> What do you think about this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#588 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAj04JSDRhF2ZphyllTfLV8ninEIXlks5rpCwdgaJpZM4MU5pu> .

[mythmon]

Perhaps obvious, but I would prefer a system that is robust to client clock.

This technique assumes that the client's clock is at least monotonic and increasing at about 1 second per second. We don't have any way to check the time more than that in a way that I'm happy with, for now. We'll use UTC to deal with time zones. The timestamps we use to test filter expressions come from the server, but that particular query happens to be one of the most expensive ones we have, so we definitely can't increase its load by 50x overnight.

Every startup (NECESARY TO handles experiment tagging for ongoing At least once per day for ongoing sessions.

I can't parse this paragraph. Did something go wrong with a copy/paste?

Derail: Is there some etag/caching magic to allow more often than once per day cheaply?

We already cache things pretty heavily, so there isn't much of an easy solution here. We are looking at some more things to potentially improve performance in the future (mostly things that require the system add-on to be finished). Once we have this interval system in the wild, we can definitely experiment with running more than once a day.

[Osmose]

I propose that we use this opportunity to decouple our execution from the browser start. Instead I suggest that we keep a record of the last time we have run. If it has been more than 24 hours since the last run, run again. We would check at browser start up, and then again periodically.

What are the benefits of making this change?

This technique assumes that the client's clock is at least monotonic and increasing at about 1 second per second. We don't have any way to check the time more than that in a way that I'm happy with, for now. We'll use UTC to deal with time zones. The timestamps we use to test filter expressions come from the server, but that particular query happens to be one of the most expensive ones we have, so we definitely can't increase its load by 50x overnight.

I've been told by several different people over the years that client clocks are broken in very odd ways for many users. Does the app updater, for example, rely on the client clock, or does it get around this issue somehow?

If we can't get confirmation that other high-priority update mechanisms consider the client clock reliable, I don't think it's wise for us to make this assumption.

Every startup (NECESARY TO handles experiment tagging for ongoing At least once per day for ongoing sessions.

I can't parse this paragraph. Did something go wrong with a copy/paste?

I believe Gregg is pointing out that he thinks we need to execute recipes on startup to make sure we tag in-progress experiments in Telemetry, as the Telemetry API for marking a user as active in an experiment doesn't retain state between sessions and needs to be called on startup.

But, I think we're okay here, because the add-on will be keeping tracking of this without needing to run recipes. On startup, the add-on will, independently of running recipes, inform Telemetry of active experiments, and then the next time we execute recipes, will inform Telemetry if any of the experiments were deactivated.

[mythmon]

What are the benefits of making this change?

Client start up is unpredictable and out of our control. It means we have a different patterns of execution for the "every 24 hours" case and the startup case. The execution model I've proposed here should be more predictable and means we only have one way of triggering execution, instead of several.

If we can't get confirmation that other high-priority update mechanisms consider the client clock reliable, I don't think it's wise for us to make this assumption.

I claim that add-on manager (at least) considers the clock to be reliable enough for updates.

• The add-on manager registers a timer callback with the nsUpdateManagerTimer.
• The IDL function that references is here.
• The actual JS code executed is in nsUpdateManagerTimer.jsm.

Tracing that maze of fun times leads me to believe that the add-on manager, via the UpdateManagerTimer, trusts the system clock at least enough to do things about every 86400 seconds.

[mythmon]

PS: I used the add-on manager as an example because it is the one I was able to find code for, with @rhelmer's help. I haven't found out how the many app updater works yet.

[Osmose]

Tracing that maze of fun times leads me to believe that the add-on manager, via the UpdateManagerTimer, trusts the system clock at least enough to do things about every 86400 seconds.

[rhelmer]

@Osmose lulz. This is why we also set timers to run shortly after startup such as https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2946-2950

The problem for hot-reloadable code like add-ons is that there isn't a great way to unregister right now - I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1350471 for that.

There is also a simple Timer module that implements set{Interval,Timeout} and also clear{Interval,Timeout} in https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/toolkit/modules/Timer.jsm which is built on nsITimer.

The advantage to the former is that it has some safeguards and does extra work for you like persisting the timer across sessions.

[rhelmer]

BTW I put up a patch on bug 1350471 - assuming I can land and uplift that in short order, you'd be able to set a timer like:

let SO_MANY_SECONDS = 1234;
let timerManager = Cc["@mozilla.org/updates/timer-manager;1"].
                   getService(Ci.nsIUpdateTimerManager);
timerManager.registerTimer("my-special-timer", () => {
  this.doVerySpecialThings();
}, SO_MANY_SECONDS);

You can also lazy-load the service if you want, with XPCOMUtils.defineLazyServiceGetter.

When your add-on is shutdown or disabled, you can unregister it (you can either get the service again or keep the timerManager reference, either way):

timerManager.unregisterTimer("my-special-timer");

Then if the user shuts down before SO_MANY_SECONDS, the timer will continue where it left off when Firefox starts up again.

[rhelmer]

Sorry I didn't really address the main point @Osmose was making above - we do need to trust the system clock, to a point. It doesn't matter if the date is wrong or the time is slow/fast, but we do expect that the time overall goes forward.

As mentioned above, the nsIUpdateTimerManager stores the last update time (in a pref like app.update.lastUpdateTime.${timerID}), so it isn't simply reset from 0 when Firefox is restarted.

I think it's true that if a user's clock is wildly fluctuating or keeps resetting to an old date etc. then it's certainly possible this won't work as expected, but they probably have other problems if this is happening.