Bug 17083

Summary: REGRESSION (r24267): nested click() calls on the same element do not work (affects Acid3 test 73)
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Antti Koivisto <koivisto>
Status: RESOLVED INVALID    
Severity: Normal CC: adele, andersca, darin, ian, koivisto, mjs
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 17064    
Attachments:
Description Flags
patch, can't land because it re-introduces a compatibility bug
none
add nesting count none

Eric Seidel (no email)
Reported 2008-01-29 16:20:16 PST
nested event dispatch does not work (Acid3 bug) function () { // test 74: nested events, from Jonas Sicking var doc = kungFuDeathGrip.childNodes[3].contentDocument; // implied events var up = 0; var down = 0; var button = doc.createElement("button"); button.type = "button"; button.onclick = function () { up += 1; if (up < 10) button.click(); down += up; }; button.click(); assertEquals(up, 10, "click event handler called the wrong number of times"); assertEquals(down, 100, "click event handler called in the wrong order"); // explicit events up = 0; down = 0; button.addEventListener('test', function () { up += 1; var e = doc.createEvent("HTMLEvents"); e.initEvent('test', false, false); if (up < 20) button.dispatchEvent(e); down += up; }, false); var evt = doc.createEvent("HTMLEvents"); evt.initEvent('test', false, false); button.dispatchEvent(evt); assertEquals(up, 20, "test event handler called the wrong number of times"); assertEquals(down, 400, "test event handler called in the wrong order"); return 5; },
Attachments
patch, can't land because it re-introduces a compatibility bug (7.68 KB, patch)
2008-02-10 13:20 PST, Darin Adler 		no flags
DetailsFormatted DiffDiff
add nesting count (2.95 KB, patch)
2008-03-16 22:34 PDT, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2008-02-07 21:01:19 PST
I'm not really sure we should support this w/ our current JS implementation. We'd have to pick an arbitrary maximum recursion value to avoid blowing out the stack. Moz can do this because their interpreter is not recursive. However, even so, I think the onclick event is required to be dispatched from within the click() call, so that would require at least a few stack frames. Hixie, if we're supposed to allow recursion here... how deep? And is 10 levels of recursion really better than 0 here?
Maciej Stachowiak
Comment 2 2008-02-07 21:39:12 PST
I think we should support this - in most cases the stack limit won't be an issue. Actually I'm surprised it doesn't work already - I don't think we have anything to block re-entry into event dispatch.
Ian 'Hixie' Hickson
Comment 3 2008-02-07 22:32:55 PST
You should allow some nesting, but exactly how much you allow is up to you. It falls under "hardware limitations". I recommend throwing an exception when you have a stack overflow.
Darin Adler
Comment 4 2008-02-10 12:50:42 PST
We pass the pure event recursive dispatch part of the test just fine. Our problem with this test is specifically in the case of click().
Darin Adler
Comment 5 2008-02-10 13:17:58 PST
This prohibition on click() recursion is new to Safari 3, added to prevent a button at <http://forums.whirlpool.net.au/> from causing the browser to hang: http://trac.webkit.org/projects/webkit/changeset/24267 We decided to change our behavior to match Firefox at that time. I have a patch that removes the prohibition and reintroduces the bug. Now we'll have to think of another way to prevent that site from hanging. The hang is due to the massive amount of work that ends up getting done during the infinite recursion. I suppose that eventually we'd hit the JavaScript recursive call limit, but in practice it's too slow and we never get there. I'll attach my patch with test case, but we can't land it because it re-introduces the bug fixed in r24267.
Ian 'Hixie' Hickson
Comment 6 2008-02-10 13:20:03 PST
Why can't the "slow script" stuff catch it?
Darin Adler
Comment 7 2008-02-10 13:20:25 PST
Created attachment 19048 [details] patch, can't land because it re-introduces a compatibility bug
Darin Adler
Comment 8 2008-02-10 13:21:26 PST
(In reply to comment #6) > Why can't the "slow script" stuff catch it? Maybe it can, but this site "just works" in other browsers. I don't want it to turn into a "slow script" site in Safari unless there's no other alternative.
Darin Adler
Comment 9 2008-02-10 13:23:14 PST
(In reply to comment #6) > Why can't the "slow script" stuff catch it? I suspect it's a bug in our slow script mechanism that we don't catch this as a slow script. So that's a separate problem. We want to be compatible with this site without the "slow script" machinery, and we want the slow script machinery to work in cases like this that involve many trips into and out of the script interpreter.
Ian 'Hixie' Hickson
Comment 10 2008-02-10 20:02:13 PST
Do no browsers nest clicks then?
Darin Adler
Comment 11 2008-02-10 20:22:54 PST
(In reply to comment #10) > Do no browsers nest clicks then? I've tried only Safari and Firefox. I haven't tested IE yet. It's entirely possible that site worked for a different reason with IE. No idea about Opera or the others either.
Antti Koivisto
Comment 12 2008-03-09 22:25:23 PDT
On Safari and Firefox <input id=button type="checkbox" onclick="document.getElementById('console').innerHTML = ++counter; this.click()"> <div id=console></div> <script> window.counter = 0; document.getElementById('button').click(); </script> prints "1". Clicking on the button increases the value by 2. IE6 prints "1" and user clicks increase the value by 1. It seems like IE is preventing click recursion completely while Safari and Firefox allow it from user event only. Acid3 is asking either some 10 upper bound for breaking the recursion or relying on slow script warning. Latter turns simple mistakes into catastrophical user visible errors while former is rather arbitrary.
Ian 'Hixie' Hickson
Comment 13 2008-03-09 23:18:40 PDT
Well, the specs make no allowance for the API being non-reentrant... Seems odd for it to not be reentrant, since you can easily imagine clicking one button from another, and from there to clicking the same button again for some reason doesn't seem like a big leap...
Antti Koivisto
Comment 14 2008-03-10 00:53:47 PDT
Completely removing the check that prevents recursion is not an option since it breaks web sites. Having some other limit than 1 is possible but astonishingly pointless. If we want to pass this test as it is we can trivially add per-node click recursion counter in a separate hash and set some limit greater than what Acid3 tests for. This might still cause web site regressions (behavior differs from any shipping browser) but at least it won't hang the browser. I'd rather see the current perfectly sane behavior specced and Acid3 fixed to match implementations.
Ian 'Hixie' Hickson
Comment 15 2008-03-10 01:46:39 PDT
Well, mail the list, if the proposal gets traction I can certainly remove that subtest.
Antti Koivisto
Comment 17 2008-03-16 22:34:40 PDT
Created attachment 19824 [details] add nesting count Maximum nesting count it set to 9, the lowest number that passes acid3. In http://forums.whirlpool.net.au/ reply screen this patch causes ~3s hang with every click in debug builds, probably something around 1s on release. This would be total browser lockup on phone class hardware. It would be shame if Acid3 forced us into checking in this.
Darin Adler
Comment 18 2008-03-17 18:47:59 PDT
Consensus is now that we're all stuck with the click recursion prevention. So we won't be changing WebKit.
Ian 'Hixie' Hickson
Comment 19 2008-03-17 18:58:13 PDT
(I've removed this part of Acid3.)
Note You need to log in before you can comment on or make changes to this bug.