Bug 93654 - REGRESSION(r125126): It made fast/events/keyevent-iframe-removed-crash.html assert
Summary: REGRESSION(r125126): It made fast/events/keyevent-iframe-removed-crash.html a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Critical
Assignee: Adam Barth
URL:
Keywords: Qt, QtTriaged
: 93602 93727 (view as bug list)
Depends on:
Blocks: 79668 93382
  Show dependency treegraph
 
Reported: 2012-08-09 14:15 PDT by Csaba Osztrogonác
Modified: 2012-09-13 17:26 PDT (History)
10 users (show)

See Also:


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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 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
...
Comment 1 Csaba Osztrogonác 2012-08-09 14:25:18 PDT
+info: it passes in itself on Qt, but it asserts if I run fast/events tests
Comment 2 Csaba Osztrogonác 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.
Comment 3 Adam Barth 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.
Comment 4 Alexey Proskuryakov 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.
Comment 5 Adam Barth 2012-08-15 08:47:22 PDT
*** Bug 93727 has been marked as a duplicate of this bug. ***
Comment 6 Adam Barth 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.
Comment 8 Alexey Proskuryakov 2012-09-13 09:44:12 PDT
*** Bug 93602 has been marked as a duplicate of this bug. ***
Comment 9 Alexey Proskuryakov 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.
Comment 10 Adam Barth 2012-09-13 10:10:04 PDT
Thanks. I've bumped this up my priority list.
Comment 11 Adam Barth 2012-09-13 11:13:10 PDT
I can reproduce the issue.
Comment 12 Adam Barth 2012-09-13 11:20:35 PDT
Adding a call to gc() right before eventSender.keyDown("x") makes it assert every time.
Comment 13 Adam Barth 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?
Comment 14 Adam Barth 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.
Comment 15 Adam Barth 2012-09-13 12:27:00 PDT
Changing contentDocument to contentWindow.document in the test makes the crash go away.
Comment 16 Adam Barth 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.
Comment 17 Adam Barth 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.
Comment 18 Adam Barth 2012-09-13 14:35:27 PDT
Created attachment 163969 [details]
Patch
Comment 19 Eric Seidel (no email) 2012-09-13 14:43:12 PDT
Comment on attachment 163969 [details]
Patch

LGTM.
Comment 20 Alexey Proskuryakov 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?
Comment 21 WebKit Review Bot 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>
Comment 22 WebKit Review Bot 2012-09-13 15:19:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Adam Barth 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).
Comment 24 Alexey Proskuryakov 2012-09-13 17:26:03 PDT
Unskipped tests in <http://trac.webkit.org/changeset/128532>.