611807 - setTimeout function in Sync.js relies on a JS engine bug to work

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Someone was being very clever here.  The function is:


183 function setTimeout(func, delay) {
184   let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
185   let callback = {
186     notify: function notify() {
187       // This line actually just keeps a reference to timer (prevent GC)
188       timer = null;
189 
190       // Call the function so that "this" is global
191       func();
192     }
193   }
194   timer.initWithCallback(callback, delay, Ci.nsITimer.TYPE_ONE_SHOT);
195 }

Now it's relying on the fact that the write to |timer| will deoptimize |notify| such that it entrains |timer| and keeps it from getting gced.  But |timer| doesn't escape and is write-only, so a good optimizer could first optimize out the timer = null set and then optimize down |notify| to a closure that doesn't entrain timer.  And if we get better at optimizing our closures this code will end up with a random failure depending on gc timing.

The right way to do this, it seems to me, is to store |timer| as a property on |callback| instead of in a local variable.

Comment 2

[Philipp von Weitershausen [:philikon]]

Sync.js's setTimeout function was removed in bug 636402.