102765 2012-11-19 21:29:38 -0800 Remove unneeded null check in NodeRendererFactory::createRendererIfNeeded 2012-11-19 22:29:39 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 esprehn esprehn bfulgham dglazkov morrita ojan tasak webkit.review.bot oldest_to_newest 771475 0 esprehn 2012-11-19 21:29:38 -0800 Remove unneeded null check in NodeRendererFactory::createRendererIfNeeded 771480 1 175136 esprehn 2012-11-19 21:36:03 -0800 Created attachment 175136 Patch 771491 2 175136 ojan 2012-11-19 21:49:55 -0800 Comment on attachment 175136 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175136&action=review > Source/WebCore/dom/NodeRenderingContext.cpp:255 > + ASSERT(m_context.parentRenderer()); Can you move this line to be above the if/else. It makes more sense to me to assert before the line that depends on the assert. 771492 3 ojan 2012-11-19 21:50:11 -0800 Actually, even better, but it directly in the else statement. 771496 4 bfulgham 2012-11-19 21:53:07 -0800 I was about to r- because I think the ASSERTs should be positioned higher in the code flow, but Ojan beat me to it. If m_context.parentRenderer() might ever ASSERT, then it's possible the new code will blow up before having a chance to assert. I also had a question about the behavior of parentRenderer(), which I think is probably NOT a problem, but I'd feel better if you confirmed it. > Source/WebCore/ChangeLog:10 > + so there's no reason to check for it again. Is it possible for parentRenderer() to return null, but parentFlowRenderer to be non-null? From a brief look it seems like the answer is no. But if I'm wrong, this change would modify the behavior. Can you please confirm that this is not a problem? 771499 5 esprehn 2012-11-19 21:57:15 -0800 (In reply to comment #3) > Actually, even better, but it directly in the else statement. I'm not actually asserting about the line in the else, I'm asserting about the rendererIsNeeded() on the following line after the assert. Calling that method on a context that doesn't have a style or a parentRenderer is always a mistake. Also if I put it in the else it makes it look like if isElementNode() is true and we don't go down the else that we may not have a renderer (which is never true). Also if we do go through that else we'll crash on the null dereference so there's no reason to ASSERT about it. 771500 6 esprehn 2012-11-19 22:01:41 -0800 (In reply to comment #4) > I was about to r- because I think the ASSERTs should be positioned higher in the code flow, but Ojan beat me to it. > > If m_context.parentRenderer() might ever ASSERT, then it's possible the new code will blow up before having a chance to assert. Yes, that was intended. We do this all over webkit, and it's even been requested by Apple on my patches before that I NOT assert about nulls and instead just let the program crash the null dereference. > > I also had a question about the behavior of parentRenderer(), which I think is probably NOT a problem, but I'd feel better if you confirmed it. > > > Source/WebCore/ChangeLog:10 > > + so there's no reason to check for it again. > > Is it possible for parentRenderer() to return null, but parentFlowRenderer to be non-null? From a brief look it seems like the answer is no. But if I'm wrong, this change would modify the behavior. Can you please confirm that this is not a problem? I don't understand what you're asking. parentRenderer never returns null in this code path because shouldCreateRenderer would have returned before we got here. There's no behavior change. 771508 7 175138 esprehn 2012-11-19 22:06:19 -0800 Created attachment 175138 Patch for landing 771509 8 esprehn 2012-11-19 22:06:45 -0800 (In reply to comment #7) > Created an attachment (id=175138) [details] > Patch for landing I moved the assert bove the if/else. 771513 9 bfulgham 2012-11-19 22:11:46 -0800 (In reply to comment #6) > (In reply to comment #4) > > Is it possible for parentRenderer() to return null, but parentFlowRenderer to be non-null? From a brief look it seems like the answer is no. But if I'm wrong, this change would modify the behavior. Can you please confirm that this is not a problem? > > I don't understand what you're asking. parentRenderer never returns null in this code path because shouldCreateRenderer would have returned before we got here. There's no behavior change. I actually went and looked at the routine, and see that you are correct. Thanks! 771525 10 175138 webkit.review.bot 2012-11-19 22:29:35 -0800 Comment on attachment 175138 Patch for landing Clearing flags on attachment: 175138 Committed r135252: <http://trac.webkit.org/changeset/135252> 771526 11 webkit.review.bot 2012-11-19 22:29:39 -0800 All reviewed patches have been landed. Closing bug. 175136 2012-11-19 21:36:03 -0800 2012-11-19 22:06:14 -0800 Patch bug-102765-20121120003352.patch text/plain 2078 esprehn U3VidmVyc2lvbiBSZXZpc2lvbjogMTM1MjQ1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNTgxODkzN2NhOGIwZmRi ZDA5MDIyMWJhNTNmYWRlODE0YmIzYTA3Yi4uN2YyNmJjNWI3ZTNlZWNmNmQ3MWVjMzU1NTEzN2Nk MGVlZTIzZjI5NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDEyLTExLTE5ICBFbGxp b3R0IFNwcmVobiAgPGVzcHJlaG5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJlbW92ZSB1bm5l ZWRlZCBudWxsIGNoZWNrIGluIE5vZGVSZW5kZXJlckZhY3Rvcnk6OmNyZWF0ZVJlbmRlcmVySWZO ZWVkZWQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEw Mjc2NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIElm IHBhcmVudFJlbmRlcmVyKCkgd2FzIG51bGwgdGhlbiB3ZSByZXR1cm4gZWFybHkgZnJvbQorICAg ICAgICBjcmVhdGVSZW5kZXJlcklmTmVlZGVkKCkgYmVjYXVzZSBzaG91bGRDcmVhdGVSZW5kZXJl cigpIHdpbGwgcmV0dXJuIGZhbHNlCisgICAgICAgIHNvIHRoZXJlJ3Mgbm8gcmVhc29uIHRvIGNo ZWNrIGZvciBpdCBhZ2Fpbi4KKworICAgICAgICBBZGRpdGlvbmFsbHkgaWYgZWl0aGVyIHBhcmVu dFJlbmRlcmVyKCkgb3Igc3R5bGUoKSBpcyBudWxsIHRoZW4KKyAgICAgICAgaW1wbGVtZW50YXRp b25zIG9mIHJlbmRlcmVySXNOZWVkZWQoKSB3aWxsIGNyYXNoLCBzbyBhZGQgYXNzZXJ0cyB0bwor ICAgICAgICBtYWtlIHRoaXMgYXNzdW1wdGlvbiBtb3JlIGNsZWFyLgorCisgICAgICAgIE5vIG5l dyB0ZXN0cywganVzdCBzaW1wbGlmaWNhdGlvbi4KKworICAgICAgICAqIGRvbS9Ob2RlUmVuZGVy aW5nQ29udGV4dC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpOb2RlUmVuZGVyZXJGYWN0b3J5Ojpj cmVhdGVSZW5kZXJlcklmTmVlZGVkKToKKwogMjAxMi0xMS0xOSAgWXVyeSBTZW1pa2hhdHNreSAg PHl1cnlzQGNocm9taXVtLm9yZz4KIAogICAgICAgICBVbnJldmlld2VkLiBGaXggQ2hyb21pdW0g RGVidWcgY29tcGlsYXRpb24gYWZ0ZXIgcjEzNTIzMC4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJD b3JlL2RvbS9Ob2RlUmVuZGVyaW5nQ29udGV4dC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9kb20vTm9k ZVJlbmRlcmluZ0NvbnRleHQuY3BwCmluZGV4IGQ3MmEyNjUzY2NlYmIzYTQzZjA4NGI0YzM2Y2Jm N2RkMzNiYTIxYjMuLjhhMWNhNmU4MDNjNzgyY2Q4MjNhNGQzODkyMDU4MDVmNTQ5Mzc4NjIgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2RvbS9Ob2RlUmVuZGVyaW5nQ29udGV4dC5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvZG9tL05vZGVSZW5kZXJpbmdDb250ZXh0LmNwcApAQCAtMjQ4LDgg KzI0OCwxMSBAQCB2b2lkIE5vZGVSZW5kZXJlckZhY3Rvcnk6OmNyZWF0ZVJlbmRlcmVySWZOZWVk ZWQoKQogICAgIEVsZW1lbnQqIGVsZW1lbnQgPSBub2RlLT5pc0VsZW1lbnROb2RlKCkgPyB0b0Vs ZW1lbnQobm9kZSkgOiAwOwogICAgIGlmIChlbGVtZW50KQogICAgICAgICBtX2NvbnRleHQuc2V0 U3R5bGUoZWxlbWVudC0+c3R5bGVGb3JSZW5kZXJlcigpKTsKLSAgICBlbHNlIGlmIChSZW5kZXJP YmplY3QqIHBhcmVudFJlbmRlcmVyID0gbV9jb250ZXh0LnBhcmVudFJlbmRlcmVyKCkpCi0gICAg ICAgIG1fY29udGV4dC5zZXRTdHlsZShwYXJlbnRSZW5kZXJlci0+c3R5bGUoKSk7CisgICAgZWxz ZQorICAgICAgICBtX2NvbnRleHQuc2V0U3R5bGUobV9jb250ZXh0LnBhcmVudFJlbmRlcmVyKCkt PnN0eWxlKCkpOworCisgICAgQVNTRVJUKG1fY29udGV4dC5zdHlsZSgpKTsKKyAgICBBU1NFUlQo bV9jb250ZXh0LnBhcmVudFJlbmRlcmVyKCkpOwogCiAgICAgaWYgKCFub2RlLT5yZW5kZXJlcklz TmVlZGVkKG1fY29udGV4dCkpIHsKICAgICAgICAgaWYgKGVsZW1lbnQgJiYgbV9jb250ZXh0LnN0 eWxlKCktPmFmZmVjdGVkQnlFbXB0eSgpKQo= 175138 2012-11-19 22:06:19 -0800 2012-11-19 22:29:35 -0800 Patch for landing bug-102765-20121120010408.patch text/plain 2140 esprehn U3VidmVyc2lvbiBSZXZpc2lvbjogMTM1MjQ1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNTgxODkzN2NhOGIwZmRi ZDA5MDIyMWJhNTNmYWRlODE0YmIzYTA3Yi4uOTE2NWM2YzVhOTE1MDk2NTBiZjc5ZjNmNjI2NDM0 OWIxNzZjY2EyMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDEyLTExLTE5ICBFbGxp b3R0IFNwcmVobiAgPGVzcHJlaG5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJlbW92ZSB1bm5l ZWRlZCBudWxsIGNoZWNrIGluIE5vZGVSZW5kZXJlckZhY3Rvcnk6OmNyZWF0ZVJlbmRlcmVySWZO ZWVkZWQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEw Mjc2NQorCisgICAgICAgIFJldmlld2VkIGJ5IE9qYW4gVmFmYWkuCisKKyAgICAgICAgSWYgcGFy ZW50UmVuZGVyZXIoKSB3YXMgbnVsbCB0aGVuIHdlIHJldHVybiBlYXJseSBmcm9tCisgICAgICAg IGNyZWF0ZVJlbmRlcmVySWZOZWVkZWQoKSBiZWNhdXNlIHNob3VsZENyZWF0ZVJlbmRlcmVyKCkg d2lsbCByZXR1cm4gZmFsc2UKKyAgICAgICAgc28gdGhlcmUncyBubyByZWFzb24gdG8gY2hlY2sg Zm9yIGl0IGFnYWluLgorCisgICAgICAgIEFkZGl0aW9uYWxseSBpZiBlaXRoZXIgcGFyZW50UmVu ZGVyZXIoKSBvciBzdHlsZSgpIGlzIG51bGwgdGhlbgorICAgICAgICBpbXBsZW1lbnRhdGlvbnMg b2YgcmVuZGVyZXJJc05lZWRlZCgpIHdpbGwgY3Jhc2gsIHNvIGFkZCBhc3NlcnRzIHRvCisgICAg ICAgIG1ha2UgdGhpcyBhc3N1bXB0aW9uIG1vcmUgY2xlYXIuCisKKyAgICAgICAgTm8gbmV3IHRl c3RzLCBqdXN0IHNpbXBsaWZpY2F0aW9uLgorCisgICAgICAgICogZG9tL05vZGVSZW5kZXJpbmdD b250ZXh0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Ok5vZGVSZW5kZXJlckZhY3Rvcnk6OmNyZWF0 ZVJlbmRlcmVySWZOZWVkZWQpOgorCiAyMDEyLTExLTE5ICBZdXJ5IFNlbWlraGF0c2t5ICA8eXVy eXNAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFVucmV2aWV3ZWQuIEZpeCBDaHJvbWl1bSBEZWJ1 ZyBjb21waWxhdGlvbiBhZnRlciByMTM1MjMwLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv ZG9tL05vZGVSZW5kZXJpbmdDb250ZXh0LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9Ob2RlUmVu ZGVyaW5nQ29udGV4dC5jcHAKaW5kZXggZDcyYTI2NTNjY2ViYjNhNDNmMDg0YjRjMzZjYmY3ZGQz M2JhMjFiMy4uN2I3OWJlMGQzZTJjZjU1ODA1MzcyZGZiMDFhMTNkZTZjNjFhNWVhYyAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvZG9tL05vZGVSZW5kZXJpbmdDb250ZXh0LmNwcAorKysgYi9T b3VyY2UvV2ViQ29yZS9kb20vTm9kZVJlbmRlcmluZ0NvbnRleHQuY3BwCkBAIC0yNDUsMTEgKzI0 NSwxNSBAQCB2b2lkIE5vZGVSZW5kZXJlckZhY3Rvcnk6OmNyZWF0ZVJlbmRlcmVySWZOZWVkZWQo KQogICAgIGlmICghbV9jb250ZXh0LnNob3VsZENyZWF0ZVJlbmRlcmVyKCkpCiAgICAgICAgIHJl dHVybjsKIAorICAgIEFTU0VSVChtX2NvbnRleHQucGFyZW50UmVuZGVyZXIoKSk7CisKICAgICBF bGVtZW50KiBlbGVtZW50ID0gbm9kZS0+aXNFbGVtZW50Tm9kZSgpID8gdG9FbGVtZW50KG5vZGUp IDogMDsKICAgICBpZiAoZWxlbWVudCkKICAgICAgICAgbV9jb250ZXh0LnNldFN0eWxlKGVsZW1l bnQtPnN0eWxlRm9yUmVuZGVyZXIoKSk7Ci0gICAgZWxzZSBpZiAoUmVuZGVyT2JqZWN0KiBwYXJl bnRSZW5kZXJlciA9IG1fY29udGV4dC5wYXJlbnRSZW5kZXJlcigpKQotICAgICAgICBtX2NvbnRl eHQuc2V0U3R5bGUocGFyZW50UmVuZGVyZXItPnN0eWxlKCkpOworICAgIGVsc2UKKyAgICAgICAg bV9jb250ZXh0LnNldFN0eWxlKG1fY29udGV4dC5wYXJlbnRSZW5kZXJlcigpLT5zdHlsZSgpKTsK KworICAgIEFTU0VSVChtX2NvbnRleHQuc3R5bGUoKSk7CiAKICAgICBpZiAoIW5vZGUtPnJlbmRl cmVySXNOZWVkZWQobV9jb250ZXh0KSkgewogICAgICAgICBpZiAoZWxlbWVudCAmJiBtX2NvbnRl eHQuc3R5bGUoKS0+YWZmZWN0ZWRCeUVtcHR5KCkpCg==