Bug 136051 - EventSender dispatches should be per-Document
Summary: EventSender dispatches should be per-Document
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 137397
Blocks: 137090
  Show dependency treegraph
 
Reported: 2014-08-18 16:07 PDT by Brian Burg
Modified: 2019-02-06 09:19 PST (History)
13 users (show)

See Also:


Attachments
Proposed Fix (31.36 KB, patch)
2014-08-18 18:56 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
WIP (31.42 KB, patch)
2014-09-26 13:42 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (40.54 KB, patch)
2014-09-26 15:04 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (+win build fix) (40.98 KB, patch)
2014-10-01 16:04 PDT, Brian Burg
kling: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-08-18 16:07:57 PDT
EventSender causes JS code to execute nondeterministically. It uses a Timer to asynchronously dispatch the same event to multiple elements of the same type on a future run loop. It also force-dispatches the event when the document closes.

Currently, each (event name, element) pair gets its own static EventSender. This is quite nondeterministic because unrelated pages (say, the Inspector or another tab) share the same static EventSenders and can force-dispatch each others' pending events on implicitClose.

I propose making EventSender instances per-Document. Then we can use ReplayableTimer to iron out the scheduling.
Comment 1 Brian Burg 2014-08-18 18:56:18 PDT
Created attachment 236793 [details]
Proposed Fix

I uploaded a patch to fix this. Looking forward to any feedback. I should probably write a replay test for this bug before landing, but the non-replay portions should be covered.
Comment 2 Brian Burg 2014-09-24 16:56:39 PDT
Splitting replay parts from the refactoring.
Comment 3 Brian Burg 2014-09-26 13:42:12 PDT
Created attachment 238735 [details]
WIP
Comment 4 Brian Burg 2014-09-26 15:04:16 PDT
Created attachment 238741 [details]
Patch
Comment 5 Brian Burg 2014-09-26 15:09:18 PDT
Comment on attachment 238741 [details]
Patch

To remove a source of nondeterminism for web replay, this patch makes EventSenders associated with the Document, without trying to overly optimize or re-architect how EventSender works. I am not equipped to make useful (ie., data driven) performance optimizations on this code.
Comment 6 Brian Burg 2014-10-01 16:04:51 PDT
Created attachment 239062 [details]
Proposed Fix (+win build fix)
Comment 7 Andreas Kling 2014-10-01 16:09:40 PDT
Comment on attachment 239062 [details]
Proposed Fix (+win build fix)

r=me, thanks for taking care of this.
Comment 8 WebKit Commit Bot 2014-10-01 16:57:01 PDT
Comment on attachment 239062 [details]
Proposed Fix (+win build fix)

Rejecting attachment 239062 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 239062, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/5927535393112064
Comment 9 Chris Dumez 2014-10-01 16:58:25 PDT
Comment on attachment 239062 [details]
Proposed Fix (+win build fix)

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

> Source/WebCore/ChangeLog:5
> +

You dropped the Reviewed By NOBODY line, which is why the commit queue complained about not being able to update that line.
Comment 10 Brian Burg 2014-10-01 17:54:15 PDT
Comment on attachment 239062 [details]
Proposed Fix (+win build fix)

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

>> Source/WebCore/ChangeLog:5
>> +
> 
> You dropped the Reviewed By NOBODY line, which is why the commit queue complained about not being able to update that line.

Weird, didn't think I touched that! Will fix and land by `webkit-patch land` later tonight.
Comment 11 Brian Burg 2014-10-03 12:21:53 PDT
Committed r174270: <http://trac.webkit.org/changeset/174270>

But, needs to be rolled out as there are several crashes on bots.
Comment 12 BJ Burg 2017-07-10 14:06:23 PDT
Closing web replay-related bugs until we resume working on the feature again.
Comment 13 Lucas Forschler 2019-02-06 09:19:06 PST
Mass move bugs into the DOM component.