Bug 33383 – REGRESSION (r52082): Missing event handlers on JQuery demo page (33383)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Kevin M. Dean]

This is a hard one the pin down since it occurs seemingly randomly, but basically for the last month with all versions from r52126 to the current nightly I've been having an annoying problem where AJAX elements on a page will fail to trigger. Reloading the page will eventually fix it, although it can take multiple reloads to fix and if you continue to reload again after it will eventually fail again. I've seen this problem on multiple sites that make use of AJAX so it appears to be a non-site specific issue. No problem is seen in Safari 4.0.4 and doesn't appear to occur in R52039 and earlier.

I'm using one of the jquery animate demos as an easy to access test case. When viewing the above link it may work fine the first time you load the page, but start reloading and it will eventually fail. I've found that it can fail anywhere from the 1st time the page has loaded to the 20th time it's been reloaded. When it's working the demo should animate as expected, but when it fails nothing visually happens except the hash (#) character appears on the URL in the Address Bar indicating that during failure the hash is not being intercepted by the AJAX scripts as expected.

In every day use this problem doesn't always take a bunch of re-loads to occur. I have it happen often and daily when just moving through sites that use AJAX such as http://personals.yahoo.com/ and http://www.mania.com/aodvb/forumdisplay.php?f=37

Under Yahoo, they have features that show floating boxes, drop down menus, changing from basic view to gallery view and select menus that should trigger a function when changed. All will fail when the problem occurs.

The latter is a vBulletin forum and the problem occurs when I use the Form Tools drop down menu to select Mark This Forum Read. When I first visit the forum, it works as expected, but as I move from board to board eventually it will fail. Normally when rolling over an item in the sub-menu it highlights with a gray background and white text, but when it fails sometimes the gray background doesn't appear or gets stuck on the first item, but doesn't turn off or move to the 2nd item. Clicking the item at this point also fails.

No Errors appear in the Web Inspector Console or the system Console except, for a while now, when launching Webkit I see the following error:

CFPropertyListCreateFromXMLData(): Old-style plist parser: missing semicolon in dictionary.

[Kevin M. Dean]

This may have some relation to the changes and fixes made just before with this bug: REGRESSION(r51978-r52039): AJAX "Mark This Forum Read" function no longer works - https://bugs.webkit.org/show_bug.cgi?id=32498

[Kevin M. Dean]

If it's like the previously mentioned bug, then it's quite possible that this is a PowerPC only bug. I can't test intel here.

Anyone checking in to this, since this pretty much breaks every AJAX site out there.

[Geoffrey Garen]

Much to my surprise, I can confirm this bug with a local r53459 debug interpreter build on Intel.

Steps to reproduce:
1. Build with JIT disabled.
2. Load http://jqueryui.com/demos/animate/
3. Click 'Toggle Effect' button
4. If you see an animation, reload and goto step 3
--> Eventually, clicking the button will fail to produce an animation.

The site seems to be set up with a hash-based notification mechanism: A function that wants something to happen modifies window.location.hash, and a 200-ms repeating timer listens for those modifications.

[Kevin M. Dean]

(In reply to comment #3)
To clarify, the issue is not limited to just this jquery technique. The jquery page was just a good reusable test case. I've seen this problem with different code libraries on different sites with different types of functions. From what I've seen they're hash based functions that use AJAX to change or load content on the page.

The jquery sample and the vbulletin forum use jquery, but Yahoo Personals uses their Yahoo User Interface Library. So, the 2 libraries are running across the same issue.

As mentioned before I have absolutely no issue running r51978, it's all the later nightlies that have the problem.

[Geoffrey Garen]

In the failing case, the document is missing a 'click' event listener, and so is the 'Toggle Effect' element:

<a href="#" id="button" class="ui-state-default ui-corner-all">Toggle Effect</a>

[Geoffrey Garen]

Turns out this bug is not interpreter-only, though it is easier to reproduce in an interpreter debug build -- say, 1 failure / 5 reloads instead of 1/20.

[Geoffrey Garen]

Can't reproduce this bug with a local copy of the page.

My guess about the failure is that either (a) the document's 'ready' event never fires or (b) the document's 'ready' event fires too early, before something relevant has loaded. So far, the evidence argues agains both theories, though.

[Geoffrey Garen]

Looks like this was introduced in r52082 (JavaScriptCore: Changed GC from mark-sweep to mark-allocate).

[Geoffrey Garen]

Garbage collecting more often seems to make the bug go away.

[Geoffrey Garen]

In the failing case, #button has a 'click' event listener added to it, then, without any call to removeEventListener() or removeAllEventListeners(), the listener somehow disappears.

[Geoffrey Garen]

The listener disappears because of a call to invalidateEventListeners() by the JSNode destructor.

So the bug here is either:

(a) A wrapper for a DOM node is garbage collected even though the node is in the document and has a registered event listener.

OR

(b) A wrapper for a DOM node invalidate's the node's listeners, even though the wrapper is stale and another wrapper is keeping the node and its listeners alive.

[Geoffrey Garen]

An ASSERT confirms that the bug is (b).

[Geoffrey Garen]

Created attachment 47568 [details]
Patch

[Darin Adler]

Comment on attachment 47568 [details]
Patch

Why not remove the EventTarget from the IDL files too?

r=me

[Alexey Proskuryakov]

Comment on attachment 47568 [details]
Patch

After a discussion with Geoff, my understanding is that this should not be landed. Feel free to land if I misunderstood.

[Geoffrey Garen]

Since our DOM isn't perfect about marking observable objects, we need to maintain some sort of weak pointer system for event listeners, to prevent stale event listeners from being invoked. (As a separate matter, we'd also like to fix those observability problems.)

[Ted]

Since r54122 I am seeing a lot of odd behavior related to jQuery and jQuery UI.

For the original URL for this bug, http://jqueryui.com/demos/animate/, about 1 in 5 times the first run of the animation does not change the background, so the text fades to white and the background stays white instead of turning red.  Subsequent runs produce the correct animation.  This does not ever happen with Safari 4.0.4 or Chrome 4.0.249.49 (35163) beta.

On my own pages, after instantiating a lot of jQuery UI widgets, I am finding that their internal state objects are unexpectedly disappearing.  These are normally accessible with jQuery's .data(), e.g.,

$('#tabs').tabs();
console.log(typeof $('#tabs').data('tabs')); // object
// later on after much other code has run...
console.log(typeof $('#tabs').data('tabs')); // undefined... why?

Looking into this further, it appears that at seemingly random places in my code, the expandos that jQuery sets on elements to tie them to the datastore are being reset to undefined.  For instance, this is a snippet from one of my pages...

    var $pagination = $('#pagination'), 
      button = $('.ui-button')[0], i;
    for (i in button) {
      if (i.match(/^jQuery/)) { console.log(i, button[i]); }
      // produces: jQuery1265066165770 12
      // that's the expando jQuery uses to tie the element to the datastore
    }
    // the following line should have no effect on button
    // there aren't even any .ui-button's within $pagination
    console.log($pagination.find('a span'));
    // produces: a normal-looking jQuery object
    for (i in button) {
      if (i.match(/^jQuery/)) { console.log(i, button[i]); }
      // produces: jQuery1265066165770 undefined
      // where'd the expando go?
    }

The best I can figure is some over-aggressive GC that is deleting jQuery's expandos... unfortunately the entirety of the above code is not publicly hosted, but I'm hoping somebody else is observing what I am and can confirm.

[Geoffrey Garen]

Created attachment 48095 [details]
Patch

[WebKit Review Bot]

Attachment 48095 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ForwardingHeaders/runtime/WeakGCPtr.h:1:  #ifndef header guard has wrong style, please use: WeakGCPtr_h  [build/header_guard] [5]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

>+        There were two bugs here:

And only one test?!

I realize that any regression test for the latter issue will become inefficient as soon as we fix a bug that makes it possible, but it will at least confirm that your fix works right now. And for something like Editor undo/redo, the bug may remain unfixed for quite a while.

+function gc() {
+    for (var i = 0; i < 5000; ++i)
+        new Object;
+}

A comment in other gc() functions states that Firefox collects after about 9000 allocations, FWIW. And we definitely want a special case for DumpRenderTree (GCController.collect()).

+    gc();
+    gc();
+    gc();

I know, I've set a bad example...

[Ted]

Since r54277 I don't see the issue with the expandos.  It may have been unrelated?  .data() seems to work on my pages again...

I do still see the problem wih the animation not affecting the background on the first run at http://jqueryui.com/demos/animate/.  Its frequency is still the same--about 1 in every 5 page loads.

[Geoffrey Garen]

> >+        There were two bugs here:
> 
> And only one test?!

Since there's no way to make bug #2 happen in the context of this jQuery demo, I thought I'd wait and write other tests along with the other bugs you've filed (like the XPath bug). 

I don't mind adding a test to this patch, though.

> +function gc() {
> +    for (var i = 0; i < 5000; ++i)
> +        new Object;
> +}
> 
> A comment in other gc() functions states that Firefox collects after about 9000
> allocations, FWIW. And we definitely want a special case for DumpRenderTree
> (GCController.collect()).

I guess this function could use a better name, but I'm not sure what to call it. We don't actually want to force a full GC (which is what GCController.collect() would do.) That would hide the bug. Rather, we want to move the collector through a full allocation phase. I guess I'll rename the function to "allocate".

I'll submit a new patch with the rename and an additional test.

[Geoffrey Garen]

Created attachment 48175 [details]
patch

It turns out that we can't check in an XPath test until the XPath bug (bug 34231) is fixed, since the test will cause assertion failures. So, I've uploaded the test to bug 34231.

Here's a new version of my patch, with "gc" renamed to "allocate", and an extra assertion added for good measure.

[Alexey Proskuryakov]

Comment on attachment 48175 [details]
patch

r=me

[Darin Adler]

Comment on attachment 48175 [details]
patch

> +    // m_wrapper might be null if it's awaiting destruction.
> +    if (!m_wrapper || m_wrapper == wrapper)
> +        m_wrapper = 0;

At first glance I did not see the need for the !m_wrapper clause: "What's the harm in setting m_wrapper to 0 if it's already 0? Or in not setting it to 0 in that case? Seems you can leave out the comment and the extra branch."

Then I finally figured it out. WeakGCPtr's use of "!" and "= 0" is subtle. There is a case where something that already returns 0 can still benefit by having the value 0 assigned to it. Yuck. Subtle in a way that can lead to later bugs. The comment tries to address the issue but doesn't speak directly enough to the strangeness of WeakGCPtr semantics.

I think you should reword the comment to give later programmers a fighting chance.

> +        // Try to verify that m_jsFunction wasn't recycled. (Not exact, since an
> +        // event listener can be almost anything, but this makes test-writing easier).
> +        ASSERT(!m_jsFunction || static_cast<JSC::JSCell*>(m_jsFunction)->isObject());

Maybe the JSObject class should help you do this debugging check somehow. It's a sort of consistency check like the malloc_size calls that Alexey was adding recently. It's quite subtle to cast to a JSCell* and then call isObject as a consistency check, and so it would be nice if it was somewhere off by itself where the technique could be explained.

I don’t know how the patch works well enough to review it, so I guess the review+ has to wait for Alexey. Heh, looks like he reviewed while I was writing this.

[Geoffrey Garen]

> I think you should reword the comment to give later programmers a fighting
> chance.

Re-worded. I think I'll write a follow-up patch to add a clear(JSCell*) interface to WeakGCPtr, and use it here. Hopefully, that will encapsulate the subtlety inside WeakGCPtr.

> > +        // Try to verify that m_jsFunction wasn't recycled. (Not exact, since an
> > +        // event listener can be almost anything, but this makes test-writing easier).
> > +        ASSERT(!m_jsFunction || static_cast<JSC::JSCell*>(m_jsFunction)->isObject());
> 
> Maybe the JSObject class should help you do this debugging check somehow. It's
> a sort of consistency check like the malloc_size calls that Alexey was adding
> recently. It's quite subtle to cast to a JSCell* and then call isObject as a
> consistency check, and so it would be nice if it was somewhere off by itself
> where the technique could be explained.

This is indeed some weird code. I think I can write a follow-up patch to move this code into JSObject.

[Geoffrey Garen]

Committed revision 54400.