JSC: JSEventListener should not access its JSFunction if its wrapper is gone

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Mark Lam]

Adding a few null checks to enforce that JSEventListener::m_jsFunction is not accessed when its m_wrapper is NULL.

ref: <rdar://problem/11760332>

[Mark Lam]

Created attachment 173769 [details]
1st draft.

[Build Bot]

Comment on attachment 173769 [details]
1st draft.

Attachment 173769 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14806958

New failing tests:
editing/selection/4895428-2.html
editing/pasteboard/copy-element-with-conflicting-background-color-from-rule.html
css3/flexbox/relayout-image-load.html
animations/suspend-resume-animation-events.html
animations/fill-mode-forwards2.html
editing/input/set-value-on-input-and-delete.html
accessibility/label-element-press.html
editing/spelling/spellcheck-input-search-crash.html
editing/selection/4776665.html
editing/style/iframe-onload-crash-unix.html
editing/execCommand/paste-1.html
editing/style/iframe-onload-crash-win.html
http/tests/cache/cancel-during-revalidation-succeeded.html
fast/block/float/float-in-float-hit-testing.html
editing/execCommand/copy-without-selection.html
editing/spelling/spellcheck-async-remove-frame.html
editing/input/set-value-on-input-and-type-textarea.html
http/tests/cache/post-with-cached-subresources.php
accessibility/media-element.html
editing/input/set-value-on-input-and-forward-delete.html
animations/fill-mode-forwards.html
editing/execCommand/5939887.html
editing/style/iframe-onload-crash-mac.html
editing/pasteboard/copy-paste-pre-line-content.html
compositing/iframes/layout-on-compositing-change.html
compositing/video/video-poster.html
editing/pasteboard/copy-resolves-urls.html
editing/input/set-value-on-input-and-type-input.html
http/tests/cache/post-redirect-get.php
editing/pasteboard/copy-standalone-image.html

[Mark Lam]

Created attachment 173909 [details]
Fix.

[Geoffrey Garen]

Comment on attachment 173909 [details]
Fix.

Your ChangeLog should describe the case you discovered that makes the old ASSERT invalid. That will also explain why you didn't write a regression test. Otherwise, a future reader won't have any way to know why our old assumption was invalid.

[Mark Lam]

(In reply to comment #4)
> (From update of attachment 173909 [details])
> Your ChangeLog should describe the case you discovered that makes the old ASSERT invalid. That will also explain why you didn't write a regression test. Otherwise, a future reader won't have any way to know why our old assumption was invalid.

Will do before landing.  Thanks.

[Mark Lam]

Landed in r134495: <http://trac.webkit.org/changeset/134495>.

[Alexey Proskuryakov]

Comment on attachment 173909 [details]
Fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=173909&action=review

> Source/WebCore/bindings/js/JSEventListener.h:94
>          ASSERT(m_wrapper || !m_jsFunction);

Why did you keep this assertion?

[Mark Lam]

(In reply to comment #7)
> (From update of attachment 173909 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173909&action=review
> 
> > Source/WebCore/bindings/js/JSEventListener.h:94
> >          ASSERT(m_wrapper || !m_jsFunction);
> 
> Why did you keep this assertion?

Paranoia.  Plus it documents a contract between m_wrapper and m_jsFunction from that point forth.

[Geoffrey Garen]

> Plus it documents a contract between m_wrapper and m_jsFunction from that point forth.

Let's update the ASSERT to cover the case that is still ASSERT-able in the main thread normal world.

[Mark Lam]

Created attachment 174027 [details]
Fixing the assertion.

[Geoffrey Garen]

Comment on attachment 174027 [details]
Fixing the assertion.

r=me

[Mark Lam]

Assertion patch landed in r134508: <http://trac.webkit.org/changeset/134508>.

[Roger Fong]

(In reply to comment #12)
> Assertion patch landed in r134508: <http://trac.webkit.org/changeset/134508>.

Looks like the patch did somethin funky with the windows bots.

http://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/29891

Looks like we have a hanging test that just keeps on hanging until run-webkit-tests times out (after 20 minutes or so)

[Csaba Osztrogonác]

It broke two tests everywhere: one fail and one timeout - https://bugs.webkit.org/show_bug.cgi?id=102183

Could you check and fix it, please?

[Mark Lam]

(In reply to comment #14)
> It broke two tests everywhere: one fail and one timeout - https://bugs.webkit.org/show_bug.cgi?id=102183
> 
> Could you check and fix it, please?

FYI, the fix has landed in r134666: <http://trac.webkit.org/changeset/134666>.