Bug 17083 - REGRESSION (r24267): nested click() calls on the same element do not work (affects Acid3 test 73)
Summary: REGRESSION (r24267): nested click() calls on the same element do not work (af...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Antti Koivisto
URL:
Keywords:
Depends on:
Blocks: Acid3
  Show dependency treegraph
 
Reported: 2008-01-29 16:20 PST by Eric Seidel (no email)
Modified: 2008-03-17 18:58 PDT (History)
6 users (show)

See Also:


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 Details | Formatted Diff | Diff
add nesting count (2.95 KB, patch)
2008-03-16 22:34 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 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;
    },
Comment 1 Eric Seidel (no email) 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?
Comment 2 Maciej Stachowiak 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.
Comment 3 Ian 'Hixie' Hickson 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.
Comment 4 Darin Adler 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().
Comment 5 Darin Adler 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.
Comment 6 Ian 'Hixie' Hickson 2008-02-10 13:20:03 PST
Why can't the "slow script" stuff catch it?
Comment 7 Darin Adler 2008-02-10 13:20:25 PST
Created attachment 19048 [details]
patch, can't land because it re-introduces a compatibility bug
Comment 8 Darin Adler 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.
Comment 9 Darin Adler 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.
Comment 10 Ian 'Hixie' Hickson 2008-02-10 20:02:13 PST
Do no browsers nest clicks then?
Comment 11 Darin Adler 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.
Comment 12 Antti Koivisto 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. 


Comment 13 Ian 'Hixie' Hickson 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...
Comment 14 Antti Koivisto 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.
Comment 15 Ian 'Hixie' Hickson 2008-03-10 01:46:39 PDT
Well, mail the list, if the proposal gets traction I can certainly remove that subtest.
Comment 17 Antti Koivisto 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.
Comment 18 Darin Adler 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.
Comment 19 Ian 'Hixie' Hickson 2008-03-17 18:58:13 PDT
(I've removed this part of Acid3.)