Bug 101985 - JSC: JSEventListener should not access its JSFunction if its wrapper is gone
Summary: JSC: JSEventListener should not access its JSFunction if its wrapper is gone
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 102183
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-12 14:03 PST by Mark Lam
Modified: 2012-11-14 14:10 PST (History)
4 users (show)

See Also:


Attachments
1st draft. (3.09 KB, patch)
2012-11-12 17:28 PST, Mark Lam
buildbot: commit-queue-
Details | Formatted Diff | Diff
Fix. (3.01 KB, patch)
2012-11-13 09:27 PST, Mark Lam
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff
Fixing the assertion. (1.93 KB, patch)
2012-11-13 16:49 PST, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2012-11-12 14:03:19 PST
Adding a few null checks to enforce that JSEventListener::m_jsFunction is not accessed when its m_wrapper is NULL.

ref: <rdar://problem/11760332>
Comment 1 Mark Lam 2012-11-12 17:28:43 PST
Created attachment 173769 [details]
1st draft.
Comment 2 Build Bot 2012-11-13 03:04:03 PST
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
Comment 3 Mark Lam 2012-11-13 09:27:37 PST
Created attachment 173909 [details]
Fix.
Comment 4 Geoffrey Garen 2012-11-13 10:01:01 PST
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.
Comment 5 Mark Lam 2012-11-13 10:02:07 PST
(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.
Comment 6 Mark Lam 2012-11-13 15:24:33 PST
Landed in r134495: <http://trac.webkit.org/changeset/134495>.
Comment 7 Alexey Proskuryakov 2012-11-13 15:31:54 PST
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?
Comment 8 Mark Lam 2012-11-13 15:39:22 PST
(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.
Comment 9 Geoffrey Garen 2012-11-13 16:26:37 PST
> 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.
Comment 10 Mark Lam 2012-11-13 16:49:50 PST
Created attachment 174027 [details]
Fixing the assertion.
Comment 11 Geoffrey Garen 2012-11-13 16:53:41 PST
Comment on attachment 174027 [details]
Fixing the assertion.

r=me
Comment 12 Mark Lam 2012-11-13 17:00:58 PST
Assertion patch landed in r134508: <http://trac.webkit.org/changeset/134508>.
Comment 13 Roger Fong 2012-11-13 17:57:46 PST
(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)
Comment 14 Csaba Osztrogonác 2012-11-13 23:16:56 PST
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?
Comment 15 Mark Lam 2012-11-14 14:10:38 PST
(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>.