Last Comment Bug 511328 - TM: make Math.random threadlocal
: TM: make Math.random threadlocal
Status: RESOLVED FIXED
fixed-in-tracemonkey [qa-examined-191...
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Andreas Gal :gal
:
Mentors:
Depends on:
Blocks: CVE-2008-5913
  Show dependency treegraph
 
Reported: 2009-08-18 19:38 PDT by Andreas Gal :gal
Modified: 2010-05-11 06:07 PDT (History)
9 users (show)
sayrer: wanted1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed
.10+
.10-fixed


Attachments
patch (7.92 KB, patch)
2009-08-18 19:40 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (7.92 KB, patch)
2009-08-18 19:51 PDT, Andreas Gal :gal
shaver: review+
Details | Diff | Splinter Review
missing upcast to int64 before the << 27 (7.76 KB, patch)
2009-08-18 20:12 PDT, Andreas Gal :gal
shaver: review+
Details | Diff | Splinter Review
cleanup (7.75 KB, patch)
2009-08-18 20:17 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
more cleanup (7.74 KB, patch)
2009-08-18 20:19 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (7.74 KB, patch)
2009-08-18 20:23 PDT, Andreas Gal :gal
jwalden+bmo: review+
christian: approval1.9.1.10+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2009-08-18 19:38:21 PDT

    
Comment 1 Andreas Gal :gal 2009-08-18 19:40:49 PDT
Created attachment 395241 [details] [diff] [review]
patch
Comment 2 Andreas Gal :gal 2009-08-18 19:50:17 PDT
  string:              1.009x as fast    324.9ms +/- 0.2%   322.0ms +/- 0.2%     significant
    base64:            1.039x as fast     20.9ms +/- 0.6%    20.1ms +/- 0.6%     significant
    fasta:             *1.023x as slow*   67.8ms +/- 0.2%    69.3ms +/- 0.2%     significant
    tagcloud:          1.008x as fast    100.2ms +/- 0.2%    99.5ms +/- 0.3%     significant
    unpack-code:       ??                 98.9ms +/- 0.4%    99.4ms +/- 0.2%     not conclusive: might be *1.005x as slow*
    validate-input:    1.098x as fast     37.1ms +/- 0.6%    33.8ms +/- 0.8%     significant
Comment 3 Andreas Gal :gal 2009-08-18 19:51:57 PDT
Created attachment 395243 [details] [diff] [review]
patch
Comment 4 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-08-18 20:03:10 PDT
Comment on attachment 395243 [details] [diff] [review]
patch

I sure won't miss that JSLL_* stuff, and it looks like a faithful translation.

r=shaver
Comment 5 Andreas Gal :gal 2009-08-18 20:12:52 PDT
Created attachment 395247 [details] [diff] [review]
 missing upcast to int64 before the << 27
Comment 6 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-08-18 20:16:44 PDT
Comment on attachment 395247 [details] [diff] [review]
 missing upcast to int64 before the << 27

Better! Can we just make random_next return uint64, though?  I'm not the guy to do the analysis of those effects, to be sure...
Comment 7 Andreas Gal :gal 2009-08-18 20:17:39 PDT
Created attachment 395250 [details] [diff] [review]
cleanup
Comment 8 Andreas Gal :gal 2009-08-18 20:19:26 PDT
Created attachment 395251 [details] [diff] [review]
more cleanup
Comment 9 Andreas Gal :gal 2009-08-18 20:23:27 PDT
Created attachment 395252 [details] [diff] [review]
patch
Comment 10 Andreas Gal :gal 2009-08-18 20:24:48 PDT
Comment on attachment 395252 [details] [diff] [review]
patch

Waldo, since this is crypto-sensitive, would you mind taking a look too?
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2009-08-19 11:02:03 PDT
Comment on attachment 395252 [details] [diff] [review]
patch

Throughout: use the u?int64_t typenames rather than u?int64.

>diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h

>+    /* Random number generator state, used by jsmath.c. */

cpp


>diff --git a/js/src/jsmath.cpp b/js/src/jsmath.cpp

I do so love these random_* methods, it's like we have to poor-man's namespace because we can't have a |class RNG|!  Doesn't need to be fixed now, maybe a followup...
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2009-08-19 11:06:44 PDT
(In reply to comment #10)
> Waldo, since this is crypto-sensitive, would you mind taking a look too?

More than happy to take a look, but this seems a bit of a non sequitur...
Comment 13 Andreas Gal :gal 2009-08-19 15:25:14 PDT
http://hg.mozilla.org/tracemonkey/rev/0307226048c8
Comment 16 Reed Loden [:reed] (use needinfo?) 2010-04-12 22:34:13 PDT
Comment on attachment 395252 [details] [diff] [review]
patch

Need this on 1.9.1 in order to land bug 475585 there.
Comment 17 christian 2010-04-13 14:24:25 PDT
Comment on attachment 395252 [details] [diff] [review]
patch

a=LegNeato for 1.9.1.10
Comment 18 Reed Loden [:reed] (use needinfo?) 2010-04-13 14:30:24 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f3a5a5da42f3
Comment 19 Al Billings [:abillings] 2010-04-27 12:02:56 PDT
Is there a repro case at all for this for testing? It doesn't look like it.
Comment 20 Mike Hommey [:glandium] 2010-05-03 23:25:21 PDT
I don't know yet if third party software use these APIs, but this patch breaks ABI.
Comment 21 Andreas Gal :gal 2010-05-03 23:55:25 PDT
Which ABI?
Comment 22 Mike Hommey [:glandium] 2010-05-04 00:03:00 PDT
libmozjs.so's ABI
Comment 23 Mike Hommey [:glandium] 2010-05-04 00:15:51 PDT
To be more precise, this breaks js_LeaveTrace() and js_CanLeaveTrace(). I'm currently going through the third party software using libmozjs in debian to see if any are using these functions apart from mozilla code itself (liveconnect and xpconnect do)
Comment 24 Mike Hommey [:glandium] 2010-05-04 00:42:47 PDT
Good news, none of the third party software using libmozjs in debian use these functions.
Comment 25 Andreas Gal :gal 2010-05-04 01:43:35 PDT
We didn't really intend those functions to be part of the ABI. In particular all of jscntxt.h is not part of the ABI. Its not even part of the friends ABI we use internally in xpconnect. If you need those to functions, we should talk about that and add a proper public API (I suspect you shouldn't need them though, whatever you do we should make sure it either works on trace, or we fall of trace inside the engine before embedding code executes).
Comment 26 Mike Hommey [:glandium] 2010-05-04 01:52:44 PDT
js_LeaveTrace *is* used in xpconnect, liveconnect, and nsJSEnvironment. Anyways, nothing else uses it, so it is okay.
Comment 27 Jeff Walden [:Waldo] (remove +bmo to email) 2010-05-04 10:03:12 PDT
Those are all incestuous and not good style, diving under the covers, so to speak.  They use this only for speed reasons, and we'd like to fix them not to do so at some point.  But ultimately, we have more important things to worry about than absolute API purity.  JS and the browser are intertwined, it's a fact of life not worth getting incredibly upset about.
Comment 28 Mike Hommey [:glandium] 2010-05-04 10:06:37 PDT
(In reply to comment #27)
> JS and the browser are intertwined, it's a
> fact of life not worth getting incredibly upset about.

"Incredibly upset" ? Haven't you read comment 26 and comment 24 ?
Comment 29 Brendan Eich [:brendan] 2010-05-04 10:12:45 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > JS and the browser are intertwined, it's a
> > fact of life not worth getting incredibly upset about.
> 
> "Incredibly upset" ? Haven't you read comment 26 and comment 24 ?

Waldo certainly exaggerated. Indeed he may be more upset (I was) to hear of ABI including js_LeaveTrace.

Anyway, with old C code one tends to find growth of the "circle of trust" in terms of Mozilla modules growing over time. We will prune back. But in no case are these js_* functions for general consumption.

/be
Comment 30 Jeff Walden [:Waldo] (remove +bmo to email) 2010-05-04 10:15:49 PDT
I have.  My interpretation of comment 26 was that it was implying those were evil witches in gingerbread houses, likely to lead little children astray, and thus something worth prioritizing for fixing.  :-)  I was simply saying it's a necessary risk for now, and that it's a fairly small one not deserving of too much concern in any case.
Comment 31 Mike Hommey [:glandium] 2010-05-04 10:24:44 PDT
> But in no case are these js_* functions for general consumption. 

But the headers as they are shipped at the moment don't help very much, especially because most mix things that are exported and things that aren't, and also because most aren't supposed to be public at all. In other words, I'd very much like more feedback on bug 554088.

Enough from me in this bug. Sorry for the noise.

Note You need to log in before you can comment on or make changes to this bug.