128820 2014-02-14 09:11:52 -0800 [GTK] Fix hitting hasClass() assertion in debug with the new JS media controls 2014-02-18 08:55:55 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 calvaris calvaris b.long commit-queue mrobinson piotr.grad oldest_to_newest 980812 0 calvaris 2014-02-14 09:11:52 -0800 [GTK] Fix assertion hit in debug with the new JS media controls 980815 1 224223 calvaris 2014-02-14 09:18:12 -0800 Created attachment 224223 Patch 980818 2 224223 mrobinson 2014-02-14 09:22:24 -0800 Comment on attachment 224223 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224223&action=review > Source/WebCore/ChangeLog:4 > + [GTK] Fix assertion hit in debug with the new JS media controls > + https://bugs.webkit.org/show_bug.cgi?id=128820 What assertion is hit? > Source/WebCore/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). > + > + * platform/gtk/RenderThemeGtk.cpp: > + (WebCore::nodeHasClass): Check for hasClass() in order not to hit This probably needs a test. > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:121 > + if (node->isElementNode()) { > + Element* element = toElement(node); > + if (element->hasClass()) > + return element->classNames().contains(className); > + } > + return false; Early returns would be better. 980856 3 calvaris 2014-02-14 10:10:25 -0800 (In reply to comment #2) > > Source/WebCore/ChangeLog:4 > > + [GTK] Fix assertion hit in debug with the new JS media controls > > + https://bugs.webkit.org/show_bug.cgi?id=128820 > > What assertion is hit? hasClass(), it is said in the changes explanation. Do you want me to be more explicit? Where exactly? > > Source/WebCore/ChangeLog:9 > > + Reviewed by NOBODY (OOPS!). > > + > > + * platform/gtk/RenderThemeGtk.cpp: > > + (WebCore::nodeHasClass): Check for hasClass() in order not to hit > > This probably needs a test. There were quite many tests crashing because of this in debug mode. Actually the bot has an early exit because of those crashes: Exiting early after 28 crashes and 22 timeouts. 27309 tests run. 121 failures 9 new passes 11 missing results The results can be seen at: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug%20WK1/r164101%20(6827)/results.html I think the functionality is quite well covered with the current test set. I could add the info to the changelog instead. Do you still want me to add the test? > > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:121 > > + if (node->isElementNode()) { > > + Element* element = toElement(node); > > + if (element->hasClass()) > > + return element->classNames().contains(className); > > + } > > + return false; > > Early returns would be better. Ok. I'll change this. 981008 4 224257 calvaris 2014-02-14 15:12:47 -0800 Created attachment 224257 Patch 981845 5 224257 commit-queue 2014-02-18 01:44:02 -0800 Comment on attachment 224257 Patch Clearing flags on attachment: 224257 Committed r164277: <http://trac.webkit.org/changeset/164277> 981846 6 commit-queue 2014-02-18 01:44:04 -0800 All reviewed patches have been landed. Closing bug. 981953 7 piotr.grad 2014-02-18 08:55:55 -0800 *** Bug 128904 has been marked as a duplicate of this bug. *** 224223 2014-02-14 09:18:12 -0800 2014-02-14 15:12:39 -0800 Patch bug-128820-20140214181811.patch text/plain 1604 calvaris U3VidmVyc2lvbiBSZXZpc2lvbjogMTY0MDI0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjM0MDcxMWVkMDBlNTM1 NDdjYWNlNzM5NDQ5MDQyMzI2ZDVjOGVlMC4uZjBlNDQ3YjRhNmQ1MzE5NTg5M2RjYmM3Y2JkYTE5 ODYyODUyNDQ0YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDE0LTAyLTE0ICBYYWJp ZXIgUm9kcmlndWV6IENhbHZhciAgPGNhbHZhcmlzQGlnYWxpYS5jb20+CisKKyAgICAgICAgW0dU S10gRml4IGFzc2VydGlvbiBoaXQgaW4gZGVidWcgd2l0aCB0aGUgbmV3IEpTIG1lZGlhIGNvbnRy b2xzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjg4 MjAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBs YXRmb3JtL2d0ay9SZW5kZXJUaGVtZUd0ay5jcHA6CisgICAgICAgIChXZWJDb3JlOjpub2RlSGFz Q2xhc3MpOiBDaGVjayBmb3IgaGFzQ2xhc3MoKSBpbiBvcmRlciBub3QgdG8gaGl0CisgICAgICAg IHRoZSBhc3NlcnRpb24gd2hlbiBnZXR0aW5nIHRoZSBjbGFzc05hbWVzKCkgaW4gZGVidWcgbW9k ZS4KKwogMjAxNC0wMi0xMyAgWGFiaWVyIFJvZHJpZ3VleiBDYWx2YXIgIDxjYWx2YXJpc0BpZ2Fs aWEuY29tPgogCiAgICAgICAgIFtHVEtdIE1FRElBX0NPTlRST0xTX1NDUklQVCBzdXBwb3J0CmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ndGsvUmVuZGVyVGhlbWVHdGsuY3Bw IGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL1JlbmRlclRoZW1lR3RrLmNwcAppbmRleCBm ZDI5YmVlM2EzYzc5NDRhNWZhNWY3NjRmZDEzYWI0ZTk1NmYxZjc4Li4zNTZjM2U1ODhmZmU1Yzgw NmE2ZDA4NDAzMzYxMmM2NDY4NGI2MDVkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9ndGsvUmVuZGVyVGhlbWVHdGsuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L2d0ay9SZW5kZXJUaGVtZUd0ay5jcHAKQEAgLTExMyw3ICsxMTMsMTIgQEAgc3RhdGljIGJvb2wg bm9kZUhhc1BzZXVkbyhOb2RlKiBub2RlLCBjb25zdCBjaGFyKiBwc2V1ZG8pCiAKIHN0YXRpYyBi b29sIG5vZGVIYXNDbGFzcyhOb2RlKiBub2RlLCBjb25zdCBjaGFyKiBjbGFzc05hbWUpCiB7Ci0g ICAgcmV0dXJuIG5vZGUtPmlzRWxlbWVudE5vZGUoKSA/IHRvRWxlbWVudChub2RlKS0+Y2xhc3NO YW1lcygpLmNvbnRhaW5zKGNsYXNzTmFtZSkgOiBmYWxzZTsKKyAgICBpZiAobm9kZS0+aXNFbGVt ZW50Tm9kZSgpKSB7CisgICAgICAgIEVsZW1lbnQqIGVsZW1lbnQgPSB0b0VsZW1lbnQobm9kZSk7 CisgICAgICAgIGlmIChlbGVtZW50LT5oYXNDbGFzcygpKQorICAgICAgICAgICAgcmV0dXJuIGVs ZW1lbnQtPmNsYXNzTmFtZXMoKS5jb250YWlucyhjbGFzc05hbWUpOworICAgIH0KKyAgICByZXR1 cm4gZmFsc2U7CiB9CiAKIFBhc3NSZWZQdHI8UmVuZGVyVGhlbWU+IFJlbmRlclRoZW1lR3RrOjpj cmVhdGUoKQo= 224257 2014-02-14 15:12:47 -0800 2014-02-18 01:44:02 -0800 Patch bug-128820-20140215001245.patch text/plain 1901 calvaris U3VidmVyc2lvbiBSZXZpc2lvbjogMTY0MDI0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjM0MDcxMWVkMDBlNTM1 NDdjYWNlNzM5NDQ5MDQyMzI2ZDVjOGVlMC4uYWM4NDE1YThlYWEyNzgwMTA5N2M5NDUxZTNjMTYy N2JmMDI2Njk1MyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDE0LTAyLTE0ICBYYWJp ZXIgUm9kcmlndWV6IENhbHZhciAgPGNhbHZhcmlzQGlnYWxpYS5jb20+CisKKyAgICAgICAgW0dU S10gRml4IGhpdHRpbmcgaGFzQ2xhc3MoKSBhc3NlcnRpb24gaW4gZGVidWcgd2l0aCB0aGUgbmV3 IEpTIG1lZGlhIGNvbnRyb2xzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xMjg4MjAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBUaGUgY29kZSBpbnRyb2R1Y2VkIGF0IHIxNjQwMjQgY2F1c2VkIHRoZSBoaXQg b2YgaGFzQ2xhc3MoKQorICAgICAgICBhc3NlcnRpb24gd2hlbiBnZXR0aW5nIHRoZSBjbGFzc05h bWVzKCkgb2YgYW4gZWxlbWVudCB3aXRoIG5vCisgICAgICAgIGNsYXNzLiBOb3cgd2UgY2hlY2sg Zm9yIGl0IHRvIGF2b2lkIHRoZSBhc3NlcnRpb24uCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLCBj dXJyZW50IHNldCBkZXRlY3RzIHRoZSBjcmFzaCBpbiBtYW55IHRlc3RzLgorCisgICAgICAgICog cGxhdGZvcm0vZ3RrL1JlbmRlclRoZW1lR3RrLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Om5vZGVI YXNDbGFzcyk6IENoZWNrIGZvciBoYXNDbGFzcygpIGluIG9yZGVyIG5vdCB0byBoaXQKKyAgICAg ICAgdGhlIGFzc2VydGlvbiB3aGVuIGdldHRpbmcgdGhlIGNsYXNzTmFtZXMoKSBpbiBkZWJ1ZyBt b2RlLgorCiAyMDE0LTAyLTEzICBYYWJpZXIgUm9kcmlndWV6IENhbHZhciAgPGNhbHZhcmlzQGln YWxpYS5jb20+CiAKICAgICAgICAgW0dUS10gTUVESUFfQ09OVFJPTFNfU0NSSVBUIHN1cHBvcnQK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2d0ay9SZW5kZXJUaGVtZUd0ay5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ndGsvUmVuZGVyVGhlbWVHdGsuY3BwCmluZGV4 IGZkMjliZWUzYTNjNzk0NGE1ZmE1Zjc2NGZkMTNhYjRlOTU2ZjFmNzguLmM5ZDQ3NGVhN2M1YTFj NzU0MDYxMTBhODJmMmNjM2FkMmFhNTRiNWIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL2d0ay9SZW5kZXJUaGVtZUd0ay5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZv cm0vZ3RrL1JlbmRlclRoZW1lR3RrLmNwcApAQCAtMTEzLDcgKzExMywxNSBAQCBzdGF0aWMgYm9v bCBub2RlSGFzUHNldWRvKE5vZGUqIG5vZGUsIGNvbnN0IGNoYXIqIHBzZXVkbykKIAogc3RhdGlj IGJvb2wgbm9kZUhhc0NsYXNzKE5vZGUqIG5vZGUsIGNvbnN0IGNoYXIqIGNsYXNzTmFtZSkKIHsK LSAgICByZXR1cm4gbm9kZS0+aXNFbGVtZW50Tm9kZSgpID8gdG9FbGVtZW50KG5vZGUpLT5jbGFz c05hbWVzKCkuY29udGFpbnMoY2xhc3NOYW1lKSA6IGZhbHNlOworICAgIGlmICghbm9kZS0+aXNF bGVtZW50Tm9kZSgpKQorICAgICAgICByZXR1cm4gZmFsc2U7CisKKyAgICBFbGVtZW50KiBlbGVt ZW50ID0gdG9FbGVtZW50KG5vZGUpOworCisgICAgaWYgKCFlbGVtZW50LT5oYXNDbGFzcygpKQor ICAgICAgICByZXR1cm4gZmFsc2U7CisKKyAgICByZXR1cm4gZWxlbWVudC0+Y2xhc3NOYW1lcygp LmNvbnRhaW5zKGNsYXNzTmFtZSk7CiB9CiAKIFBhc3NSZWZQdHI8UmVuZGVyVGhlbWU+IFJlbmRl clRoZW1lR3RrOjpjcmVhdGUoKQo=