102761 2012-11-19 20:06:30 -0800 Call directly RenderBlock::deleteLineBoxTree 2013-04-15 14:24:17 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 igor.oliveira igor.oliveira bfulgham commit-queue eric esprehn+autocc inferno jchaffraix mitz ojan webkit.review.bot oldest_to_newest 771425 0 igor.oliveira 2012-11-19 20:06:30 -0800 Instead of implementing deleteLineBoxTree logic inside RenderBlock::determineStartPosition, we can reuse the code. 771432 1 175126 igor.oliveira 2012-11-19 20:12:51 -0800 Created attachment 175126 Proposed patch 771486 2 175126 bfulgham 2012-11-19 21:41:00 -0800 Comment on attachment 175126 Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=175126&action=review Certainly the coding of this change is fine. However, I am concerned that we will start seeing flakiness in the 'fast/repaint' test cases again. Please revise the ChangeLog (and/or the bug) to indicate why this is no longer a concern and I'll happily r+ it for you. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1814 > - // crashes for fast/repaint tests. What has changed since this comment was made that makes it possible to use 'deleteLineBoxTree' directly? Is the comment no longer true -- i.e., have other bug fixes have rendered it unnecessary to avoid calling the method directly? If so, please indicate this in the ChangeLog. Ideally, we would identify the change that fixed the original problem but that's not critical. On the other hand, if we have no reason to believe that the crash is now resolved, I don't think it wise to move to this new implementation. 771497 3 igor.oliveira 2012-11-19 21:53:44 -0800 The commentaries added in the changeset http://trac.webkit.org/changeset/86628 are not valid anymore. nextLineBox: InlineFlowBox* nextLineBox() const { return m_nextLineBox; } and nextRootBox(): RootInlineBox* nextRootBox() const { return static_cast<RootInlineBox*>(m_nextLineBox); } nextRootBox is just casting m_nextLineBox. Additionally, RenderLineBoxList::deleteLineBoxTree is doing the same thing that the old code is doing. The main difference is because we need to set curr = 0, because it is used later in the code. A conservative change will be instead of calling deleteLineBoxTree() we should call m_lineBoxes.deleteLineBoxTree(renderArena()); What do you think? (In reply to comment #2) > (From update of attachment 175126 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175126&action=review > > Certainly the coding of this change is fine. However, I am concerned that we will start seeing flakiness in the 'fast/repaint' test cases again. Please revise the ChangeLog (and/or the bug) to indicate why this is no longer a concern and I'll happily r+ it for you. > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1814 > > - // crashes for fast/repaint tests. > > What has changed since this comment was made that makes it possible to use 'deleteLineBoxTree' directly? Is the comment no longer true -- i.e., have other bug fixes have rendered it unnecessary to avoid calling the method directly? If so, please indicate this in the ChangeLog. Ideally, we would identify the change that fixed the original problem but that's not critical. > > On the other hand, if we have no reason to believe that the crash is now resolved, I don't think it wise to move to this new implementation. 771533 4 175143 igor.oliveira 2012-11-19 22:39:39 -0800 Created attachment 175143 Proposed patch Use a conservative approach. 772338 5 175143 tonikitoo 2012-11-20 17:52:36 -0800 Comment on attachment 175143 Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=175143&action=review > Source/WebCore/ChangeLog:9 > + RenderBlock::determineStartPosition, we can reuse the code. The commentaries added typo:comments 876065 6 175143 commit-queue 2013-04-15 14:24:15 -0700 Comment on attachment 175143 Proposed patch Clearing flags on attachment: 175143 Committed r148468: <http://trac.webkit.org/changeset/148468> 876066 7 commit-queue 2013-04-15 14:24:17 -0700 All reviewed patches have been landed. Closing bug. 175126 2012-11-19 20:12:51 -0800 2012-11-19 23:39:11 -0800 Proposed patch 0001-Instead-of-implementing-deleteLineBoxTree-logic-insi.patch text/plain 2275 igor.oliveira RnJvbSA5NjUzYzY4YmRiYzE3MDZmMWI0YjkxNDMzYTRhYmU4MGU2N2VkMjY4IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBJZ29yIE9saXZlaXJhIDxpZ29yLm9Ac2lzYS5zYW1zdW5nLmNv bT4KRGF0ZTogTW9uLCAxOSBOb3YgMjAxMiAxOToxNTozNCAtMDgwMApTdWJqZWN0OiBbUEFUQ0hd IEluc3RlYWQgb2YgaW1wbGVtZW50aW5nIGRlbGV0ZUxpbmVCb3hUcmVlIGxvZ2ljIGluc2lkZQog UmVuZGVyQmxvY2s6OmRldGVybWluZVN0YXJ0UG9zaXRpb24sIHdlIGNhbiByZXVzZSB0aGUgY29k ZS4KCi0tLQogU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nICAgICAgICAgICAgICAgICAgICAgICAg ICAgfCAgIDEzICsrKysrKysrKysrKysKIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJC bG9ja0xpbmVMYXlvdXQuY3BwIHwgICAxMyArKystLS0tLS0tLS0tCiAyIGZpbGVzIGNoYW5nZWQs IDE2IGluc2VydGlvbnMoKyksIDEwIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBiYzZmZWMx Li4xOTBmMzY4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTItMTEtMTkgIElnb3Ig T2xpdmVpcmEgIDxpZ29yLm9Ac2lzYS5zYW1zdW5nLmNvbT4KKworICAgICAgICBDYWxsIGRpcmVj dGx5IFJlbmRlckJsb2NrOjpkZWxldGVMaW5lQm94VHJlZQorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyNzYxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW5zdGVhZCBvZiBpbXBsZW1lbnRpbmcgZGVsZXRl TGluZUJveFRyZWUgbG9naWMgaW5zaWRlIAorICAgICAgICBSZW5kZXJCbG9jazo6ZGV0ZXJtaW5l U3RhcnRQb3NpdGlvbiwgd2UgY2FuIHJldXNlIHRoZSBjb2RlLgorCisgICAgICAgICogcmVuZGVy aW5nL1JlbmRlckJsb2NrTGluZUxheW91dC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJC bG9jazo6ZGV0ZXJtaW5lU3RhcnRQb3NpdGlvbik6CisKIDIwMTItMTEtMTkgIEtlbnRhcm8gSGFy YSAgPGhhcmFrZW5AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIEluIENvZGVHZW5lcmF0b3JHT2Jq ZWN0LnBtIHdlIHNob3VsZCByZW5hbWUgJGRhdGFOb2RlIHRvICRpbnRlcmZhY2UKZGlmZiAtLWdp dCBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJCbG9ja0xpbmVMYXlvdXQuY3BwIGIv U291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckJsb2NrTGluZUxheW91dC5jcHAKaW5kZXgg MmU1OGVhNi4uNTY1YTRjMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1Jl bmRlckJsb2NrTGluZUxheW91dC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1Jl bmRlckJsb2NrTGluZUxheW91dC5jcHAKQEAgLTE4MTAsMTYgKzE4MTAsOSBAQCBSb290SW5saW5l Qm94KiBSZW5kZXJCbG9jazo6ZGV0ZXJtaW5lU3RhcnRQb3NpdGlvbihMaW5lTGF5b3V0U3RhdGUm IGxheW91dFN0YXRlLAogICAgIH0KIAogICAgIGlmIChsYXlvdXRTdGF0ZS5pc0Z1bGxMYXlvdXQo KSkgewotICAgICAgICAvLyBGSVhNRTogVGhpcyBzaG91bGQganVzdCBjYWxsIGRlbGV0ZUxpbmVC b3hUcmVlLCBidXQgdGhhdCBjYXVzZXMKLSAgICAgICAgLy8gY3Jhc2hlcyBmb3IgZmFzdC9yZXBh aW50IHRlc3RzLgotICAgICAgICBSZW5kZXJBcmVuYSogYXJlbmEgPSByZW5kZXJBcmVuYSgpOwot ICAgICAgICBjdXJyID0gZmlyc3RSb290Qm94KCk7Ci0gICAgICAgIHdoaWxlIChjdXJyKSB7Ci0g ICAgICAgICAgICAvLyBOb3RlOiBUaGlzIHVzZXMgbmV4dFJvb3RCb3goKSBpbnN0ZWQgb2YgbmV4 dExpbmVCb3goKSBsaWtlIGRlbGV0ZUxpbmVCb3hUcmVlIGRvZXMuCi0gICAgICAgICAgICBSb290 SW5saW5lQm94KiBuZXh0ID0gY3Vyci0+bmV4dFJvb3RCb3goKTsKLSAgICAgICAgICAgIGN1cnIt PmRlbGV0ZUxpbmUoYXJlbmEpOwotICAgICAgICAgICAgY3VyciA9IG5leHQ7Ci0gICAgICAgIH0K KyAgICAgICAgZGVsZXRlTGluZUJveFRyZWUoKTsKKyAgICAgICAgY3VyciA9IDA7CisKICAgICAg ICAgQVNTRVJUKCFmaXJzdExpbmVCb3goKSAmJiAhbGFzdExpbmVCb3goKSk7CiAgICAgfSBlbHNl IHsKICAgICAgICAgaWYgKGN1cnIpIHsKLS0gCjEuNy43LjUgKEFwcGxlIEdpdC0yNikKCg== 175143 2012-11-19 22:39:39 -0800 2013-04-15 14:24:14 -0700 Proposed patch 0001-Instead-of-implementing-deleteLineBoxTree-logic-insi.patch text/plain 2491 igor.oliveira RnJvbSAxNzczODVhNWQ4NGI2NWVhOTNmNjZmOTU1ZWI5ZmUxZDI5YmIzMzQ5IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBJZ29yIE9saXZlaXJhIDxpZ29yLm9Ac2lzYS5zYW1zdW5nLmNv bT4KRGF0ZTogTW9uLCAxOSBOb3YgMjAxMiAxOToxNTozNCAtMDgwMApTdWJqZWN0OiBbUEFUQ0hd IEluc3RlYWQgb2YgaW1wbGVtZW50aW5nIGRlbGV0ZUxpbmVCb3hUcmVlIGxvZ2ljIGluc2lkZQog UmVuZGVyQmxvY2s6OmRldGVybWluZVN0YXJ0UG9zaXRpb24sIHdlIGNhbiByZXVzZSB0aGUgY29k ZS4KCi0tLQogU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nICAgICAgICAgICAgICAgICAgICAgICAg ICAgfCAgIDE1ICsrKysrKysrKysrKysrKwogU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRl ckJsb2NrTGluZUxheW91dC5jcHAgfCAgIDEzICsrKy0tLS0tLS0tLS0KIDIgZmlsZXMgY2hhbmdl ZCwgMTggaW5zZXJ0aW9ucygrKSwgMTAgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYkNvcmUvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCmluZGV4IGJjNmZl YzEuLmM4NjQxMDQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9T b3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOCBAQAorMjAxMi0xMS0xOSAgSWdv ciBPbGl2ZWlyYSAgPGlnb3Iub0BzaXNhLnNhbXN1bmcuY29tPgorCisgICAgICAgIENhbGwgZGly ZWN0bHkgUmVuZGVyQmxvY2s6OmRlbGV0ZUxpbmVCb3hUcmVlCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDI3NjEKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJbnN0ZWFkIG9mIGltcGxlbWVudGluZyBSZW5k ZXJMaW5lQm94TGlzdDo6ZGVsZXRlTGluZUJveFRyZWUgbG9naWMgaW5zaWRlIAorICAgICAgICBS ZW5kZXJCbG9jazo6ZGV0ZXJtaW5lU3RhcnRQb3NpdGlvbiwgd2UgY2FuIHJldXNlIHRoZSBjb2Rl LiBUaGUgY29tbWVudGFyaWVzIGFkZGVkCisgICAgICAgIGluIHRoZSBjaGFuZ2VzZXQgIzg2NjI4 IGFyZSBub3QgdmFsaWQgYW55bW9yZSwgbmV4dFJvb3RCb3ggaXMgY2FzdGluZyBtX25leHRMaW5l Qm94IAorICAgICAgICBhbmQgbmV4dExpbmVCb3ggaXMgcmV0dXJuaW5nIG1fbmV4dExpbmVCb3gu CisKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyQmxvY2tMaW5lTGF5b3V0LmNwcDoKKyAgICAg ICAgKFdlYkNvcmU6OlJlbmRlckJsb2NrOjpkZXRlcm1pbmVTdGFydFBvc2l0aW9uKToKKwogMjAx Mi0xMS0xOSAgS2VudGFybyBIYXJhICA8aGFyYWtlbkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAg SW4gQ29kZUdlbmVyYXRvckdPYmplY3QucG0gd2Ugc2hvdWxkIHJlbmFtZSAkZGF0YU5vZGUgdG8g JGludGVyZmFjZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckJs b2NrTGluZUxheW91dC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQmxvY2tM aW5lTGF5b3V0LmNwcAppbmRleCAyZTU4ZWE2Li43MTU4YWZmIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQmxvY2tMaW5lTGF5b3V0LmNwcAorKysgYi9Tb3VyY2Uv V2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQmxvY2tMaW5lTGF5b3V0LmNwcApAQCAtMTgxMCwxNiAr MTgxMCw5IEBAIFJvb3RJbmxpbmVCb3gqIFJlbmRlckJsb2NrOjpkZXRlcm1pbmVTdGFydFBvc2l0 aW9uKExpbmVMYXlvdXRTdGF0ZSYgbGF5b3V0U3RhdGUsCiAgICAgfQogCiAgICAgaWYgKGxheW91 dFN0YXRlLmlzRnVsbExheW91dCgpKSB7Ci0gICAgICAgIC8vIEZJWE1FOiBUaGlzIHNob3VsZCBq dXN0IGNhbGwgZGVsZXRlTGluZUJveFRyZWUsIGJ1dCB0aGF0IGNhdXNlcwotICAgICAgICAvLyBj cmFzaGVzIGZvciBmYXN0L3JlcGFpbnQgdGVzdHMuCi0gICAgICAgIFJlbmRlckFyZW5hKiBhcmVu YSA9IHJlbmRlckFyZW5hKCk7Ci0gICAgICAgIGN1cnIgPSBmaXJzdFJvb3RCb3goKTsKLSAgICAg ICAgd2hpbGUgKGN1cnIpIHsKLSAgICAgICAgICAgIC8vIE5vdGU6IFRoaXMgdXNlcyBuZXh0Um9v dEJveCgpIGluc3RlZCBvZiBuZXh0TGluZUJveCgpIGxpa2UgZGVsZXRlTGluZUJveFRyZWUgZG9l cy4KLSAgICAgICAgICAgIFJvb3RJbmxpbmVCb3gqIG5leHQgPSBjdXJyLT5uZXh0Um9vdEJveCgp OwotICAgICAgICAgICAgY3Vyci0+ZGVsZXRlTGluZShhcmVuYSk7Ci0gICAgICAgICAgICBjdXJy ID0gbmV4dDsKLSAgICAgICAgfQorICAgICAgICBtX2xpbmVCb3hlcy5kZWxldGVMaW5lQm94VHJl ZShyZW5kZXJBcmVuYSgpKTsKKyAgICAgICAgY3VyciA9IDA7CisKICAgICAgICAgQVNTRVJUKCFm aXJzdExpbmVCb3goKSAmJiAhbGFzdExpbmVCb3goKSk7CiAgICAgfSBlbHNlIHsKICAgICAgICAg aWYgKGN1cnIpIHsKLS0gCjEuNy43LjUgKEFwcGxlIEdpdC0yNikKCg==