Bug 101428

Summary:

REGRESSION (r133633): ASSERTION FAILED: m_wrapper || !m_jsFunction

Product:

WebKit

Reporter:

Kangil Han <kangil.han>

Component:

WebCore JavaScript

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, ap, dglazkov, d-r, eric.carlson, eric, feature-media-reviews, ggaren, haraken, japhet, jussi.kukkonen, mark.lam, simon.fraser, webkit.review.bot, zan

Priority:

Keywords:

Gtk, LayoutTestFailure, Regression

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Makes ASSERT reliable on apple-mac	none
Reduction of patch that still exhibits the problem	none
Reduction of patch that still exhibits the problem	none
Patch	none

Kangil Han

Reported 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

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Zan Dobersek

Comment 1 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

Adam Barth

Comment 2 2012-11-07 08:28:43 PST

What is the regression range?

Zan Dobersek

Comment 3 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

Adam Barth

Comment 4 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?

Alexey Proskuryakov

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

Alexey Proskuryakov

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

Adam Barth

Comment 7 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?

Alexey Proskuryakov

Comment 8 2012-11-12 17:01:39 PST

Certainly! Thank you.

Alexey Proskuryakov

Comment 9 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

Mark Lam

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

Adam Barth

Comment 11 2012-11-15 15:20:13 PST

Created attachment 174532 [details] Makes ASSERT reliable on apple-mac

Adam Barth

Comment 12 2012-11-15 15:42:11 PST

I have verified that reverting http://trac.webkit.org/changeset/133633 locally makes the ASSERT not occur.

Adam Barth

Comment 13 2012-11-15 17:05:40 PST

Created attachment 174563 [details] Reduction of patch that still exhibits the problem

Adam Barth

Comment 14 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?

Adam Barth

Comment 15 2012-11-15 17:37:45 PST

Created attachment 174572 [details] Reduction of patch that still exhibits the problem

Adam Barth

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

Adam Barth

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

Jussi Kukkonen (jku)

Comment 18 2012-11-16 05:34:14 PST

bug 102217 seems to be related

Alexey Proskuryakov

Comment 19 2012-11-16 09:51:20 PST

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

Geoffrey Garen

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

Adam Barth

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

Adam Barth

Comment 22 2012-11-16 12:31:21 PST

Created attachment 174744 [details] Patch

WebKit Review Bot

Comment 23 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

Adam Barth

Comment 24 2012-11-17 09:59:17 PST

@ggaren: Any interest in reviewing this patch? I believe I've taken the approach you recommended.

Geoffrey Garen

Comment 25 2012-11-17 20:43:46 PST

Comment on attachment 174744 [details] Patch r=me

WebKit Review Bot

Comment 26 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>

WebKit Review Bot

Comment 27 2012-11-17 21:17:13 PST

All reviewed patches have been landed. Closing bug.

Darin Adler

Comment 28 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?

Adam Barth

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

Simon Fraser (smfr)

Comment 30 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

Adam Barth

Comment 31 2012-11-18 13:56:42 PST

Fixenated in http://trac.webkit.org/changeset/135081

Note You need to log in before you can comment on or make changes to this bug.