Bug 93654 – REGRESSION(r125126): It made fast/events/keyevent-iframe-removed-crash.html assert

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

... 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
...

[Csaba Osztrogonác]

+info: it passes in itself on Qt, but it asserts if I run fast/events tests

[Csaba Osztrogonác]

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]

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]

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]

*** Bug 93727 has been marked as a duplicate of this bug. ***

[Adam Barth]

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.

[Adam Barth]

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

[Alexey Proskuryakov]

*** Bug 93602 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

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]

Thanks. I've bumped this up my priority list.

[Adam Barth]

I can reproduce the issue.

[Adam Barth]

Adding a call to gc() right before eventSender.keyDown("x") makes it assert every time.

[Adam Barth]

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]

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]

Changing contentDocument to contentWindow.document in the test makes the crash go away.

[Adam Barth]

Maybe the problem is that we never create the DOMWindow wrapper for the child iframe.  Continuing to investigate.

[Adam Barth]

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]

Created attachment 163969 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 163969 [details]
Patch

LGTM.

[Alexey Proskuryakov]

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 on attachment 163969 [details]
Patch

Clearing flags on attachment: 163969

Committed r128513: <http://trac.webkit.org/changeset/128513>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

> 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]

Unskipped tests in <http://trac.webkit.org/changeset/128532>.