| Summary: | REGRESSION(r125126): It made fast/events/keyevent-iframe-removed-crash.html assert | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||
| Component: | WebCore JavaScript | Assignee: | 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
Csaba Osztrogonác
2012-08-09 14:15:01 PDT
+info: it passes in itself on Qt, but it asserts if I run fast/events tests 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. 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.
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. *** Bug 93727 has been marked as a duplicate of this bug. *** 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. http://build.webkit.sed.hu/waterfall?show=x86-64%20Linux%20Qt%20Debug seems to indicate that it is still a problem: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r127050%20(24811)/fast/events/keyevent-iframe-removed-crash-stderr.txt *** Bug 93602 has been marked as a duplicate of this bug. *** 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. Thanks. I've bumped this up my priority list. I can reproduce the issue. Adding a call to gc() right before eventSender.keyDown("x") makes it assert every time.
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?
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. Changing contentDocument to contentWindow.document in the test makes the crash go away. Maybe the problem is that we never create the DOMWindow wrapper for the child iframe. Continuing to investigate. 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. Created attachment 163969 [details]
Patch
Comment on attachment 163969 [details]
Patch
LGTM.
Thanks Adam! I'll work on unskipping the tests once this is in. Did you figure out why this started with r125126? Comment on attachment 163969 [details] Patch Clearing flags on attachment: 163969 Committed r128513: <http://trac.webkit.org/changeset/128513> All reviewed patches have been landed. Closing bug. > 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). Unskipped tests in <http://trac.webkit.org/changeset/128532>. |