Closed Bug 525819 Opened 12 years ago Closed 11 years ago

Make do_timeout take a callback function, not a string to eval


(Testing :: XPCShell Harness, defect)

Not set


(Not tracked)



(Reporter: Waldo, Assigned: johnsdaniels)



(Keywords: dev-doc-complete, Whiteboard: [good first bug])


(3 files, 1 obsolete file)

Functions, lambdas, functional programming and all that is cool -- gratuitous dynamic code compilation is potentially insecure in other contexts, inhibits optimizations, and is generally something to discourage (ES5's strict mode starts to de-tooth eval).  There's no reason to require evaluating a string here; calling a callback is every bit as powerful as evaluating a string.
This is fairly easy to fix, but it might require some tedium.  On the other hand,

suggests a careful search-and-replace might be able to fix 99.9% of all cases to leave only two to manual fixing.
Whiteboard: [good first bug]
This should be all the changes necessary to move the do_timeout to using a callback. I'm not too familiar with the javascript testing framework, but all the tests are passing on "make xpcshell-tests". Tell me if I missed anything.
Attachment #414180 - Flags: review?(jwalden+bmo)
Assignee: nobody → johnsdaniels
Comment on attachment 414180 [details] [diff] [review]
Changes eval strings to function calls

Sheesh this is huge; wish we'd done this from the start.  :-\  These are the nits I see:

>diff -r 8ca9facea08d testing/xpcshell/head.js

> function _TimerCallback(expr, timer) {
>-  this._expr = expr;
>+  this._func = expr;
>   // Keep timer alive until it fires
>   _pendingCallbacks.push(timer);
> }
> _TimerCallback.prototype = {
>-  _expr: "",
>+  _func: function(){},

This property, of course, wasn't and isn't necessary (the property in the constructor will always be seen before the other), so I've removed it from my local application of the patch.

>   notify: function(timer) {
>     _pendingCallbacks.splice(_pendingCallbacks.indexOf(timer), 1);
>-    eval(this._expr);
>+    this._func();
>   }

I expect it won't matter for most (all?) tests, but if a timer callback uses |this| as a shortcut for getting the global object this formulation will instead give it the _TimerCallback instance.  I've changed this to || locally to avoid this problem.

I've made these changes locally and pushed it to tryserver; assuming all is green there I'll push to mozilla-central later today.  Thanks!
Attachment #414180 - Flags: review?(jwalden+bmo) → review+

Now to address comm-central, I completely forgot about it.  :-\

That readds support for do_timeout accepting a string argument.  I looked at the number of comm-central files to change, and if the first one I tried to edit is representative they don't use the function continuation style m-c used that made search-and-replace feasible for the vast majority of the fixes.  Given that, it seemed too much work to finish that while keeping the tree colorful, so I decided better to just make them work again than to fix right then.

John, would you be able to get the comm-central tests updated as well, so we can completely flip to functions rather than accepting either?
Yeah, I'll work on that.
This should do it for the comm-central stuff. I factored out one of the anonymous functions to its own named function since it was called four times. Otherwise, I pretty much just closed over anything that had a parameter with an anonymous function. 

I did notice along the way that there is a do_timeout_function:\%28.*&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

I suppose the next step after this patch would be to get rid of that. Also, it might be useful to set up the do_timeout to allow passing in a "this" pointer and arguments like you can with do_timeout_function. Currently, no one uses that feature of do_timeout_function, and when I was righting the patch I just closed over any arguments with a new function.

And did I mention how much I hate the way "this" is done in javascript?
Attachment #414684 - Flags: review?(jwalden+bmo)
Comment on attachment 414684 [details] [diff] [review]
Patch to change do_timeout to use function parameter in comm-central

There are a bunch of places where whitespace around operators is kind of weird and inconsistent (generally there should be a space on each side of a binary operator like +), but it's test code.  Also, the existing tests sometimes didn't have this, so this isn't really a change from status quo.

>diff -r 8c33e59f55dc mailnews/compose/test/unit/test_attachment.js

>@@ -203,18 +203,22 @@ function doTest(test)
> {
>   dump("doTest " + test + "\n");
>   if (test <= gTestArray.length) {
>     gCurTestNum = test;
>     var testFn = gTestArray[test-1];
>     // Set a limit in case the notifications haven't arrived (i.e. a problem)
>-    do_timeout(10000, "if (gCurTestNum == "+test+")               \
>-               do_throw('Notifications not received in 10000 ms for operation "", current status is '+gCurrStatus);");
>+    do_timeout(10000, function() {
>+        if(gCurTestNum == test)
>+	  do_throw("Notifications not received in 10000 ms for operation " + +
>+            ", current statis is " + gCurrStatus);
>+        }
>+      );

Typo: "statis" instead of "status".

I've unfortunately probably sat on this too long for it to apply without being incomplete.  Would you mind fixing the typo above, updating for any new tests that have since been added, and posting a revised version?  I'll be busy tomorrow and Monday and likely unable to respond to bugmail or review requests, but Tuesday or afterward I promise to land it ASAP.
(In reply to comment #8)
> I suppose the next step after this patch would be to get rid of that. Also, it
> might be useful to set up the do_timeout to allow passing in a "this" pointer
> and arguments like you can with do_timeout_function. Currently, no one uses
> that feature of do_timeout_function, and when I was righting the patch I just
> closed over any arguments with a new function.

Sounds good to me.  And yes, |this| is a rather weird beast in JS.  :-)
There was only one more instance that I found where I needed to remove the quotes, although all the tests were passing without it since it was a timer until failure. There's also some rather bizarre use of the do_timeout function at , but I figured it would be best if I left that alone. Otherwise it all seems to be working for me, and I have the mozilla tree source modified so do_timeout can't handle strings.
Attachment #414684 - Attachment is obsolete: true
Attachment #418283 - Flags: review?(jwalden+bmo)
Attachment #414684 - Flags: review?(jwalden+bmo)
Comment on attachment 418283 [details] [diff] [review]
One more change and minor fixes.

Landing momentarily...
Attachment #418283 - Flags: review?(jwalden+bmo) → review+
Hm.  So comm-central is currently working at building against both 1.9.2 and m-c, so we need a 1.9.2 backport that allows callback-as-function.  I don't think it's a good idea to backport all the individual test changes -- there's no reason to take that much churn, or spend time on it -- so just allowing both behaviors in 1.9.2 should be adequate.

I'll whip up the 1.9.2 patch, should be trivial will eliminate the holdup on landing the m-c and c-c changes...
Current m-c also includes defense against pending timers being gc'd, figured I'd port that at the same time to keep the code more similar...

Super-trivial review, Ted -- appreciate it if you could get to it ASAP so it can get 192 approval ASAP so it can land in 192 ASAP so the comm-central patch here doesn't bitrot too heavily in the meantime...
Attachment #418401 - Flags: review?(ted.mielczarek)
Attachment #418401 - Flags: review?(ted.mielczarek) → review+
Landed attachment 418401 [details] [diff] [review] in 192, looks like we're not quite to the point of everything in the world requiring approval to land there:

c-c is open but has two reds right now, so I'm going to hold off until tomorrow morning to land there -- hopefully everything else pending in this bug can go in then with minimal effort.
Hmm...I forgot about it that next morning, then was offline while working the rest of the week or on vacation, so it didn't happen.  Landed the big switch just now in m-c and the c-c change in sync with it:

It's possible intervening changes have happened to invalidate either of these, so this may not be the final word, but I expect any added tweaks to be trivial.
...and that was it, all fixed.  Thanks for the patches and work here, John!

I also updated the do_timeout documentation at this location:
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Duplicate of this bug: 489242
You need to log in before you can comment on or make changes to this bug.