Bug 36376 – Assertion failure in media/video-controls-with-mutation-event-handler.html

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[David Kilzer (:ddkilzer)]

After fixes for Bug 36259 landed, media/video-controls-with-mutation-event-handler.html began to crash (assert) on debug builds on Leopard:

ASSERTION FAILED: !ec
(/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebCore/rendering/MediaControlElements.cpp:672 virtual void WebCore::MediaControlVolumeSliderElement::defaultEventHandler(WebCore::Event*))

[Simon Fraser (smfr)]

Assertion is here:
        ExceptionCode ec = 0;
        m_mediaElement->setVolume(volume, ec);
        ASSERT(!ec);

[David Kilzer (:ddkilzer)]

The issue is that HTMLMediaElement::setVolume() expects a range of 0.0f to 1.0f, but when RenderMedia::createVolumeSlider() sets the precision attribute to "float" (before setting the maximum value to 1), an event is fired off (with the default range still 0 to 100), after which MediaControlVolumeSliderElement::defaultEventHandler() tries to set the volume to 50 (since that's the default value for the slider until the maximum value is changed).  HTMLMediaElement::setVolume() sees that 50 is greater than 1.0f, and sets an exception code, after which we assert.

Joe is looking into potential solutions now.

[Joseph Pecoraro]

Created attachment 51175 [details]
[PATCH] Temporary Fix

This was reviewed on IRC and committed.

[Joseph Pecoraro]

I am going to leave this open and write a better patch tonight.

[David Kilzer (:ddkilzer)]

Comment on attachment 51175 [details]
[PATCH] Temporary Fix

r=me

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> Created an attachment (id=51175) [details]
> [PATCH] Temporary Fix
> 
> This was reviewed on IRC and committed.

Committed r56250
<http://trac.webkit.org/changeset/56250>

[Joseph Pecoraro]

Yes, here is the relevant part of the stack trace from David Kilzer's comments:

> #0  WebCore::HTMLMediaElement::setVolume at WebKit/WebCore/html/HTMLMediaElement.cpp:1270
> #1  WebCore::MediaControlVolumeSliderElement::defaultEventHandler at WebKit/WebCore/rendering/MediaControlElements.cpp:671
> #2  WebCore::Node::dispatchGenericEvent at WebKit/WebCore/dom/Node.cpp:2680
> #3  WebCore::Node::dispatchEvent at WebKit/WebCore/dom/Node.cpp:2567
> #4  WebCore::Node::dispatchSubtreeModifiedEvent at WebKit/WebCore/dom/Node.cpp:2720
> #5  WebCore::NamedNodeMap::addAttribute at WebKit/WebCore/dom/NamedAttrMap.cpp:257
> #6  WebCore::Element::setAttribute at WebKit/WebCore/dom/Element.cpp:606
> #7  WebCore::Element::setAttribute at WebKit/WebCore/dom/Element.cpp:186
> #8  WebCore::RenderMedia::createVolumeSlider at WebKit/WebCore/rendering/RenderMedia.cpp:266
> #9  WebCore::RenderMedia::updateControls at WebKit/WebCore/rendering/RenderMedia.cpp:349
> #10 WebCore::RenderMedia::updateFromElement at WebKit/WebCore/rendering/RenderMedia.cpp:295
> #11 WebCore::RenderVideo::updateFromElement at WebKit/WebCore/rendering/RenderVideo.cpp:187
> #12 WebCore::HTMLMediaElement::attach at WebKit/WebCore/html/HTMLMediaElement.cpp:311
> ...

At #8 was the setAttribute(precisionAttr), triggering setVolume with the default value of 50.

I looked into this again tonight. I propose a quick break from defaultEventHandler() if the element is not yet attached; in both MediaControlVolumeSliderElement and MediaControlTimelineElement since they both use the range input. In both createVolumeSlider() and createTimeline() there is an explicit attach at the end of the method. Swapping the lines back to their original position and adding a check passes the media tests. setVolume() doesn't event need to be called before the explicit attach because the setAttribute is setting it with that value!

Thinking deeper: Does it ever make sense to fire an event on a non-attached node? If someone is modifying elements they created, unattached to the main DOM tree, should events be fired? In this case, the DOMSubtreeMutation event is being fired. I'll investigate this further on my own.

[Joseph Pecoraro]

Created attachment 51220 [details]
[PATCH] Bail in Default Handler before Attached

[Joseph Pecoraro]

To those who know the media elements well: Can these inputs be manipulated by scripts? Could someone dynamically set the volume slider's max value to something greater than 1 and cause this problem to resurface?

[David Kilzer (:ddkilzer)]

Comment on attachment 51220 [details]
[PATCH] Bail in Default Handler before Attached

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index cfcd247..6742c90 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,19 @@
> [phone]  Joseph Pecoraro  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Assertion failure in media/video-controls-with-mutation-event-handler.html
> +        https://bugs.webkit.org/show_bug.cgi?id=36376

Please mention the test in the ChangeLog entry here:

        Test: media/video-controls-with-mutation-event-handler.html

> +        Break early (when not attached) in the defaultEventHandler before the
> +        slider is completely set up.
> +
> +        * rendering/MediaControlElements.cpp:
> +        (WebCore::MediaControlTimelineElement::defaultEventHandler):
> +        (WebCore::MediaControlVolumeSliderElement::defaultEventHandler):
> +        * rendering/RenderMedia.cpp:
> +        (WebCore::RenderMedia::createVolumeSlider):

r=me

[Joseph Pecoraro]

Committed r56301
	M	WebCore/ChangeLog
	M	WebCore/rendering/MediaControlElements.cpp
	M	WebCore/rendering/RenderMedia.cpp
r56301 = e0a505ccc4600f75ecae5b7003ad75cb7b6e3c24 (trunk)
<http://trac.webkit.org/changeset/56301>