Closed Bug 1405833 Opened 3 years ago Closed 3 years ago

Fix or remove improper CommonUtils.namedTimer usage from engines.js

Categories

(Firefox :: Sync, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

Attachments

(1 file)

CommonUtils.namedTimer won't change the callback after you bind it initially, unless you delete the timer[0], but we use it here[1] and here[2] as if it will (e.g, using variables scoped locally to the function).

This means, unless I'm missing something (And I really hope I am), that we won't write the correct values for toFetch/previousFailed after the first time. This is the only place in sync that we use it like this, AFAICT.

Arguably we shouldn't use it at all, as it's crufty, error prone, and not promise-aware, but barring that, just making sure we use it correctly would probably be good enough.

[0]: http://searchfox.org/mozilla-central/source/services/common/utils.js#163-166
[1]: http://searchfox.org/mozilla-central/source/services/sync/modules/engines.js#885-898
[2]: http://searchfox.org/mozilla-central/source/services/sync/modules/engines.js#918-925
(In reply to Thom Chiovoloni [:tcsc] from comment #0)
> 
> This means, unless I'm missing something (And I really hope I am), that we
> won't write the correct values for toFetch/previousFailed after the first
> time.
> 

I was missing something. It does clear itself by calling timer.clear which it sets up earlier. Still there's a chance for data loss due to using it like this, if we write to it before it finishes saving, we'll drop the 2nd write.
Assignee: nobody → tchiovoloni
Note that the right thing to do here is probably to use JSONFile (or DeferredSave?) for toFetch/previousFailed, but actually replacing those would break a lot of tests, and they seem substantially harder to mock than Utils.jsonSave and friends... 
LMK if you'd prefer that be the approach we take though

(If so, it's probably worth investigating what other users of those APIs do wrt tests, it seems plausible that we don't actually need to mock them to test what we want).
Comment on attachment 8915352 [details]
Bug 1405833 - Ensure SyncEngine uses CommonUtils.namedTimer properly.

I'm starting to run out of time for things I want to get done before PTO, so Kit, would you mind taking this?
Attachment #8915352 - Flags: review?(markh) → review?(kit)
Comment on attachment 8915352 [details]
Bug 1405833 - Ensure SyncEngine uses CommonUtils.namedTimer properly.

https://reviewboard.mozilla.org/r/186546/#review192382

I agree that the right long-term fix is to use `JSONFile`, which will correctly set up the shutdown blocker for us, and handle the timer logistics. That said, your patch is still an improvement, so I'm inclined to land this as-is and convert everything over in a follow-up.

::: services/sync/modules/engines.js:893
(Diff revision 1)
>        return;
>      }
>      this._toFetch = val;
>      CommonUtils.namedTimer(function() {
>        try {
> -        Async.promiseSpinningly(Utils.jsonSave("toFetch/" + this.name, this, val));
> +        Async.promiseSpinningly(Utils.jsonSave("toFetch/" + this.name, this, this._toFetch));

It's not clear to me why this uses `promiseSpinningly`, while the timer below kicks off the save in the background...but I don't think it's worth changing in this patch.
Attachment #8915352 - Flags: review?(kit) → review+
See Also: → 1366067
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf80be6c2210
Ensure SyncEngine uses CommonUtils.namedTimer properly. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/bf80be6c2210
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.