Bug 93812 - REGRESSION(r125251): It made svg/custom/use-instanceRoot-as-event-target.xhtml assert and flakey
Summary: REGRESSION(r125251): It made svg/custom/use-instanceRoot-as-event-target.xhtm...
Status: RESOLVED DUPLICATE of bug 132148
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords: Gtk, LayoutTestFailure, Qt, QtTriaged
: 93819 (view as bug list)
Depends on: 132471
Blocks: 79668 88232
  Show dependency treegraph
 
Reported: 2012-08-13 02:46 PDT by Csaba Osztrogonác
Modified: 2015-01-18 09:06 PST (History)
16 users (show)

See Also:


Attachments
crash log (5.35 KB, text/plain)
2012-08-13 02:47 PDT, Csaba Osztrogonác
no flags Details
Patch (6.19 KB, patch)
2014-08-27 13:21 PDT, Guillaume Emont
darin: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (625.83 KB, application/zip)
2014-08-27 22:38 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2012-08-13 02:46:45 PDT
Qt crash log attached

It seems https://bugs.webkit.org/show_bug.cgi?id=93727 is a similar bug.
This test started to crash from r125133 On GTK, but from r125251 on Qt.
Comment 1 Csaba Osztrogonác 2012-08-13 02:47:23 PDT
Created attachment 157949 [details]
crash log
Comment 2 Csaba Osztrogonác 2012-08-13 04:18:17 PDT
I marked it as crash on Qt in debug mode - https://trac.webkit.org/changeset/125404/trunk/LayoutTests/platform/qt/TestExpectations

Please remove this expectation with the proper fix. Thanks.
Comment 3 Hayato Ito 2012-08-13 04:23:41 PDT
As for svg/custom/use-instanceRoot-as-event-target.xhtml, It seems r125251 is the cause.

I am now taking a look at other crashes on https://bugs.webkit.org/show_bug.cgi?id=93727.
Comment 4 Csaba Osztrogonác 2012-08-13 06:24:55 PDT
*** Bug 93819 has been marked as a duplicate of this bug. ***
Comment 5 Csaba Osztrogonác 2012-08-13 06:26:41 PDT
This test asserts in debug mode, but it is flakey (TEXT PASS) in relase mode.

After r125251 svg/custom/use-instanceRoot-as-event-target.xhtml
is very flakey on Qt platforms with the following error:

FAIL: Timed out waiting for notifyDone to be called

On Qt WK1 it was flakey between r125251-125396 and it fails always from 
r125398. And it fails always (not flakey) on Qt WK2 from r125251.


And it is flakey again on Qt WK1 after r125404 (skip an unrelated test)
Comment 6 Csaba Osztrogonác 2012-08-13 06:34:43 PDT
(In reply to comment #2)
> I marked it as crash on Qt in debug mode - https://trac.webkit.org/changeset/125404/trunk/LayoutTests/platform/qt/TestExpectations
> 
> Please remove this expectation with the proper fix. Thanks.

I moved it to the Skipped list instead of TestExpectations because of
this complex bug - https://trac.webkit.org/changeset/125416
Comment 7 Dominic Cooney 2012-08-13 23:47:23 PDT
This is my mess. I will try to clean it up.
Comment 8 Alexey Proskuryakov 2012-09-13 17:19:15 PDT
Also flaky on Mac.
Comment 9 Alexey Proskuryakov 2012-09-13 17:19:59 PDT
This was already skipped on platform/mac, I'll re-classify the failure.
Comment 10 Zan Dobersek 2013-01-17 03:42:08 PST
The test is still exhibiting flaky crashing in debug builds and flaky timeouts in release builds.
The Mac and Qt ports now skip the test. It's not failing on EFL, though.

Dashboard:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&showAllRuns=true&tests=svg%2Fcustom%2Fuse-instanceRoot-as-event-target.xhtml
Comment 11 Alexey Proskuryakov 2013-09-24 16:14:07 PDT
We still have <svg/custom/use-instanceRoot-as-event-target.xhtml> skipped because of this.
Comment 12 Alexey Proskuryakov 2014-05-01 22:20:45 PDT
I expect that Darin fixed this one too, unmarked in <http://trac.webkit.org/r168150>.

*** This bug has been marked as a duplicate of bug 132148 ***
Comment 13 WebKit Commit Bot 2014-05-02 10:24:45 PDT
Re-opened since this is blocked by bug 132471
Comment 14 Alexey Proskuryakov 2014-05-02 10:26:31 PDT
Surprisingly, this test is still failing/asserting. Rolling the expectations back in.

ASSERTION FAILED: !m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction
/Volumes/Data/slave/mavericks-debug/build/Source/WebCore/bindings/js/JSEventListener.h(96) : JSC::JSObject *WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext *) const
1   0x10e107b80 WTFCrash
2   0x10ffd9f20 WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) const
3   0x11029ec3b WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*)
4   0x10fa6748f WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow>&)
5   0x10fa66d5e WebCore::EventTarget::fireEventListeners(WebCore::Event*)
6   0x11091ccec WebCore::Node::handleLocalEvents(WebCore::Event&)
7   0x10fa31281 WebCore::EventContext::handleLocalEvents(WebCore::Event&) const
8   0x10fa31597 WebCore::MouseOrFocusEventContext::handleLocalEvents(WebCore::Event&) const
Comment 15 Darin Adler 2014-05-03 20:01:19 PDT
It’s entirely possible that my fix for bug 132148 didn’t actually solve the problem!
Comment 16 Guillaume Emont 2014-08-27 13:21:26 PDT
Created attachment 237244 [details]
Patch
Comment 17 Guillaume Emont 2014-08-27 13:26:25 PDT
While I could not reproduce the issue with svg/custom/use-instanceRoot-as-event-target.xhtml, I hit the same ASSERT in an (oldish) qt branch I am working on, and I could reproduce the issue in an up to date WebkitGtk+ build. The test I provide seem to be flakey if I test without my patch (though it always fails at least once if I --repeat-each=2), and it also fails (in the same flakey manner) in Release, even though it doesn't hit the ASSERT.
Comment 18 Build Bot 2014-08-27 22:38:24 PDT
Comment on attachment 237244 [details]
Patch

Attachment 237244 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5584579922493440

New failing tests:
storage/indexeddb/delete-closed-database-object.html
Comment 19 Build Bot 2014-08-27 22:38:28 PDT
Created attachment 237290 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 20 Andreas Kling 2014-09-03 10:43:01 PDT
Does this mean that registering an event listener on an object will cause that object to stay alive forever? That seems bad.
Comment 21 Darin Adler 2014-09-03 23:37:17 PDT
Comment on attachment 237244 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237244&action=review

Geoff, what’s your analysis about what’s happening here?

> Source/WebCore/bindings/js/JSEventListener.h:72
> +        mutable JSC::Strong<JSC::JSObject> m_wrapper;

I don’t think we can just change this from Weak to Strong; doing that would lead to cycles and every object with an event listener that captured the object would leak. We should write a test case that checks this does not happen.

> LayoutTests/ChangeLog:9
> +        There are cases where the wrapper is not referenced from anywhere any more at
> +        the time when the event needs to be fired, which prevents that firing.

We can’t fix this bug without understanding more about those cases.
Comment 22 Darin Adler 2015-01-18 09:05:15 PST

*** This bug has been marked as a duplicate of bug 132148 ***
Comment 23 Darin Adler 2015-01-18 09:06:07 PST
This time for sure. I actually understand what’s going on now!