Bug 93654

Summary: REGRESSION(r125126): It made fast/events/keyevent-iframe-removed-crash.html assert
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: WebCore JavaScriptAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Critical CC: abarth, ap, eric, ggaren, mhahnenberg, mikhail.pozdnyakov, oliver, ossy, webkit.review.bot, zan
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 79668, 93382    
Attachments:
Description Flags
stack
none
Patch none

Csaba Osztrogonác
Reported 2012-08-09 14:15:01 PDT
... at least on GTK and Qt. GTK log: --------- - http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r125194%20%2835482%29/fast/events/keyevent-iframe-removed-crash-crash-log.txt Program terminated with signal 11, Segmentation fault. #0 0x00007fea831700ef in WebCore::JSEventListener::jsFunction (this=0x1b8eb1c0, scriptExecutionContext=0x1b389908) at ../../Source/WebCore/bindings/js/JSEventListener.h:90 90 ASSERT(m_wrapper || !m_jsFunction); ... Qt log: -------- - http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r125135%20%2824390%29/fast/events/keyevent-iframe-removed-crash-crash-log.txt crash log for DumpRenderTree (pid 13348): STDOUT: <empty> STDERR: ASSERTION FAILED: m_wrapper || !m_jsFunction STDERR: /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/Source/WebCore/bindings/js/JSEventListener.h(90) : JSC::JSObject* WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) const ...
Attachments
stack (32.56 KB, text/plain)
2012-08-09 15:25 PDT, Adam Barth 		no flags
Details
Patch (5.36 KB, patch)
2012-09-13 14:35 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Csaba Osztrogonác
Comment 1 2012-08-09 14:25:18 PDT
+info: it passes in itself on Qt, but it asserts if I run fast/events tests
Csaba Osztrogonác
Comment 2 2012-08-09 14:56:23 PDT
I marked it as CRASH on Qt - https://trac.webkit.org/changeset/125206/trunk/LayoutTests/platform/qt/TestExpectations Please remove this expectation with the proper fix.
Adam Barth
Comment 3 2012-08-09 15:25:58 PDT
Created attachment 157563 [details] stack This doesn't appear to repro on apple-mac. I've attached the crash log so we don't lose it.
Alexey Proskuryakov
Comment 4 2012-08-10 11:24:45 PDT
This assertion means that event listener's wrapper got garbage collected, so it's quite possible that the issue also affects Mac when stars align accordingly.
Adam Barth
Comment 5 2012-08-15 08:47:22 PDT
*** Bug 93727 has been marked as a duplicate of this bug. ***
Adam Barth
Comment 6 2012-08-29 17:02:11 PDT
I went to work on this today, but I couldn't tell if it's still a problem. All the tests are timing out on http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug and http://build.webkit.org/builders/Qt%20Windows%2032-bit%20Debug is offline.
Alexey Proskuryakov
Comment 8 2012-09-13 09:44:12 PDT
*** Bug 93602 has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 9 2012-09-13 09:45:57 PDT
Adam, this is reproducible on Mac with --repeat-each 100. See also bug 93878, which lists a number of tests failing with this assertion, attributed as a regression to same time frame.
Adam Barth
Comment 10 2012-09-13 10:10:04 PDT
Thanks. I've bumped this up my priority list.
Adam Barth
Comment 11 2012-09-13 11:13:10 PDT
I can reproduce the issue.
Adam Barth
Comment 12 2012-09-13 11:20:35 PDT
Adding a call to gc() right before eventSender.keyDown("x") makes it assert every time.
Adam Barth
Comment 13 2012-09-13 11:31:17 PDT
Maybe I don't understand JSC::Weak, but it looks like the wrapper is non-0: 0x00000001021c9539 in WebCore::JSEventListener::jsFunction (this=0x10d943a90, scriptExecutionContext=0x11186bd28) at JSEventListener.h:90 90 ASSERT(m_wrapper || !m_jsFunction); (gdb) p m_wrapper $1 = { <JSC::WeakImplAccessor<JSC::Weak<JSC::JSObject>, JSC::JSObject>> = {<No data fields>}, members of JSC::Weak<JSC::JSObject>: m_impl = 0x11181c378 } Is there more to converting m_wrapper to a bool than whether m_impl is 0?
Adam Barth
Comment 14 2012-09-13 11:34:16 PDT
Looks like the object is in the Finalized state: (gdb) p m_wrapper.m_impl->state() $4 = JSC::WeakImpl::Finalized Ok, this is starting to make sense.
Adam Barth
Comment 15 2012-09-13 12:27:00 PDT
Changing contentDocument to contentWindow.document in the test makes the crash go away.
Adam Barth
Comment 16 2012-09-13 12:32:00 PDT
Maybe the problem is that we never create the DOMWindow wrapper for the child iframe. Continuing to investigate.
Adam Barth
Comment 17 2012-09-13 13:19:59 PDT
It's enough to just touch contentWindow before touching contentDocument. In the V8 bindings, we have some code that makes sure we have a DOMWindow wrapper before we create a Document wrapper. I suspect JSC needs that as well. I don't fully understand the relation to r125126 yet.
Adam Barth
Comment 18 2012-09-13 14:35:27 PDT
Created attachment 163969 [details] Patch
Eric Seidel (no email)
Comment 19 2012-09-13 14:43:12 PDT
Comment on attachment 163969 [details] Patch LGTM.
Alexey Proskuryakov
Comment 20 2012-09-13 14:52:19 PDT
Thanks Adam! I'll work on unskipping the tests once this is in. Did you figure out why this started with r125126?
WebKit Review Bot
Comment 21 2012-09-13 15:19:21 PDT
Comment on attachment 163969 [details] Patch Clearing flags on attachment: 163969 Committed r128513: <http://trac.webkit.org/changeset/128513>
WebKit Review Bot
Comment 22 2012-09-13 15:19:26 PDT
All reviewed patches have been landed. Closing bug.
Adam Barth
Comment 23 2012-09-13 16:09:17 PDT
> Did you figure out why this started with r125126? Not entirely. r125126 changed a bunch of things about how install a new Document and a new DOMWindow into a Frame. It's possible that previously we were eagerly creating the DOMWindow wrapper somehow, but I don't see precisely how. We'd prefer to create it lazily, however, so that we can avoid creating it if its not needed (e.g., a frame with no script).
Alexey Proskuryakov
Comment 24 2012-09-13 17:26:03 PDT
Unskipped tests in <http://trac.webkit.org/changeset/128532>.
Note You need to log in before you can comment on or make changes to this bug.