102975 2012-11-21 13:18:21 -0800 Node::createRenderer should never return null 2013-01-15 11:34:59 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 102986 103475 1 esprehn esprehn eric kling morrita ojan webkit.review.bot oldest_to_newest 773211 0 esprehn 2012-11-21 13:18:21 -0800 Node::createRenderer should never return null 773214 1 175507 esprehn 2012-11-21 13:22:24 -0800 Created attachment 175507 Patch 773236 2 175507 ojan 2012-11-21 13:39:49 -0800 Comment on attachment 175507 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175507&action=review > Source/WebCore/dom/NodeRenderingContext.cpp:234 > + ASSERT(newRenderer); There are some codepaths that ASSERT_NOT_REACHED and then return null. If this isn't actually a performance improvement, it might be better to be a bit more defensive here. How about: if (!newRenderer) { ASSERT_NOT_REACHED(); return; } 773242 3 esprehn 2012-11-21 13:47:30 -0800 (In reply to comment #2) > (From update of attachment 175507 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175507&action=review > > > Source/WebCore/dom/NodeRenderingContext.cpp:234 > > + ASSERT(newRenderer); > > There are some codepaths that ASSERT_NOT_REACHED and then return null. If this isn't actually a performance improvement, it might be better to be a bit more defensive here. How about: > if (!newRenderer) { > ASSERT_NOT_REACHED(); > return; > } The return value is required for the code to compile, we could return anything there. If you return true from rendererIsNeeded and then return 0 from createRenderer the program should crash in a release build. I don't think it makes sense to protect against this case. (Similarly returning null from styleForRenderer() is a fatal error and we assert about it right above this, but we don't protect you from developer error of doing that) 773244 4 esprehn 2012-11-21 13:50:27 -0800 (In reply to comment #3) > ... Btw, I don't feel very strongly about this, I can add the protection if you want. :) 773256 5 175517 esprehn 2012-11-21 14:01:38 -0800 Created attachment 175517 Patch Add return after assert 773260 6 175507 ojan 2012-11-21 14:03:34 -0800 Comment on attachment 175507 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175507&action=review >>> Source/WebCore/dom/NodeRenderingContext.cpp:234 >>> + ASSERT(newRenderer); >> >> There are some codepaths that ASSERT_NOT_REACHED and then return null. If this isn't actually a performance improvement, it might be better to be a bit more defensive here. How about: >> if (!newRenderer) { >> ASSERT_NOT_REACHED(); >> return; >> } > > The return value is required for the code to compile, we could return anything there. If you return true from rendererIsNeeded and then return 0 from createRenderer the program should crash in a release build. I don't think it makes sense to protect against this case. > > (Similarly returning null from styleForRenderer() is a fatal error and we assert about it right above this, but we don't protect you from developer error of doing that) Yeah. I'm on the fence. 773302 7 175517 webkit.review.bot 2012-11-21 14:36:43 -0800 Comment on attachment 175517 Patch Clearing flags on attachment: 175517 Committed r135437: <http://trac.webkit.org/changeset/135437> 773303 8 webkit.review.bot 2012-11-21 14:36:47 -0800 All reviewed patches have been landed. Closing bug. 773369 9 webkit.review.bot 2012-11-21 15:46:37 -0800 Re-opened since this is blocked by bug 102986 774688 10 esprehn 2012-11-23 13:02:07 -0800 So it turns out there's a bug my CL uncovered: Node::rendererIsNeeded has: (document()->documentElement() == this) || (context.style()->display() != NONE); Element::createRenderer has a special case for this: // Ignore display: none on root elements. Force a display of block in that case. if (document()->documentElement() == this && style->display() == NONE) { RenderBlock* result = new (arena) RenderBlock(this); ... } but then in HTMLElement::createRenderer we don't call super: return RenderObject::createObject(this, style); so in an HTML document display: none on the root element is NOT ignored! This was introduced way back in https://trac.webkit.org/changeset/21405 when adding a special case for <wbr> elements in HTMLElement::createRenderer. 807511 11 esprehn 2013-01-15 11:34:59 -0800 (In reply to comment #10) > ... > so in an HTML document display: none on the root element is NOT ignored! > I fixed the bug with the documentElement in r136331, but having dug deeper I think it'd be a time consuming task to make createRenderer never return null as in this original bug, and I'm not sure it has a lot of benefits since we'd still need the if statement check in NodeRenderingContext. 175507 2012-11-21 13:22:24 -0800 2012-11-21 14:03:34 -0800 Patch bug-102975-20121121162010.patch text/plain 2624 esprehn U3VidmVyc2lvbiBSZXZpc2lvbjogMTM1NDIyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjE4ZjQ1YjhjODEzYzAy OGI1NTNhZmRjYjA1YTE0OTJmMjM4MzQxZi4uZDY5YjFjNTZhNDYxYjE1ODk3ZWYzZWM3MTEyODQ3 MTRjYmY5NzEyMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDEyLTExLTIxICBFbGxp b3R0IFNwcmVobiAgPGVzcHJlaG5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIE5vZGU6OmNyZWF0 ZVJlbmRlcmVyIHNob3VsZCBuZXZlciByZXR1cm4gbnVsbAorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyOTc1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm9kZTo6Y3JlYXRlUmVuZGVyZXIgc2hvdWxkIG5l dmVyIHJldHVybiBudWxsLCBpbnN0ZWFkIE5vZGU6OnJlbmRlcmVySXNOZWVkZWQKKyAgICAgICAg c2hvdWxkIHJldHVybiBmYWxzZSBpZiBhIG5vZGUgZG9lc24ndCB3aXNoIHRvIGhhdmUgYSByZW5k ZXJlci4gVGhpcyBtZXRob2QKKyAgICAgICAgbmV2ZXIgYWN0dWFsbHkgcmV0dXJucyBudWxsIHJp Z2h0IG5vdywgYnV0IHRoZSBjb2RlIG1ha2VzIGl0IGxvb2sgbGlrZSBpdAorICAgICAgICBjb3Vs ZC4KKworICAgICAgICBObyBuZXcgdGVzdHMsIGp1c3QgcmVmYWN0b3JpbmcuCisKKyAgICAgICAg KiBkb20vTm9kZVJlbmRlcmluZ0NvbnRleHQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6Tm9kZVJl bmRlcmluZ0NvbnRleHQ6OmNyZWF0ZVJlbmRlcmVySWZOZWVkZWQpOgorICAgICAgICAqIHJlbmRl cmluZy9SZW5kZXJPYmplY3QuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyT2JqZWN0Ojpj cmVhdGVPYmplY3QpOgorCiAyMDEyLTExLTIxICBBbmRyZWFzIEtsaW5nICA8YWtsaW5nQGFwcGxl LmNvbT4KIAogICAgICAgICBNYWtlIGl0IHBvc3NpYmxlIGZvciBlbGVtZW50cyB3aXRoIGRpZmZl cmVudCB0YWcgbmFtZXMgdG8gc2hhcmUgYXR0cmlidXRlIGRhdGEuCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViQ29yZS9kb20vTm9kZVJlbmRlcmluZ0NvbnRleHQuY3BwIGIvU291cmNlL1dlYkNvcmUv ZG9tL05vZGVSZW5kZXJpbmdDb250ZXh0LmNwcAppbmRleCAwMmIxYTNjMjhmYzA0MzI5MDRjMTVl NTE0MDkyNGI5OTZhOTA4ZjM4Li4wYzhjOTMwZmY5NmM2YjllMzY4NDNjZjI2MzVmY2RhNTQwNDVm NWE4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9kb20vTm9kZVJlbmRlcmluZ0NvbnRleHQu Y3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2RvbS9Ob2RlUmVuZGVyaW5nQ29udGV4dC5jcHAKQEAg LTIzMSw4ICsyMzEsOCBAQCB2b2lkIE5vZGVSZW5kZXJpbmdDb250ZXh0OjpjcmVhdGVSZW5kZXJl cklmTmVlZGVkKCkKIAogICAgIERvY3VtZW50KiBkb2N1bWVudCA9IG1fbm9kZS0+ZG9jdW1lbnQo KTsKICAgICBSZW5kZXJPYmplY3QqIG5ld1JlbmRlcmVyID0gbV9ub2RlLT5jcmVhdGVSZW5kZXJl cihkb2N1bWVudC0+cmVuZGVyQXJlbmEoKSwgbV9zdHlsZS5nZXQoKSk7Ci0gICAgaWYgKCFuZXdS ZW5kZXJlcikKLSAgICAgICAgcmV0dXJuOworICAgIEFTU0VSVChuZXdSZW5kZXJlcik7CisKICAg ICBpZiAoIXBhcmVudFJlbmRlcmVyLT5pc0NoaWxkQWxsb3dlZChuZXdSZW5kZXJlciwgbV9zdHls ZS5nZXQoKSkpIHsKICAgICAgICAgbmV3UmVuZGVyZXItPmRlc3Ryb3koKTsKICAgICAgICAgcmV0 dXJuOwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlck9iamVjdC5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyT2JqZWN0LmNwcAppbmRleCAxNWRj ZGQwNTgyOWU0ZTc2ZDhhMTQyNzZmMWJhN2Y2NDg0Yzc3NzIwLi4xNWQyMWE2OThkODQ3YWQ5YTli ZTE5NzlhMTU2NzFkYzMyMjkwZjc2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJp bmcvUmVuZGVyT2JqZWN0LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVy T2JqZWN0LmNwcApAQCAtMTcwLDYgKzE3MCw3IEBAIFJlbmRlck9iamVjdCogUmVuZGVyT2JqZWN0 OjpjcmVhdGVPYmplY3QoTm9kZSogbm9kZSwgUmVuZGVyU3R5bGUqIHN0eWxlKQogICAgICAgICBy ZXR1cm4gbmV3IChhcmVuYSkgUmVuZGVyUmVnaW9uKG5vZGUsIDApOwogICAgIHN3aXRjaCAoc3R5 bGUtPmRpc3BsYXkoKSkgewogICAgIGNhc2UgTk9ORToKKyAgICAgICAgQVNTRVJUX05PVF9SRUFD SEVEKCk7CiAgICAgICAgIHJldHVybiAwOwogICAgIGNhc2UgSU5MSU5FOgogICAgICAgICByZXR1 cm4gbmV3IChhcmVuYSkgUmVuZGVySW5saW5lKG5vZGUpOwpAQCAtMjA5LDYgKzIxMCw3IEBAIFJl bmRlck9iamVjdCogUmVuZGVyT2JqZWN0OjpjcmVhdGVPYmplY3QoTm9kZSogbm9kZSwgUmVuZGVy U3R5bGUqIHN0eWxlKQogICAgICAgICByZXR1cm4gbmV3IChhcmVuYSkgUmVuZGVyR3JpZChub2Rl KTsKICAgICB9CiAKKyAgICBBU1NFUlRfTk9UX1JFQUNIRUQoKTsKICAgICByZXR1cm4gMDsKIH0K IAo= 175517 2012-11-21 14:01:38 -0800 2012-11-21 14:36:43 -0800 Patch bug-102975-20121121165924.patch text/plain 2643 esprehn U3VidmVyc2lvbiBSZXZpc2lvbjogMTM1NDMwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNGZlMWNkOThjNGQyODdi YTM2YTdhNmU2ZTlhMTE2Y2QyY2QxMDkyZi4uMjYyYmE3NzNiOGJhYTRlM2M3ZTI2ZTY4YTg3YTA3 ZWM5N2QxYTNjOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDEyLTExLTIxICBFbGxp b3R0IFNwcmVobiAgPGVzcHJlaG5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIE5vZGU6OmNyZWF0 ZVJlbmRlcmVyIHNob3VsZCBuZXZlciByZXR1cm4gbnVsbAorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyOTc1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm9kZTo6Y3JlYXRlUmVuZGVyZXIgc2hvdWxkIG5l dmVyIHJldHVybiBudWxsLCBpbnN0ZWFkIE5vZGU6OnJlbmRlcmVySXNOZWVkZWQKKyAgICAgICAg c2hvdWxkIHJldHVybiBmYWxzZSBpZiBhIG5vZGUgZG9lc24ndCB3aXNoIHRvIGhhdmUgYSByZW5k ZXJlci4gVGhpcyBtZXRob2QKKyAgICAgICAgbmV2ZXIgYWN0dWFsbHkgcmV0dXJucyBudWxsIHJp Z2h0IG5vdywgYnV0IHRoZSBjb2RlIG1ha2VzIGl0IGxvb2sgbGlrZSBpdAorICAgICAgICBjb3Vs ZC4KKworICAgICAgICBObyBuZXcgdGVzdHMsIGp1c3QgcmVmYWN0b3JpbmcuCisKKyAgICAgICAg KiBkb20vTm9kZVJlbmRlcmluZ0NvbnRleHQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6Tm9kZVJl bmRlcmluZ0NvbnRleHQ6OmNyZWF0ZVJlbmRlcmVySWZOZWVkZWQpOgorICAgICAgICAqIHJlbmRl cmluZy9SZW5kZXJPYmplY3QuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyT2JqZWN0Ojpj cmVhdGVPYmplY3QpOgorCiAyMDEyLTExLTIwICBSeW9zdWtlIE5pd2EgIDxybml3YUB3ZWJraXQu b3JnPgogCiAgICAgICAgIEhUTUxDb2xsZWN0aW9uIHNob3VsZCB1c2UgdGhlIHNhbWUgc3RvcmFn ZSBhcyBEeW5hbWljTm9kZUxpc3QKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2RvbS9Ob2Rl UmVuZGVyaW5nQ29udGV4dC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9kb20vTm9kZVJlbmRlcmluZ0Nv bnRleHQuY3BwCmluZGV4IDAyYjFhM2MyOGZjMDQzMjkwNGMxNWU1MTQwOTI0Yjk5NmE5MDhmMzgu LjAwMjlmY2U0ODJlOWM5MTRmOTI3M2E2NWNiYzVhMGMzMjcxMGM3MWYgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL2RvbS9Ob2RlUmVuZGVyaW5nQ29udGV4dC5jcHAKKysrIGIvU291cmNlL1dl YkNvcmUvZG9tL05vZGVSZW5kZXJpbmdDb250ZXh0LmNwcApAQCAtMjMxLDggKzIzMSwxMSBAQCB2 b2lkIE5vZGVSZW5kZXJpbmdDb250ZXh0OjpjcmVhdGVSZW5kZXJlcklmTmVlZGVkKCkKIAogICAg IERvY3VtZW50KiBkb2N1bWVudCA9IG1fbm9kZS0+ZG9jdW1lbnQoKTsKICAgICBSZW5kZXJPYmpl Y3QqIG5ld1JlbmRlcmVyID0gbV9ub2RlLT5jcmVhdGVSZW5kZXJlcihkb2N1bWVudC0+cmVuZGVy QXJlbmEoKSwgbV9zdHlsZS5nZXQoKSk7Ci0gICAgaWYgKCFuZXdSZW5kZXJlcikKKyAgICBpZiAo IW5ld1JlbmRlcmVyKSB7CisgICAgICAgIEFTU0VSVF9OT1RfUkVBQ0hFRCgpOwogICAgICAgICBy ZXR1cm47CisgICAgfQorCiAgICAgaWYgKCFwYXJlbnRSZW5kZXJlci0+aXNDaGlsZEFsbG93ZWQo bmV3UmVuZGVyZXIsIG1fc3R5bGUuZ2V0KCkpKSB7CiAgICAgICAgIG5ld1JlbmRlcmVyLT5kZXN0 cm95KCk7CiAgICAgICAgIHJldHVybjsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3JlbmRl cmluZy9SZW5kZXJPYmplY3QuY3BwIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlck9i amVjdC5jcHAKaW5kZXggMTVkY2RkMDU4MjllNGU3NmQ4YTE0Mjc2ZjFiYTdmNjQ4NGM3NzcyMC4u MTVkMjFhNjk4ZDg0N2FkOWE5YmUxOTc5YTE1NjcxZGMzMjI5MGY3NiAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlck9iamVjdC5jcHAKKysrIGIvU291cmNlL1dlYkNv cmUvcmVuZGVyaW5nL1JlbmRlck9iamVjdC5jcHAKQEAgLTE3MCw2ICsxNzAsNyBAQCBSZW5kZXJP YmplY3QqIFJlbmRlck9iamVjdDo6Y3JlYXRlT2JqZWN0KE5vZGUqIG5vZGUsIFJlbmRlclN0eWxl KiBzdHlsZSkKICAgICAgICAgcmV0dXJuIG5ldyAoYXJlbmEpIFJlbmRlclJlZ2lvbihub2RlLCAw KTsKICAgICBzd2l0Y2ggKHN0eWxlLT5kaXNwbGF5KCkpIHsKICAgICBjYXNlIE5PTkU6CisgICAg ICAgIEFTU0VSVF9OT1RfUkVBQ0hFRCgpOwogICAgICAgICByZXR1cm4gMDsKICAgICBjYXNlIElO TElORToKICAgICAgICAgcmV0dXJuIG5ldyAoYXJlbmEpIFJlbmRlcklubGluZShub2RlKTsKQEAg LTIwOSw2ICsyMTAsNyBAQCBSZW5kZXJPYmplY3QqIFJlbmRlck9iamVjdDo6Y3JlYXRlT2JqZWN0 KE5vZGUqIG5vZGUsIFJlbmRlclN0eWxlKiBzdHlsZSkKICAgICAgICAgcmV0dXJuIG5ldyAoYXJl bmEpIFJlbmRlckdyaWQobm9kZSk7CiAgICAgfQogCisgICAgQVNTRVJUX05PVF9SRUFDSEVEKCk7 CiAgICAgcmV0dXJuIDA7CiB9CiAK