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.
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.
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?
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.
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
(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.
A single global queue is probably fine. There’s no reason it has to be per-page or per-document.
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?
(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.
Mutation event should probably work in frameless documents.
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.
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.
Created attachment 75079 [details] scoped events approach
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.
+ * 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.
(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.
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.
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.
(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.
Created attachment 75584 [details] Make DOMFocusIn, DOMFocousOut, focusin, and focusout scoped as well
Comment on attachment 75584 [details] Make DOMFocusIn, DOMFocousOut, focusin, and focusout scoped as well Please remove the inlining. Otherwise, looks good.
Committed r73690: <http://trac.webkit.org/changeset/73690>
(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>