Bug 136051 – EventSender dispatches should be per-Document

• Component: DOM
• Status: RESOLVED LATER

[Brian Burg]

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.

[Brian Burg]

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.

[Brian Burg]

Splitting replay parts from the refactoring.

[Brian Burg]

Created attachment 238735 [details]
WIP

[Brian Burg]

Created attachment 238741 [details]
Patch

[Brian Burg]

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.

[Brian Burg]

Created attachment 239062 [details]
Proposed Fix (+win build fix)

[Andreas Kling]

Comment on attachment 239062 [details]
Proposed Fix (+win build fix)

r=me, thanks for taking care of this.

[WebKit Commit Bot]

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

[Chris Dumez]

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.

[Brian Burg]

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.

[Brian Burg]

Committed r174270: <http://trac.webkit.org/changeset/174270>

But, needs to be rolled out as there are several crashes on bots.

[Blaze Burg]

Closing web replay-related bugs until we resume working on the feature again.

[Lucas Forschler]

Mass move bugs into the DOM component.