Bug 129451

Summary: Web Replay: make calls into FrameLoader::checkLoadComplete() deterministic
Product: WebKit Reporter: BJ Burg <bburg>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED LATER    
Severity: Major CC: andersca, ap, bburg, bunhere, cdumez, commit-queue, darin, ddkilzer, dino, esprehn+autocc, glenn, gyuyoung.kim, japhet, joepeck, kangil.han, kling, macpherson, menard, rakuco, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 136290    
Attachments:
Description Flags
serialization of wip
none
the patch
none
the patch none

Description BJ Burg 2014-02-27 14:29:42 PST
I have implemented Timer workalike that:

- when capturing, saves when it fires into the replay session
- when replaying, doesn't schedule itself, and instead is called by EventLoopInputDispatcher
- when neither capturing or replaying, it acts like a regular timer (by forwarding calls to an inner Timer instance).

The implementation in my branch supports the basic Timer API, but it should also support SuspendableTimer so that it can be used to make DOMTimer deterministic as well.
Comment 1 BJ Burg 2014-03-28 14:41:07 PDT
Created attachment 228085 [details]
serialization of wip
Comment 2 Brian Burg 2014-04-02 17:21:00 PDT
A deterministic timer implementation addresses two major sources of nondeterminism:

1) During replay, timers that could possibly cause DOM events to be fired should fire in the same order to ensure JS is triggered in the right order.

2) During replay, we similarly want DOMTimers to fire in the same order with respect to other timers and other run loops like mouse, keyboard, network.

For the first version of ReplayableTimer I want to support single-shot timers. Later patches will teach ReplayableTimer to do intervals, and later work as a drop-in replacement of SuspendableTimer.

Handling intervals is sufficient for most WebCore-internal timers affecting JS determinism, such as those in FrameLoader, DocumentEventQueue, EventSender, etc.

Handling the SuspendableTimer interface is required for this implementation to be used for capture/replay of DOM timers. This will be a bit more work. An alternative (or stopgap) is to write custom replayable subclasses just for DOMTimer, and then switch off of those once ReplayableTimer learns how to deal with suspend. The "stopgap" approach can be seen here: https://github.com/burg/timelapse/blob/timelapse/Source/WebCore/page/DOMTimer.cpp

I'm going to work on implementing repeatInterval and suspendable functionality, and if it proves to be more than a few days' work, then we can think about pushing on the DOMTimer stopgap first. It's really important to get DOMTimer to be deterministic ASAP, as it blocks any sort of error-checking and is used by every webpage.
Comment 3 Brian Burg 2014-04-03 18:19:31 PDT
Created attachment 228571 [details]
the patch
Comment 4 WebKit Commit Bot 2014-04-03 18:21:56 PDT
Attachment 228571 [details] did not pass style-queue:


ERROR: Source/WebCore/replay/ReplayableTimer.h:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/replay/ReplayableTimer.h:120:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Brian Burg 2014-04-04 12:09:25 PDT
Created attachment 228612 [details]
the patch
Comment 6 WebKit Commit Bot 2014-04-04 12:10:32 PDT
Attachment 228612 [details] did not pass style-queue:


ERROR: Source/WebCore/replay/ReplayableTimer.h:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/replay/ReplayableTimer.h:120:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Timothy Hatcher 2014-04-04 12:37:34 PDT
The patch looks fine to me. I'd like others to take a look.

Why shouldn't / can't all Timers be replayable?
Comment 8 Brian Burg 2014-04-04 15:47:47 PDT
(In reply to comment #7)
> The patch looks fine to me. I'd like others to take a look.
> 
> Why shouldn't / can't all Timers be replayable?

Most of them are irrelevant from a determinism point of view, so they would fill the recording with unnecessary data. The most important ones are those that could transitively dispatch DOM events or otherwise run JS code.

I haven't benchmarked it, but ReplayableTimer is probably a little slower because it has to dig out an input cursor. It does an allocation if the cursor is capturing.
Comment 9 Timothy Hatcher 2014-08-05 11:55:07 PDT
Darin, maybe you can review this for Brian? I don't feel as comfortable reviewing it.
Comment 10 BJ Burg 2015-08-05 11:29:29 PDT
Comment on attachment 228612 [details]
the patch

Clearing r? flag as this patch is bit-rotted.
Comment 11 BJ Burg 2017-07-10 13:59:28 PDT
Closing web replay-related bugs until we resume working on the feature again.