Bug 46936 – Make DOM Mutation Events Asynchronous

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

DOM mutation events are sources of many security holes and crashes in HTML editing.  I propose to make them asynchronous and in particular delay all mutation events triggered by mutations inside a EditCommand until the completion of the command to eliminate the chance of JavaScript modifying DOM in a way editing code does not expect.  There are two benefits to this approach:
* Robust Security - Many crashes in HTML editing will be eliminated
* Performance Gain - Multiple mutation events can be collapsed into one if parent nodes of the subtree being modified are same

While there is a fear that this might break the existing websites, I recently discovered that Microsoft Internet Explorer 9 Beta fires DOM mutation events asynchronously.  Given the market share of the Internet Explorer, it seems clear to me that popular websites will eventually (if not immediately) support asynchronous DOM mutation events.

[James Robinson]

From a standards point of view, the latest editor's draft of DOM events level 3 states that mutation events are optional and discouraged.  I think we're completely justified in changing the behavior to be saner.

[Ryosuke Niwa]

Created attachment 74066 [details]
work in progress

Here's my work-in-progress patch.  I talke with Ojan, and we decided to queue up events in page rather than in each document because it's common for developers to listen to events across frame boundaries, and we'd like to preserve the order of events in such cases.

Any feedback / suggestions?

[Ryosuke Niwa]

Our current plan is to have Chrome's QA team test WebKit/Chromium with this patch, and see if things break.  We'll also make use of Google Code Search and look for ECMAScript code that listens to DOM mutation events, and test on those as well.

[Mihai Parparita]

I've been working on making scroll events async (see patch up at 45631), and as part of that introducing an EventQueue class (attached to Document, since it already had some async event functionality that I refactored) for use in async events. When that ends up landing, you should be abel to use that instead of needing to add Page::appendAsyncEvent

[Ryosuke Niwa]

(In reply to comment #4)
> I've been working on making scroll events async (see patch up at 45631), and as part of that introducing an EventQueue class (attached to Document, since it already had some async event functionality that I refactored) for use in async events. When that ends up landing, you should be abel to use that instead of needing to add Page::appendAsyncEvent

The problem with queueing per document is that the ordering of DOM mutation events from different frames will change when events are interleaved with each other.  We should investigate whether this will affect a lot of websites or not though.

[Darin Adler]

A single global queue is probably fine. There’s no reason it has to be per-page or per-document.

[Ryosuke Niwa]

I've been hitting an assertion in JSEventListener.h:

    inline JSC::JSObject* JSEventListener::jsFunction(ScriptExecutionContext* scriptExecutionContext) const
    {
        if (!m_jsFunction)
            m_jsFunction = initializeJSFunction(scriptExecutionContext);

        // Verify that we have a valid wrapper protecting our function from
        // garbage collection.
>       ASSERT(m_wrapper || !m_jsFunction);

I think the problem is that I'm calling the event listener after the page is unloaded.  Is there a way I can figure out if the document has been unloaded?

[Darin Adler]

(In reply to comment #7)
> Is there a way I can figure out if the document has been unloaded?

There are multiple possible meanings of “unloaded” and stages to the unloading process, which makes it hard to get right. A better question is exactly at what point is it not a good idea to deliver the events.

One issue is that a document could be in the back/forward cache.

A document that’s no longer loaded will have a Frame* of 0, but I’m not sure that this happens at the exact right time for your purposes.

[Alexey Proskuryakov]

Mutation event should probably work in frameless documents.

[Ryosuke Niwa]

Created attachment 74172 [details]
alt. work in progress

Here's an alternative approach where I queue events up in the document.  Per discussion with Alexey on IRC, queuing events in the page requires keeping the JS wrapper object for Document alive even after the document is unloaded.  I've thought about it for a while but it's just odd to keep the document around after the document is detached.  I also considered firing all pending events when we're detaching the document but that sounds arbitrary too.

Furthermore, async event doesn't need to preserve the order with respect to other events according to http://www.w3.org/TR/DOM-Level-3-Events/#sync-async.  If we're really making DOM mutation events async, then we shouldn't preserve the order because that'll add another constraint specific to DOM mutation events on the future browser implementations.  If preserving order is so important, then we should probably implement Maciej's scoping object idea rather than making the event async.

[Ryosuke Niwa]

I talked with Darin@Google, and we concluded that we should try Maciej's scoping object idea.  With scoping object, we don't have to deal with document going out of scope because we're only delaying the event firing inside WebCore.  Also, this approach will keep DOM mutation events look like synchronous to JavaScript and might be better from compatibility standpoint as well.

I'll try & see what I can do with this approach in the next couple of weeks.

[Ryosuke Niwa]

Created attachment 75079 [details]
scoped events approach

[Ryosuke Niwa]

E(In reply to comment #12)
> Created an attachment (id=75079) [details]
> scoped events approach

Many of ScopedEventQueue' members functions are inline because I tried to minimize the runtime cost.  EventQueueScope, in fact, shouldn't add runtime cost other than incrementing / decrementing the counter since the instantiation of the event queue happens only once.

I hate the names ScopedEventQueue and EventQueueScope but I couldn't come up with anything better so far.

[Alexey Proskuryakov]

+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY

Looks like a wrong copyright header.

-    if (event.type == "DOMNodeInserted" && event.target.nodeName == "BR")
+    if (event.type == "DOMNodeInserted" && event.target.nodeType == 3)

It's surprising if this makes a difference.

[Ryosuke Niwa]

(In reply to comment #14)
> -    if (event.type == "DOMNodeInserted" && event.target.nodeName == "BR")
> +    if (event.type == "DOMNodeInserted" && event.target.nodeType == 3)
> 
> It's surprising if this makes a difference.

The event for BR is never observable because BR is attached and detached within a single edit command, and the event is fired from the detached node.

Per discussion with Alexey on IRC:
I decided not to make DOMFocusIn/DOMFocusOut async for now because:
1. I wanted to know what Apple folks think about those events since it has radar bug associated with it.
2. They are UI events and therefore has associated underlying events.  It'll be weird to hold back events that have underlying events because those underlying events will be fired synchronously.  We could queue them only if they didn't have underlying events but that seems to unnecessarily complicate the problem.

[Ojan Vafai]

Comment on attachment 75079 [details]
scoped events approach

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

> WebCore/ChangeLog:15
> +        All DOM mutation events except DOMFocusIn and DOMFocusOut are treated as scoped events,

Nit: DOMFocusIn/DOMFocusOut are synchronous, but they're not mutation events.

> WebCore/dom/Node.cpp:2556
> +    event->setTarget(eventTargetRespectingSVGTargetRules(this));

Nit: a comment explaining why you need to do this would be good.

> WebCore/dom/ScopedEventQueue.h:89
> +inline void ScopedEventQueue::enqueueEvent(PassRefPtr<Event> event)
> +{
> +    if (m_scopingLevel)
> +        m_queuedEvents.append(event);
> +    else
> +        dispatchEvent(event);
> +}
> +
> +inline ScopedEventQueue* ScopedEventQueue::instance()
> +{
> +    if (!s_instance)
> +        initialize();
> +
> +    return s_instance;
> +}
> +
> +inline void ScopedEventQueue::incrementScopingLevel()
> +{
> +    m_scopingLevel++;
> +}
> +
> +inline void ScopedEventQueue::decrementScopingLevel()
> +{
> +    ASSERT(m_scopingLevel);
> +    m_scopingLevel--;
> +    if (!m_scopingLevel)
> +        dispatchAllEvents();
> +}

I'm very skeptical that inlining these provides any meaningful performance improvement.

> LayoutTests/fast/events/mutation/execCommands.html:96
> +        window.getSelection().selectAllChildren(test, 0);

nit: selectAllChildren only takes 1 argument.

[Ryosuke Niwa]

Ojan and I tried to add an assert in dispatchGenericEvent that the scoping is always 0 but this attempt failed.

Besides the fact we need to make DOMFocusIn, DOMFocusOut, focusin, focusout scoped (i.e. all UI events), there are cases where editing commands moves iframe and script elements and this results in a whole bunch of synchronous events being fired.  In short, we can't really make all editing events safe.

When a synchronous event fires while RAII objects are still alive, we can do either:
1. Keep scoped events alone.
2. Fire all queued (scoped) events because delaying the events won't do any good.

For now, I'm inclined to take option 1 because IE9 makes DOM mutation events completely asynchronous.  But we might need to take option 2 when we make other events such as focusin/focusout scoped.

[Ojan Vafai]

(In reply to comment #17)
> When a synchronous event fires while RAII objects are still alive, we can do either:
> 1. Keep scoped events alone.
> 2. Fire all queued (scoped) events because delaying the events won't do any good.
> 
> For now, I'm inclined to take option 1 because IE9 makes DOM mutation events completely asynchronous.  But we might need to take option 2 when we make other events such as focusin/focusout scoped.

We should try scoping the focus/blur/etc events. They happen after the focus has moved and are not cancelable, so they should be scopeable.

[Ryosuke Niwa]

Created attachment 75584 [details]
Make DOMFocusIn, DOMFocousOut, focusin, and focusout scoped as well

[Ojan Vafai]

Comment on attachment 75584 [details]
Make DOMFocusIn, DOMFocousOut, focusin, and focusout scoped as well

Please remove the inlining. Otherwise, looks good.

[Ryosuke Niwa]

Committed r73690: <http://trac.webkit.org/changeset/73690>

[David Kilzer (:ddkilzer)]

(In reply to comment #21)
> Committed r73690: <http://trac.webkit.org/changeset/73690>

For future reference, this fix was originally committed as r73684, but rolled out in r73689 for Bug 50801 because the ChangeLog didn't have a bug number in it.

<http://trac.webkit.org/changeset/73684>
<http://trac.webkit.org/changeset/73689>