Bug 17083 – REGRESSION (r24267): nested click() calls on the same element do not work (affects Acid3 test 73)

• Component: New Bugs
• Status: RESOLVED INVALID

[Eric Seidel (no email)]

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;
    },

[Eric Seidel (no email)]

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]

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]

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]

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]

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]

Why can't the "slow script" stuff catch it?

[Darin Adler]

Created attachment 19048 [details]
patch, can't land because it re-introduces a compatibility bug

[Darin Adler]

(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]

(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]

Do no browsers nest clicks then?

[Darin Adler]

(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]

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]

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]

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]

Well, mail the list, if the proposal gets traction I can certainly remove that subtest.

[Antti Koivisto]

http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-March/014181.html

[Antti Koivisto]

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]

Consensus is now that we're all stuck with the click recursion prevention. So we won't be changing WebKit.

[Ian 'Hixie' Hickson]

(I've removed this part of Acid3.)