Bug 128633 - Web Replay: capture and replay nondeterminism of Date.now() and Math.random()
Summary: Web Replay: capture and replay nondeterminism of Date.now() and Math.random()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-11 16:25 PST by BJ Burg
Modified: 2014-02-12 19:29 PST (History)
3 users (show)

See Also:


Attachments
patch (15.06 KB, patch)
2014-02-11 17:25 PST, BJ Burg
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2014-02-11 16:25:44 PST
These are the only sources of nondeterminism implemented in JSC that are visible to script (excluding VM bugs, etc).

For Math.random(), the seed given to WeakRandom can be memoized. For Date.now() and new Date(), we have to memoize each call.
Comment 1 BJ Burg 2014-02-11 17:25:03 PST
Created attachment 223920 [details]
patch
Comment 2 Filip Pizlo 2014-02-11 17:41:44 PST
Comment on attachment 223920 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223920&action=review

R=me but please consider encapsulating the #if ENABLE(WEB_REPLAY) inside a clever macro.

> Source/JavaScriptCore/runtime/DateConstructor.cpp:128
> +#if ENABLE(WEB_REPLAY)
> +        value = deterministicCurrentTime(globalObject);
> +#else
>          value = jsCurrentTime();
> +#endif

Can't you have a macro like REPLAY(jsCurrentTime(), deterministicCurrentTime(globalObject))?

The reason why I would like that more is that #if's can often disturb our ability to visualize the nesting of control flow.  The definition of REPLAY would literally be:

#if ENABLE(WEB_REPLAY)
#define REPLAY(a, b) (b)
#else
#define REPLAY(a, b) (a)
#endif

You can call REPLAY() whatever you like...

> Source/JavaScriptCore/runtime/DateConstructor.cpp:214
> +#if ENABLE(WEB_REPLAY)
> +    return JSValue::encode(jsNumber(deterministicCurrentTime(exec->lexicalGlobalObject())));
> +#else
> +    UNUSED_PARAM(exec);
>      return JSValue::encode(jsNumber(jsCurrentTime()));
> +#endif

Another fine example of a place where that REPLAY() macro would simplify things.
Comment 3 BJ Burg 2014-02-12 16:07:45 PST
Comment on attachment 223920 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223920&action=review

>> Source/JavaScriptCore/runtime/DateConstructor.cpp:128
>> +#endif
> 
> Can't you have a macro like REPLAY(jsCurrentTime(), deterministicCurrentTime(globalObject))?
> 
> The reason why I would like that more is that #if's can often disturb our ability to visualize the nesting of control flow.  The definition of REPLAY would literally be:
> 
> #if ENABLE(WEB_REPLAY)
> #define REPLAY(a, b) (b)
> #else
> #define REPLAY(a, b) (a)
> #endif
> 
> You can call REPLAY() whatever you like...

Sure thing, I can add that. It will only help out in cases where the difference is which function to call, but that's fine. (Most replay-oriented hooks add a branch without interleaving the normal code path.)
Comment 4 Joseph Pecoraro 2014-02-12 16:18:50 PST
Comment on attachment 223920 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223920&action=review

> Source/JavaScriptCore/runtime/DateConstructor.cpp:88
> +    if (cursor.isCapturing())
> +        cursor.appendInput<GetCurrentTime>(currentTime);
> +
> +    if (cursor.isReplaying()) {

If a cursor cannot be both capturing and replaying then this can be an "else if" instead of another if. Or the states could move to an enum (Default, Capturing, Replaying).

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:793
> +    if (cursor.isCapturing())
> +        cursor.appendInput<SetRandomSeed>(m_weakRandom.seedUnsafe());
> +
> +    if (cursor.isReplaying()) {

Ditto
Comment 5 BJ Burg 2014-02-12 19:29:26 PST
Committed r164007: <http://trac.webkit.org/changeset/164007>