[Webkit-unassigned] [Bug 33383] REGRESSION (r52082): Missing event handlers on JQuery demo page (33383)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33383





--- Comment #25 from Darin Adler <darin at apple.com>  2010-02-04 17:06:42 PST ---
(From update of attachment 48175)
> +    // 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.