Bug 36376 - Assertion failure in media/video-controls-with-mutation-event-handler.html
Summary: Assertion failure in media/video-controls-with-mutation-event-handler.html
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.5
: P2 Normal
Assignee: Joseph Pecoraro
URL: http://build.webkit.org/results/Leopa...
Depends on: 36259
  Show dependency treegraph
Reported: 2010-03-19 10:46 PDT by David Kilzer (:ddkilzer)
Modified: 2010-03-20 02:09 PDT (History)
3 users (show)

See Also:

[PATCH] Temporary Fix (1.38 KB, patch)
2010-03-19 11:45 PDT, Joseph Pecoraro
ddkilzer: review+
Details | Formatted Diff | Diff
[PATCH] Bail in Default Handler before Attached (2.56 KB, patch)
2010-03-20 01:54 PDT, Joseph Pecoraro
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2010-03-19 10:46:43 PDT
After fixes for Bug 36259 landed, media/video-controls-with-mutation-event-handler.html began to crash (assert) on debug builds on Leopard:

(/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebCore/rendering/MediaControlElements.cpp:672 virtual void WebCore::MediaControlVolumeSliderElement::defaultEventHandler(WebCore::Event*))
Comment 1 Simon Fraser (smfr) 2010-03-19 10:51:54 PDT
Assertion is here:
        ExceptionCode ec = 0;
        m_mediaElement->setVolume(volume, ec);
Comment 2 David Kilzer (:ddkilzer) 2010-03-19 11:32:16 PDT
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.
Comment 3 Joseph Pecoraro 2010-03-19 11:45:17 PDT
Created attachment 51175 [details]
[PATCH] Temporary Fix

This was reviewed on IRC and committed.
Comment 4 Joseph Pecoraro 2010-03-19 11:46:59 PDT
I am going to leave this open and write a better patch tonight.
Comment 5 David Kilzer (:ddkilzer) 2010-03-19 11:49:41 PDT
Comment on attachment 51175 [details]
[PATCH] Temporary Fix

Comment 6 David Kilzer (:ddkilzer) 2010-03-19 11:50:37 PDT
(In reply to comment #3)
> Created an attachment (id=51175) [details]
> [PATCH] Temporary Fix
> This was reviewed on IRC and committed.

Committed r56250
Comment 7 Joseph Pecoraro 2010-03-20 01:51:48 PDT
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.
Comment 8 Joseph Pecoraro 2010-03-20 01:54:07 PDT
Created attachment 51220 [details]
[PATCH] Bail in Default Handler before Attached
Comment 9 Joseph Pecoraro 2010-03-20 01:54:39 PDT
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?
Comment 10 David Kilzer (:ddkilzer) 2010-03-20 02:00:05 PDT
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 @@
> +2010-03-20  Joseph Pecoraro  <joepeck@webkit.org>
> +
> +        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):

Comment 11 Joseph Pecoraro 2010-03-20 02:09:17 PDT
Committed r56301
	M	WebCore/ChangeLog
	M	WebCore/rendering/MediaControlElements.cpp
	M	WebCore/rendering/RenderMedia.cpp
r56301 = e0a505ccc4600f75ecae5b7003ad75cb7b6e3c24 (trunk)