Bug 73492 - [MutationObservers] V8LazyEventHandler breaks microtask delivery semantics
Summary: [MutationObservers] V8LazyEventHandler breaks microtask delivery semantics
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on: 73867
Blocks: 68729
  Show dependency treegraph
 
Reported: 2011-11-30 14:39 PST by Rafael Weinstein
Modified: 2019-05-02 16:25 PDT (History)
11 users (show)

See Also:


Attachments
Patch (1.92 KB, patch)
2011-12-08 12:21 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (3.29 KB, patch)
2011-12-08 12:51 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (3.16 KB, patch)
2011-12-08 14:17 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch now with layout test (7.42 KB, patch)
2011-12-08 16:39 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Weinstein 2011-11-30 14:39:52 PST
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.
Comment 1 Adam Klein 2011-12-08 11:45:38 PST
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).
Comment 2 Adam Klein 2011-12-08 12:21:57 PST
Created attachment 118446 [details]
Patch
Comment 3 Adam Klein 2011-12-08 12:51:39 PST
Created attachment 118454 [details]
Patch
Comment 4 Adam Barth 2011-12-08 12:55:00 PST
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?
Comment 5 WebKit Review Bot 2011-12-08 13:26:08 PST
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
Comment 6 Adam Klein 2011-12-08 13:33:46 PST
(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.
Comment 7 Adam Barth 2011-12-08 13:36:47 PST
Comment on attachment 118454 [details]
Patch

Ok
Comment 8 Adam Klein 2011-12-08 13:42:43 PST
(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.
Comment 9 Adam Barth 2011-12-08 13:44:44 PST
This is an editing test.  Maybe rniwa has some insight?
Comment 10 Adam Klein 2011-12-08 13:53:16 PST
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).
Comment 11 Adam Klein 2011-12-08 14:17:53 PST
Created attachment 118467 [details]
Patch
Comment 12 Adam Klein 2011-12-08 16:39:28 PST
Created attachment 118489 [details]
Patch now with layout test
Comment 13 WebKit Review Bot 2011-12-08 20:20:05 PST
Comment on attachment 118489 [details]
Patch now with layout test

Clearing flags on attachment: 118489

Committed r102424: <http://trac.webkit.org/changeset/102424>
Comment 14 WebKit Review Bot 2011-12-08 20:20:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 noel gordon 2011-12-09 19:35:40 PST
Other port DRT's support eventSender.scheduleAsynchronousKeyDown ?
Comment 16 Adam Klein 2011-12-12 09:06:28 PST
(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.
Comment 17 noel gordon 2011-12-13 02:33:44 PST
OK. Thanks.