Bug 101428 - REGRESSION (r133633): ASSERTION FAILED: m_wrapper || !m_jsFunction
Summary: REGRESSION (r133633): ASSERTION FAILED: m_wrapper || !m_jsFunction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords: Gtk, LayoutTestFailure, Regression
: 102217 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-11-06 21:55 PST by Kangil Han
Modified: 2012-11-18 13:56 PST (History)
15 users (show)

See Also:


Attachments
Makes ASSERT reliable on apple-mac (624 bytes, patch)
2012-11-15 15:20 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Reduction of patch that still exhibits the problem (3.38 KB, patch)
2012-11-15 17:05 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Reduction of patch that still exhibits the problem (3.35 KB, patch)
2012-11-15 17:37 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (4.67 KB, patch)
2012-11-16 12:31 PST, 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 Kangil Han 2012-11-06 21:55:39 PST
21:06:24.901 12251 worker/17 media/media-continues-playing-after-replace-source.html crashed, (stderr lines):
21:06:24.901 12251   ASSERTION FAILED: m_wrapper || !m_jsFunction
21:06:24.901 12251   /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/bindings/js/JSEventListener.h(90) : JSC::JSObject* WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) const
21:06:24.901 12251   1   0x7fd440decf63 WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) const
21:06:24.901 12251   2   0x7fd440df6a42 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*)
21:06:24.901 12251   3   0x7fd4403326ca WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul>&)
21:06:24.901 12251   4   0x7fd44033248a WebCore::EventTarget::fireEventListeners(WebCore::Event*)
21:06:24.901 12251   5   0x7fd44035dd8a WebCore::Node::handleLocalEvents(WebCore::Event*)
21:06:24.901 12251   6   0x7fd4403267b9 WebCore::EventContext::handleLocalEvents(WebCore::Event*) const
21:06:24.901 12251   7   0x7fd4403292f1 WebCore::EventDispatcher::dispatchEventAtTarget(WTF::PassRefPtr<WebCore::Event>)
21:06:24.901 12251   8   0x7fd4403286f7 WebCore::EventDispatcher::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)
21:06:24.901 12251   9   0x7fd440326b4c WebCore::EventDispatchMediator::dispatchEvent(WebCore::EventDispatcher*) const
21:06:24.901 12251   10  0x7fd440327814 WebCore::EventDispatcher::dispatchEvent(WebCore::Node*, WTF::PassRefPtr<WebCore::EventDispatchMediator>)
21:06:24.901 12251   11  0x7fd44035de84 WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)
21:06:24.901 12251   12  0x7fd440501663 WebCore::HTMLMediaElement::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)
21:06:24.901 12251   13  0x7fd44033444a WebCore::GenericEventQueue::timerFired(WebCore::Timer<WebCore::GenericEventQueue>*)
21:06:24.901 12251   14  0x7fd440335470 WebCore::Timer<WebCore::GenericEventQueue>::fired()
21:06:24.901 12251   15  0x7fd4408c04fa WebCore::ThreadTimers::sharedTimerFiredInternal()
21:06:24.901 12251   16  0x7fd4408c041b WebCore::ThreadTimers::sharedTimerFired()
21:06:24.901 12251   17  0x7fd441268fa5
21:06:24.901 12251   18  0x7fd43d76f33e _ecore_timer_expired_call
21:06:24.901 12251   19  0x7fd43d76f50b _ecore_timer_expired_timers_call
21:06:24.901 12251   20  0x7fd43d76c421
21:06:24.901 12251   21  0x7fd43d76cab7 ecore_main_loop_begin
21:06:24.902 12251   22  0x7fd441267919 WebCore::RunLoop::run()
21:06:24.902 12251   23  0x7fd44456aa22 WebProcessMainEfl
21:06:24.902 12251   24  0x4007f4 main
21:06:24.902 12251   25  0x7fd44392c76d __libc_start_main
21:06:24.902 12251   26  0x400719
Comment 1 Zan Dobersek 2012-11-07 06:51:56 PST
I'd say r133633 is at blame here.
http://trac.webkit.org/changeset/133633

At least wo media failures on GTK builders seem related, both time out:
media/audio-concurrent-supported.html
media/media-continues-playing-after-replace-source.html

There are also random crashes and increased flakiness on Mac in (again, at least) these two tests.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=media%2Faudio-concurrent-supported.html%2Cmedia%2Fmedia-continues-playing-after-replace-source.html
Comment 2 Adam Barth 2012-11-07 08:28:43 PST
What is the regression range?
Comment 3 Zan Dobersek 2012-11-07 09:12:41 PST
(In reply to comment #2)
> What is the regression range?

media/media-continues-playing-after-replace-source.html is crashing on EFL builders consistently, after build with the following range:
http://trac.webkit.org/log/?verbose=on&rev=133635&stop_rev=133630
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/5560
Comment 4 Adam Barth 2012-11-07 09:36:56 PST
http://trac.webkit.org/log/trunk/Source?verbose=on&rev=133635&stop_rev=133630

I agree that my patch is the only likely candidate.  Perhaps we should roll it out to see if that heals the ASSERT?
Comment 5 Alexey Proskuryakov 2012-11-07 16:09:30 PST
This assertion usually means that JS wrapper was garbage collected too early. Perhaps some code still tries to look for the wrapper in the HashMap only, and thus fails to protect the wrapper from GC.
Comment 6 Alexey Proskuryakov 2012-11-12 16:28:11 PST
Adam, do you plan to look into this soon? I think that this is a pretty bad assertion to fail.
Comment 7 Adam Barth 2012-11-12 16:32:51 PST
> Adam, do you plan to look into this soon? I think that this is a pretty bad assertion to fail.

It's on my list for Wednesday.  Is that soon enough?
Comment 8 Alexey Proskuryakov 2012-11-12 17:01:39 PST
Certainly! Thank you.
Comment 9 Alexey Proskuryakov 2012-11-13 15:43:41 PST
The assertion has been essentially removed in <http://trac.webkit.org/changeset/134495>. But the fact that it was firing presumably means that the events fail to be dispatched sometimes
Comment 10 Mark Lam 2012-11-13 17:08:28 PST
(In reply to comment #9)
> The assertion has been essentially removed in <http://trac.webkit.org/changeset/134495>. But the fact that it was firing presumably means that the events fail to be dispatched sometimes

FYI, the assertion has been "re-installed" for the normal world in <http://trac.webkit.org/changeset/134508>.
Comment 11 Adam Barth 2012-11-15 15:20:13 PST
Created attachment 174532 [details]
Makes ASSERT reliable on apple-mac
Comment 12 Adam Barth 2012-11-15 15:42:11 PST
I have verified that reverting http://trac.webkit.org/changeset/133633 locally makes the ASSERT not occur.
Comment 13 Adam Barth 2012-11-15 17:05:40 PST
Created attachment 174563 [details]
Reduction of patch that still exhibits the problem
Comment 14 Adam Barth 2012-11-15 17:15:23 PST
Here's the definition of wrapperOwner from JSNode.h:

inline JSC::WeakHandleOwner* wrapperOwner(DOMWrapperWorld*, Node*)
{
    DEFINE_STATIC_LOCAL(JSNodeOwner, jsNodeOwner, ());
    return &jsNodeOwner;
}

What's interesting about this definition is that it's declaring a static local variable in an inline function in a header.  I wonder if we're getting a separate instance of the static for each compilation unit?
Comment 15 Adam Barth 2012-11-15 17:37:45 PST
Created attachment 174572 [details]
Reduction of patch that still exhibits the problem
Comment 16 Adam Barth 2012-11-15 18:01:31 PST
> I wonder if we're getting a separate instance of the static for each compilation unit?

I moved the static out of line, but that didn't help.
Comment 17 Adam Barth 2012-11-15 18:11:41 PST
The issue is that Node and HTMLAudioElement are fighting about whether to use JSNodeOwner or JSHTMLAudioElementOwner as the owner object.  Previously, we used JSNodeOwner in the main world and JSHTMLAudioElementOwner in isolated worlds.  After my patch, we use JSHTMLAudioElementOwner in both kinds of worlds.
Comment 18 Jussi Kukkonen (jku) 2012-11-16 05:34:14 PST
bug 102217 seems to be related
Comment 19 Alexey Proskuryakov 2012-11-16 09:51:20 PST
*** Bug 102217 has been marked as a duplicate of this bug. ***
Comment 20 Geoffrey Garen 2012-11-16 11:22:01 PST
(In reply to comment #17)
> The issue is that Node and HTMLAudioElement are fighting about whether to use JSNodeOwner or JSHTMLAudioElementOwner as the owner object.

Nice debugging.

The behavior we want is both: We should use JSHTMLAudioElementOwner (to get ActiveDOMObject behavior), and that owner should delegate (possibly through inheritance) to JSNodeOwner.
Comment 21 Adam Barth 2012-11-16 11:24:25 PST
> The behavior we want is both: We should use JSHTMLAudioElementOwner (to get ActiveDOMObject behavior), and that owner should delegate (possibly through inheritance) to JSNodeOwner.

Thanks.  I'll try to work up a patch that does that.
Comment 22 Adam Barth 2012-11-16 12:31:21 PST
Created attachment 174744 [details]
Patch
Comment 23 WebKit Review Bot 2012-11-16 16:15:52 PST
Comment on attachment 174744 [details]
Patch

Attachment 174744 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14868357

New failing tests:
fast/dom/shadow/shadow-dom-event-dispatching.html
Comment 24 Adam Barth 2012-11-17 09:59:17 PST
@ggaren: Any interest in reviewing this patch?  I believe I've taken the approach you recommended.
Comment 25 Geoffrey Garen 2012-11-17 20:43:46 PST
Comment on attachment 174744 [details]
Patch

r=me
Comment 26 WebKit Review Bot 2012-11-17 21:17:06 PST
Comment on attachment 174744 [details]
Patch

Clearing flags on attachment: 174744

Committed r135063: <http://trac.webkit.org/changeset/135063>
Comment 27 WebKit Review Bot 2012-11-17 21:17:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Darin Adler 2012-11-18 07:35:51 PST
Comment on attachment 174744 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1060
> +        push(@headerContent, "public:\n");

Why did we add this?
Comment 29 Adam Barth 2012-11-18 08:43:41 PST
(In reply to comment #28)
> (From update of attachment 174744 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174744&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1060
> > +        push(@headerContent, "public:\n");
> 
> Why did we add this?

So that we could call these functions from their overrides in the derived class.  I guess could have made them protected instead.  The functions are basically public anyway since they're virtual and declared public in the base class.
Comment 30 Simon Fraser (smfr) 2012-11-18 12:46:18 PST
This broke bindings tests (probably need new results):
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/2930/steps/bindings-generation-tests/logs/stdio
Comment 31 Adam Barth 2012-11-18 13:56:42 PST
Fixenated in http://trac.webkit.org/changeset/135081