36376 2010-03-19 10:46:43 -0700 Assertion failure in media/video-controls-with-mutation-event-handler.html 2010-03-20 02:09:17 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Mac (Intel) OS X 10.5 RESOLVED FIXED http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r56242%20(11656)/results.html P2 Normal --- 36259 1 ddkilzer joepeck eric.carlson joepeck simon.fraser oldest_to_newest 201927 0 ddkilzer 2010-03-19 10:46:43 -0700 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*)) 201933 1 simon.fraser 2010-03-19 10:51:54 -0700 Assertion is here: ExceptionCode ec = 0; m_mediaElement->setVolume(volume, ec); ASSERT(!ec); 201952 2 ddkilzer 2010-03-19 11:32:16 -0700 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. 201954 3 51175 joepeck 2010-03-19 11:45:17 -0700 Created attachment 51175 [PATCH] Temporary Fix This was reviewed on IRC and committed. 201956 4 joepeck 2010-03-19 11:46:59 -0700 I am going to leave this open and write a better patch tonight. 201958 5 51175 ddkilzer 2010-03-19 11:49:41 -0700 Comment on attachment 51175 [PATCH] Temporary Fix r=me 201959 6 ddkilzer 2010-03-19 11:50:37 -0700 (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> 202194 7 joepeck 2010-03-20 01:51:48 -0700 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. 202196 8 51220 joepeck 2010-03-20 01:54:07 -0700 Created attachment 51220 [PATCH] Bail in Default Handler before Attached 202197 9 joepeck 2010-03-20 01:54:39 -0700 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? 202198 10 51220 ddkilzer 2010-03-20 02:00:05 -0700 Comment on attachment 51220 [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): r=me 202200 11 joepeck 2010-03-20 02:09:17 -0700 Committed r56301 M WebCore/ChangeLog M WebCore/rendering/MediaControlElements.cpp M WebCore/rendering/RenderMedia.cpp r56301 = e0a505ccc4600f75ecae5b7003ad75cb7b6e3c24 (trunk) <http://trac.webkit.org/changeset/56301> 51175 2010-03-19 11:45:17 -0700 2010-03-20 01:54:07 -0700 [PATCH] Temporary Fix temp-fix text/plain 1414 joepeck ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YWEzNzVlMS4uNDYzMjM5YSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNiBAQAorMjAxMC0wMy0xOSAgSm9zZXBoIFBlY29y YXJvICA8am9lcGVja0B3ZWJraXQub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IERhdmlkIEtp bHplci4KKworICAgICAgICBBc3NlcnRpb24gZmFpbHVyZSBpbiBtZWRpYS92aWRlby1jb250cm9s cy13aXRoLW11dGF0aW9uLWV2ZW50LWhhbmRsZXIuaHRtbAorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzYzNzYKKworICAgICAgICBGaXggY3Jhc2hpbmcg dGVzdC4gVW5kZXJseWluZyB0aGUgbWVkaWEgZWxlbWVudCdzIHZvbHVtZSBzbGlkZXIgaXMgYQor ICAgICAgICByYW5nZSBpbnB1dCwgd2hpY2ggc2FuaXRpemVzIHZhbHVlcyBiZWZvcmUgZXhwZWN0 ZWQgb24gdGhlIG1lZGlhIGNvbnRyb2wuCisKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyTWVk aWEuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyTWVkaWE6OmNyZWF0ZVZvbHVtZVNsaWRl cik6CisKIDIwMTAtMDMtMTkgIEJyYWR5IEVpZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgogCiAg ICAgICAgIFJldmlld2VkIGJ5IERhcmluIEFkbGVyLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9yZW5k ZXJpbmcvUmVuZGVyTWVkaWEuY3BwIGIvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTWVkaWEuY3Bw CmluZGV4IDcxMDA0MzUuLjNkYzBkNmMgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvcmVuZGVyaW5nL1Jl bmRlck1lZGlhLmNwcAorKysgYi9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJNZWRpYS5jcHAKQEAg LTI2Myw4ICsyNjMsOCBAQCB2b2lkIFJlbmRlck1lZGlhOjpjcmVhdGVWb2x1bWVTbGlkZXIoKQog ewogICAgIEFTU0VSVCghbV92b2x1bWVTbGlkZXIpOwogICAgIG1fdm9sdW1lU2xpZGVyID0gbmV3 IE1lZGlhQ29udHJvbFZvbHVtZVNsaWRlckVsZW1lbnQoZG9jdW1lbnQoKSwgbWVkaWFFbGVtZW50 KCkpOwotICAgIG1fdm9sdW1lU2xpZGVyLT5zZXRBdHRyaWJ1dGUocHJlY2lzaW9uQXR0ciwgImZs b2F0Iik7CiAgICAgbV92b2x1bWVTbGlkZXItPnNldEF0dHJpYnV0ZShtYXhBdHRyLCAiMSIpOwor ICAgIG1fdm9sdW1lU2xpZGVyLT5zZXRBdHRyaWJ1dGUocHJlY2lzaW9uQXR0ciwgImZsb2F0Iik7 CiAgICAgbV92b2x1bWVTbGlkZXItPnNldEF0dHJpYnV0ZSh2YWx1ZUF0dHIsIFN0cmluZzo6bnVt YmVyKG1lZGlhRWxlbWVudCgpLT52b2x1bWUoKSkpOwogICAgIG1fdm9sdW1lU2xpZGVyLT5hdHRh Y2hUb1BhcmVudChtX3ZvbHVtZVNsaWRlckNvbnRhaW5lci5nZXQoKSk7CiB9Cg== 51220 2010-03-20 01:54:07 -0700 2010-03-20 02:00:05 -0700 [PATCH] Bail in Default Handler before Attached patch text/plain 2621 joepeck ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg Y2ZjZDI0Ny4uNjc0MmM5MCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOSBAQAorMjAxMC0wMy0yMCAgSm9zZXBoIFBlY29y YXJvICA8am9lcGVja0B3ZWJraXQub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIEFzc2VydGlvbiBmYWlsdXJlIGluIG1lZGlhL3ZpZGVvLWNvbnRy b2xzLXdpdGgtbXV0YXRpb24tZXZlbnQtaGFuZGxlci5odG1sCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zNjM3NgorCisgICAgICAgIEJyZWFrIGVhcmx5 ICh3aGVuIG5vdCBhdHRhY2hlZCkgaW4gdGhlIGRlZmF1bHRFdmVudEhhbmRsZXIgYmVmb3JlIHRo ZQorICAgICAgICBzbGlkZXIgaXMgY29tcGxldGVseSBzZXQgdXAuCisKKyAgICAgICAgKiByZW5k ZXJpbmcvTWVkaWFDb250cm9sRWxlbWVudHMuY3BwOgorICAgICAgICAoV2ViQ29yZTo6TWVkaWFD b250cm9sVGltZWxpbmVFbGVtZW50OjpkZWZhdWx0RXZlbnRIYW5kbGVyKToKKyAgICAgICAgKFdl YkNvcmU6Ok1lZGlhQ29udHJvbFZvbHVtZVNsaWRlckVsZW1lbnQ6OmRlZmF1bHRFdmVudEhhbmRs ZXIpOgorICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJNZWRpYS5jcHA6CisgICAgICAgIChXZWJD b3JlOjpSZW5kZXJNZWRpYTo6Y3JlYXRlVm9sdW1lU2xpZGVyKToKKwogMjAxMC0wMy0xOSAgU2hp bmljaGlybyBIYW1hamkgIDxoYW1hamlAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2Vk IGJ5IERhbiBCZXJuc3RlaW4uCmRpZmYgLS1naXQgYS9XZWJDb3JlL3JlbmRlcmluZy9NZWRpYUNv bnRyb2xFbGVtZW50cy5jcHAgYi9XZWJDb3JlL3JlbmRlcmluZy9NZWRpYUNvbnRyb2xFbGVtZW50 cy5jcHAKaW5kZXggMmI3YThjOC4uMTViZmQxOCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9yZW5kZXJp bmcvTWVkaWFDb250cm9sRWxlbWVudHMuY3BwCisrKyBiL1dlYkNvcmUvcmVuZGVyaW5nL01lZGlh Q29udHJvbEVsZW1lbnRzLmNwcApAQCAtNjE0LDYgKzYxNCw4IEBAIHZvaWQgTWVkaWFDb250cm9s VGltZWxpbmVFbGVtZW50OjpkZWZhdWx0RXZlbnRIYW5kbGVyKEV2ZW50KiBldmVudCkKICAgICAv LyBMZWZ0IGJ1dHRvbiBpcyAwLiBSZWplY3RzIG1vdXNlIGV2ZW50cyBub3QgZnJvbSBsZWZ0IGJ1 dHRvbi4KICAgICBpZiAoZXZlbnQtPmlzTW91c2VFdmVudCgpICYmIHN0YXRpY19jYXN0PE1vdXNl RXZlbnQqPihldmVudCktPmJ1dHRvbigpKQogICAgICAgICByZXR1cm47CisgICAgaWYgKCFhdHRh Y2hlZCgpKQorICAgICAgICByZXR1cm47CiAKICAgICBpZiAoZXZlbnQtPnR5cGUoKSA9PSBldmVu dE5hbWVzKCkubW91c2Vkb3duRXZlbnQpCiAgICAgICAgIG1fbWVkaWFFbGVtZW50LT5iZWdpblNj cnViYmluZygpOwpAQCAtNjYwLDYgKzY2Miw5IEBAIHZvaWQgTWVkaWFDb250cm9sVm9sdW1lU2xp ZGVyRWxlbWVudDo6ZGVmYXVsdEV2ZW50SGFuZGxlcihFdmVudCogZXZlbnQpCiAgICAgaWYgKGV2 ZW50LT5pc01vdXNlRXZlbnQoKSAmJiBzdGF0aWNfY2FzdDxNb3VzZUV2ZW50Kj4oZXZlbnQpLT5i dXR0b24oKSkKICAgICAgICAgcmV0dXJuOwogCisgICAgaWYgKCFhdHRhY2hlZCgpKQorICAgICAg ICByZXR1cm47CisKICAgICBNZWRpYUNvbnRyb2xJbnB1dEVsZW1lbnQ6OmRlZmF1bHRFdmVudEhh bmRsZXIoZXZlbnQpOwogCiAgICAgaWYgKGV2ZW50LT50eXBlKCkgPT0gZXZlbnROYW1lcygpLm1v dXNlb3ZlckV2ZW50IHx8IGV2ZW50LT50eXBlKCkgPT0gZXZlbnROYW1lcygpLm1vdXNlb3V0RXZl bnQgfHwgZXZlbnQtPnR5cGUoKSA9PSBldmVudE5hbWVzKCkubW91c2Vtb3ZlRXZlbnQpCmRpZmYg LS1naXQgYS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJNZWRpYS5jcHAgYi9XZWJDb3JlL3JlbmRl cmluZy9SZW5kZXJNZWRpYS5jcHAKaW5kZXggM2RjMGQ2Yy4uNzEwMDQzNSAxMDA2NDQKLS0tIGEv V2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTWVkaWEuY3BwCisrKyBiL1dlYkNvcmUvcmVuZGVyaW5n L1JlbmRlck1lZGlhLmNwcApAQCAtMjYzLDggKzI2Myw4IEBAIHZvaWQgUmVuZGVyTWVkaWE6OmNy ZWF0ZVZvbHVtZVNsaWRlcigpCiB7CiAgICAgQVNTRVJUKCFtX3ZvbHVtZVNsaWRlcik7CiAgICAg bV92b2x1bWVTbGlkZXIgPSBuZXcgTWVkaWFDb250cm9sVm9sdW1lU2xpZGVyRWxlbWVudChkb2N1 bWVudCgpLCBtZWRpYUVsZW1lbnQoKSk7Ci0gICAgbV92b2x1bWVTbGlkZXItPnNldEF0dHJpYnV0 ZShtYXhBdHRyLCAiMSIpOwogICAgIG1fdm9sdW1lU2xpZGVyLT5zZXRBdHRyaWJ1dGUocHJlY2lz aW9uQXR0ciwgImZsb2F0Iik7CisgICAgbV92b2x1bWVTbGlkZXItPnNldEF0dHJpYnV0ZShtYXhB dHRyLCAiMSIpOwogICAgIG1fdm9sdW1lU2xpZGVyLT5zZXRBdHRyaWJ1dGUodmFsdWVBdHRyLCBT dHJpbmc6Om51bWJlcihtZWRpYUVsZW1lbnQoKS0+dm9sdW1lKCkpKTsKICAgICBtX3ZvbHVtZVNs aWRlci0+YXR0YWNoVG9QYXJlbnQobV92b2x1bWVTbGlkZXJDb250YWluZXIuZ2V0KCkpOwogfQo=