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

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

This is fairly easy to fix, but it might require some tedium.  On the other hand,

http://mxr.mozilla.org/mozilla-central/search?string=do_timeout

suggests a careful search-and-replace might be able to fix 99.9% of all cases to leave only two to manual fixing.

Comment 2

[John Daniels]

Attachment: Changes eval strings to function calls

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.

Comment 3

[Jeff Walden [:Waldo]]

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 |this._func.call(null)| 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!

Comment 4

[Serge Gautherie (:sgautherie)]

What about comm-central?
http://mxr.mozilla.org/comm-central/search?string=do_timeout%5C%28.*%2C+%5C%22&regexp=on&find=%2Fmailnews%2F
"Found 74 matching lines in 34 files"

Comment 5

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/6c577d1d9a67

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

Comment 6

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/0a70f1501c30

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?

Comment 7

[John Daniels]

Yeah, I'll work on that.

Comment 8

[John Daniels]

Attachment: Patch to change do_timeout to use function parameter in comm-central

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:
http://mxr.mozilla.org/comm-central/search?string=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?

Comment 9

[Jeff Walden [:Waldo]]

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 "+testFn.name+", current status is '+gCurrStatus);");
>+    do_timeout(10000, function() {
>+        if(gCurTestNum == test)
>+	  do_throw("Notifications not received in 10000 ms for operation " + testFn.name +
>+            ", 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.

Comment 10

[Jeff Walden [:Waldo]]

(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.  :-)

Comment 11

[John Daniels]

Attachment: One more change and minor fixes.

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 http://mxr.mozilla.org/comm-central/source/mailnews/compose/test/unit/test_sendMessageLater3.js#100 , 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.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 418283 [details] [diff] [review]
One more change and minor fixes.

Landing momentarily...

Comment 13

[Jeff Walden [:Waldo]]

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...

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Permit (but don't require) function callbacks, 192 patch

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...

Comment 15

[Jeff Walden [:Waldo]]

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:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7f7565e06255

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.

Comment 16

[Jeff Walden [:Waldo]]

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:

http://hg.mozilla.org/mozilla-central/rev/8e0e5ab8951f
http://hg.mozilla.org/comm-central/rev/07b1d024d903

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.

Comment 17

[Jeff Walden [:Waldo]]

...and that was it, all fixed.  Thanks for the patches and work here, John!

I also updated the do_timeout documentation at this location:

https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests