Bug 73492 – [MutationObservers] V8LazyEventHandler breaks microtask delivery semantics

• Component: DOM
• Status: RESOLVED FIXED

[Rafael Weinstein]

It implements the target->form->document scoping for inline event handler names by executing script (in prepareListenerObject).

As a result, any pending mutation observers are delivered before the inline event handler runs.

[Adam Klein]

The tricky part of this is testing, since events dispatched synchronously from script in a layout test won't trigger the incorrect behavior (since the recursion counter will already be 1 when the handler's script is evaluated). So it's only testable from outside the browser (e.g., in a Chromium UI test).

[Adam Klein]

Created attachment 118446 [details]
Patch

[Adam Klein]

Created attachment 118454 [details]
Patch

[Adam Barth]

Comment on attachment 118454 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Test: ManualTests/mutation-observers-inline-event-handler.html

Why can't we create an automated test for this?  We need the event handler to be triggered without other JavaScript on the stack?

[WebKit Review Bot]

Comment on attachment 118454 [details]
Patch

Attachment 118454 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10780363

New failing tests:
editing/style/iframe-onload-crash-mac.html

[Adam Klein]

(In reply to comment #4)
> (From update of attachment 118454 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118454&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Test: ManualTests/mutation-observers-inline-event-handler.html
> 
> Why can't we create an automated test for this?  We need the event handler to be triggered without other JavaScript on the stack?

We need to meet both of the following requirements:

1. DOM mutation that happens without JS on the stack
2. An inline event handler that runs in the same task as the DOM mutation in (1)

I suspect the manual test I've added here could be adapted to run as a Chromium ui_test, but I've not written one before. I don't think there's any way to test this code in a pure-WebKit automated test.

[Adam Barth]

Comment on attachment 118454 [details]
Patch

Ok

[Adam Klein]

(In reply to comment #5)
> (From update of attachment 118454 [details])
> Attachment 118454 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10780363
> 
> New failing tests:
> editing/style/iframe-onload-crash-mac.html

Looks like this is a real bug, I can reproduce locally (and it goes away if I comment out the V8RecursionScope).  I know next-to-nothing about the loader, do you happen to know what the issue here is?  The stack is deep and confusing.

[Adam Barth]

This is an editing test.  Maybe rniwa has some insight?

[Adam Klein]

Better data is to be found in the stderr output, seems we're hitting the maximum call stack size:

Extension or internal compilation error at line 14.
CONSOLE MESSAGE: line 33: Uncaught RangeError: Maximum call stack size exceeded.

#
# Fatal error in ../../v8/src/handles-inl.h, line 64
# CHECK(location_ != __null) failed
#

Attempt to print stack while printing stack (double fault)
If you are lucky you may find a partial stack dump on stdout.

The JS stack looks like this:
    1: /* anonymous */ [file:///..../LayoutTests/editing/style/iframe-onload-crash-mac.html:33] (this=0x33209525f599 <an HTMLIFrameElement>#0#,evt=0x2e9faae479e9 <an Event>#1#)
    2: onload [file:///.../LayoutTests/editing/style/iframe-onload-crash-mac.html:38] (this=0x33209525f599 <an HTMLIFrameElement>#0#,evt=0x2e9faae479e9 <an Event>#1#)
    6: /* anonymous */ [file:///.../LayoutTests/editing/style/iframe-onload-crash-mac.html:34] (this=0x33209525f599 <an HTMLIFrameElement>#0#,evt=0x2e9faafe4631 <an Event>#2#)
    7: onload [file:///.../LayoutTests/editing/style/iframe-onload-crash-mac.html:38] (this=0x33209525f599 <an HTMLIFrameElement>#0#,evt=0x2e9faafe4631 <an Event>#2#)
...

and so on (back and forth between onload and anonymous).

[Adam Klein]

Created attachment 118467 [details]
Patch

[Adam Klein]

Created attachment 118489 [details]
Patch now with layout test

[WebKit Review Bot]

Comment on attachment 118489 [details]
Patch now with layout test

Clearing flags on attachment: 118489

Committed r102424: <http://trac.webkit.org/changeset/102424>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

Other port DRT's support eventSender.scheduleAsynchronousKeyDown ?

[Adam Klein]

(In reply to comment #15)
> Other port DRT's support eventSender.scheduleAsynchronousKeyDown ?

Not yet, but these tests are currently skipped on non-Chromium ports (the feature doesn't work at all on JSC-based ports). Should be relatively easy to implement if/when other ports want to start supporting MutationObservers.

[noel gordon]

OK. Thanks.