Bug 101428 – REGRESSION (r133633): ASSERTION FAILED: m_wrapper || !m_jsFunction

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kangil Han]

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

[Zan Dobersek]

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]

What is the regression range?

[Zan Dobersek]

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

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]

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]

Adam, do you plan to look into this soon? I think that this is a pretty bad assertion to fail.

[Adam Barth]

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

Certainly! Thank you.

[Alexey Proskuryakov]

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]

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

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

[Adam Barth]

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

[Adam Barth]

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

[Adam Barth]

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]

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

[Adam Barth]

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

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

bug 102217 seems to be related

[Alexey Proskuryakov]

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

[Geoffrey Garen]

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

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

Created attachment 174744 [details]
Patch

[WebKit Review Bot]

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]

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

[Geoffrey Garen]

Comment on attachment 174744 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 174744 [details]
Patch

Clearing flags on attachment: 174744

Committed r135063: <http://trac.webkit.org/changeset/135063>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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]

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

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]

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