146038 2015-06-16 17:09:12 -0700 Position::findParent() should take a reference 2015-06-17 17:50:33 -0700 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jhoneycutt jhoneycutt commit-queue esprehn+autocc kangil.han oldest_to_newest 1102403 0 jhoneycutt 2015-06-16 17:09:12 -0700 Position::findParent() should take a reference. 1102404 1 254982 jhoneycutt 2015-06-16 17:10:31 -0700 Created attachment 254982 Patch 1102451 2 254982 darin 2015-06-16 20:07:16 -0700 Comment on attachment 254982 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254982&action=review > Source/WebCore/dom/Position.cpp:165 > + return findParent(*m_anchorNode.get()); No need for .get() here. > Source/WebCore/dom/Position.cpp:225 > + if (findParent(*m_anchorNode.get()) && (editingIgnoresContent(m_anchorNode.get()) || isRenderedTable(m_anchorNode.get()))) No need for .get() in the *m_anchorNode part. > Source/WebCore/dom/Position.cpp:314 > + if (!node) > + return *this; Looks like this fixes a bug. Can we make a test that shows this was broken before? > Source/WebCore/dom/Position.cpp:367 > + if (!node) > + return *this; Looks like this fixes a bug. Can we make a test that shows this was broken before? 1102723 3 jhoneycutt 2015-06-17 17:39:26 -0700 (In reply to comment #2) > Comment on attachment 254982 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254982&action=review > > > Source/WebCore/dom/Position.cpp:165 > > + return findParent(*m_anchorNode.get()); > > No need for .get() here. > > > Source/WebCore/dom/Position.cpp:225 > > + if (findParent(*m_anchorNode.get()) && (editingIgnoresContent(m_anchorNode.get()) || isRenderedTable(m_anchorNode.get()))) > > No need for .get() in the *m_anchorNode part. Fixed. > > > Source/WebCore/dom/Position.cpp:314 > > + if (!node) > > + return *this; > > Looks like this fixes a bug. Can we make a test that shows this was broken > before? > > > Source/WebCore/dom/Position.cpp:367 > > + if (!node) > > + return *this; > > Looks like this fixes a bug. Can we make a test that shows this was broken > before? I couldn’t find a way to make this crash. I looked for existing bug reports and found <rdar://problem/21026135>, but I was unable to reproduce a crash given the repro steps in the bug. Thanks for the review! 1102728 4 jhoneycutt 2015-06-17 17:50:33 -0700 Committed r185682: <http://trac.webkit.org/changeset/185682> 254982 2015-06-16 17:10:31 -0700 2015-06-16 20:07:16 -0700 Patch bug-146038-20150616170955.patch text/plain 5128 jhoneycutt U3VidmVyc2lvbiBSZXZpc2lvbjogMTg1NjEzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggY2UyMGUyZjgxMmRhOTk5 ZjBhZTA2ZjA0MDE2YmMxMjExNDNiMzA0Ny4uMTZkZDVmMmQwODhhYjE3ODliYjRhYjc5OGM0NWJm NDI4ZjFjNGFiZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI3IEBACisyMDE1LTA2LTE2ICBKb24g SG9uZXljdXR0ICA8amhvbmV5Y3V0dEBhcHBsZS5jb20+CisKKyAgICAgICAgUG9zaXRpb246OmZp bmRQYXJlbnQoKSBzaG91bGQgdGFrZSBhIHJlZmVyZW5jZQorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ2MDM4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBkb20vUG9zaXRpb24uY3BwOgorICAgICAgICAo V2ViQ29yZTo6UG9zaXRpb246OmNvbnRhaW5lck5vZGUpOgorICAgICAgICAoV2ViQ29yZTo6UG9z aXRpb246OnBhcmVudEFuY2hvcmVkRXF1aXZhbGVudCk6CisgICAgICAgIFBhc3MgYSByZWZlcmVu Y2U7IHRoZXJlIGlzIGFscmVhZHkgYSBudWxsIGNoZWNrLgorICAgICAgICAoV2ViQ29yZTo6UG9z aXRpb246OnByZXZpb3VzKToKKyAgICAgICAgQWRkIGEgbWlzc2luZyBudWxsIGNoZWNrLiBDb2Rl IGJlbG93IHRoaXMgZXhwZWN0cyB0aGF0IG5vZGUgaXMgbm9uLW51bGwuCisgICAgICAgIChXZWJD b3JlOjpQb3NpdGlvbjo6bmV4dCk6CisgICAgICAgIERpdHRvLgorICAgICAgICAoV2ViQ29yZTo6 UG9zaXRpb246OmF0U3RhcnRPZlRyZWUpOgorICAgICAgICAoV2ViQ29yZTo6UG9zaXRpb246OmF0 RW5kT2ZUcmVlKToKKyAgICAgICAgUGFzcyBhIHJlZmVyZW5jZS4KKyAgICAgICAgKFdlYkNvcmU6 OlBvc2l0aW9uOjpmaW5kUGFyZW50KToKKyAgICAgICAgQ2hhbmdlZCB0byB0YWtlIGEgcmVmZXJl bmNlLgorCisgICAgICAgICogZG9tL1Bvc2l0aW9uLmg6CisgICAgICAgIERpdHRvLgorCiAyMDE1 LTA2LTE1ICBKb24gSG9uZXljdXR0ICA8amhvbmV5Y3V0dEBhcHBsZS5jb20+CiAKICAgICAgICAg W2lPU10gQ3Jhc2ggbG9uZyBwcmVzc2luZyBvbiA8aW5wdXQgdHlwZT1maWxlPgpkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvZG9tL1Bvc2l0aW9uLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9Q b3NpdGlvbi5jcHAKaW5kZXggZWQ5ZTI0ODBhNmYzNzFmMTcxNzViODRjN2I0NmNjYWM0ZTA5YTkw YS4uN2QxZGY1NDIwZDA0MTI5NDBiNDBhNmY0MjRjMWQ2YjgwMDg3MDFkZiAxMDA2NDQKLS0tIGEv U291cmNlL1dlYkNvcmUvZG9tL1Bvc2l0aW9uLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9kb20v UG9zaXRpb24uY3BwCkBAIC0xNjIsNyArMTYyLDcgQEAgTm9kZSogUG9zaXRpb246OmNvbnRhaW5l ck5vZGUoKSBjb25zdAogICAgICAgICByZXR1cm4gbV9hbmNob3JOb2RlLmdldCgpOwogICAgIGNh c2UgUG9zaXRpb25Jc0JlZm9yZUFuY2hvcjoKICAgICBjYXNlIFBvc2l0aW9uSXNBZnRlckFuY2hv cjoKLSAgICAgICAgcmV0dXJuIGZpbmRQYXJlbnQobV9hbmNob3JOb2RlLmdldCgpKTsKKyAgICAg ICAgcmV0dXJuIGZpbmRQYXJlbnQoKm1fYW5jaG9yTm9kZS5nZXQoKSk7CiAgICAgfQogICAgIEFT U0VSVF9OT1RfUkVBQ0hFRCgpOwogICAgIHJldHVybiBudWxscHRyOwpAQCAtMjIyLDcgKzIyMiw3 IEBAIFBvc2l0aW9uIFBvc2l0aW9uOjpwYXJlbnRBbmNob3JlZEVxdWl2YWxlbnQoKSBjb25zdAog ICAgIAogICAgIC8vIEZJWE1FOiBUaGlzIHNob3VsZCBvbmx5IGJlIG5lY2Vzc2FyeSBmb3IgbGVn YWN5IHBvc2l0aW9ucywgYnV0IGlzIGFsc28gbmVlZGVkIGZvciBwb3NpdGlvbnMgYmVmb3JlIGFu ZCBhZnRlciBUYWJsZXMKICAgICBpZiAobV9vZmZzZXQgPD0gMCAmJiAobV9hbmNob3JUeXBlICE9 IFBvc2l0aW9uSXNBZnRlckFuY2hvciAmJiBtX2FuY2hvclR5cGUgIT0gUG9zaXRpb25Jc0FmdGVy Q2hpbGRyZW4pKSB7Ci0gICAgICAgIGlmIChmaW5kUGFyZW50KG1fYW5jaG9yTm9kZS5nZXQoKSkg JiYgKGVkaXRpbmdJZ25vcmVzQ29udGVudChtX2FuY2hvck5vZGUuZ2V0KCkpIHx8IGlzUmVuZGVy ZWRUYWJsZShtX2FuY2hvck5vZGUuZ2V0KCkpKSkKKyAgICAgICAgaWYgKGZpbmRQYXJlbnQoKm1f YW5jaG9yTm9kZS5nZXQoKSkgJiYgKGVkaXRpbmdJZ25vcmVzQ29udGVudChtX2FuY2hvck5vZGUu Z2V0KCkpIHx8IGlzUmVuZGVyZWRUYWJsZShtX2FuY2hvck5vZGUuZ2V0KCkpKSkKICAgICAgICAg ICAgIHJldHVybiBwb3NpdGlvbkluUGFyZW50QmVmb3JlTm9kZShtX2FuY2hvck5vZGUuZ2V0KCkp OwogICAgICAgICByZXR1cm4gUG9zaXRpb24obV9hbmNob3JOb2RlLmdldCgpLCAwLCBQb3NpdGlv bklzT2Zmc2V0SW5BbmNob3IpOwogICAgIH0KQEAgLTMxMCw2ICszMTAsOSBAQCBQb3NpdGlvbiBQ b3NpdGlvbjo6cHJldmlvdXMoUG9zaXRpb25Nb3ZlVHlwZSBtb3ZlVHlwZSkgY29uc3QKIAogICAg IGlmIChhbmNob3JUeXBlKCkgPT0gUG9zaXRpb25Jc0JlZm9yZUFuY2hvcikgewogICAgICAgICBu b2RlID0gY29udGFpbmVyTm9kZSgpOworICAgICAgICBpZiAoIW5vZGUpCisgICAgICAgICAgICBy ZXR1cm4gKnRoaXM7CisKICAgICAgICAgb2Zmc2V0ID0gY29tcHV0ZU9mZnNldEluQ29udGFpbmVy Tm9kZSgpOwogICAgIH0KIApAQCAtMzMyLDcgKzMzNSw3IEBAIFBvc2l0aW9uIFBvc2l0aW9uOjpw cmV2aW91cyhQb3NpdGlvbk1vdmVUeXBlIG1vdmVUeXBlKSBjb25zdAogICAgICAgICB9CiAgICAg fQogCi0gICAgQ29udGFpbmVyTm9kZSogcGFyZW50ID0gZmluZFBhcmVudChub2RlKTsKKyAgICBD b250YWluZXJOb2RlKiBwYXJlbnQgPSBmaW5kUGFyZW50KCpub2RlKTsKICAgICBpZiAoIXBhcmVu dCkKICAgICAgICAgcmV0dXJuICp0aGlzOwogCkBAIC0zNjAsNiArMzYzLDkgQEAgUG9zaXRpb24g UG9zaXRpb246Om5leHQoUG9zaXRpb25Nb3ZlVHlwZSBtb3ZlVHlwZSkgY29uc3QKIAogICAgIGlm IChhbmNob3JUeXBlKCkgPT0gUG9zaXRpb25Jc0FmdGVyQW5jaG9yKSB7CiAgICAgICAgIG5vZGUg PSBjb250YWluZXJOb2RlKCk7CisgICAgICAgIGlmICghbm9kZSkKKyAgICAgICAgICAgIHJldHVy biAqdGhpczsKKwogICAgICAgICBvZmZzZXQgPSBjb21wdXRlT2Zmc2V0SW5Db250YWluZXJOb2Rl KCk7CiAgICAgfQogCkBAIC0zNzYsNyArMzgyLDcgQEAgUG9zaXRpb24gUG9zaXRpb246Om5leHQo UG9zaXRpb25Nb3ZlVHlwZSBtb3ZlVHlwZSkgY29uc3QKICAgICAgICAgcmV0dXJuIGNyZWF0ZUxl Z2FjeUVkaXRpbmdQb3NpdGlvbihub2RlLCAobW92ZVR5cGUgPT0gQ2hhcmFjdGVyKSA/IHVuY2hl Y2tlZE5leHRPZmZzZXQobm9kZSwgb2Zmc2V0KSA6IG9mZnNldCArIDEpOwogICAgIH0KIAotICAg IENvbnRhaW5lck5vZGUqIHBhcmVudCA9IGZpbmRQYXJlbnQobm9kZSk7CisgICAgQ29udGFpbmVy Tm9kZSogcGFyZW50ID0gZmluZFBhcmVudCgqbm9kZSk7CiAgICAgaWYgKCFwYXJlbnQpCiAgICAg ICAgIHJldHVybiAqdGhpczsKIApAQCAtNDc4LDcgKzQ4NCw3IEBAIGJvb2wgUG9zaXRpb246OmF0 U3RhcnRPZlRyZWUoKSBjb25zdAogICAgICAgICByZXR1cm4gdHJ1ZTsKIAogICAgIE5vZGUqIGNv bnRhaW5lciA9IGNvbnRhaW5lck5vZGUoKTsKLSAgICBpZiAoY29udGFpbmVyICYmIGZpbmRQYXJl bnQoY29udGFpbmVyKSkKKyAgICBpZiAoY29udGFpbmVyICYmIGZpbmRQYXJlbnQoKmNvbnRhaW5l cikpCiAgICAgICAgIHJldHVybiBmYWxzZTsKIAogICAgIHN3aXRjaCAobV9hbmNob3JUeXBlKSB7 CkBAIC01MDMsNyArNTA5LDcgQEAgYm9vbCBQb3NpdGlvbjo6YXRFbmRPZlRyZWUoKSBjb25zdAog ICAgICAgICByZXR1cm4gdHJ1ZTsKIAogICAgIE5vZGUqIGNvbnRhaW5lciA9IGNvbnRhaW5lck5v ZGUoKTsKLSAgICBpZiAoY29udGFpbmVyICYmIGZpbmRQYXJlbnQoY29udGFpbmVyKSkKKyAgICBp ZiAoY29udGFpbmVyICYmIGZpbmRQYXJlbnQoKmNvbnRhaW5lcikpCiAgICAgICAgIHJldHVybiBm YWxzZTsKIAogICAgIHN3aXRjaCAobV9hbmNob3JUeXBlKSB7CkBAIC05MzgsOSArOTQ0LDkgQEAg Ym9vbCBQb3NpdGlvbjo6bm9kZUlzVXNlclNlbGVjdE5vbmUoTm9kZSogbm9kZSkKICAgICByZXR1 cm4gbm9kZSAmJiBub2RlLT5yZW5kZXJlcigpICYmIG5vZGUtPnJlbmRlcmVyKCktPnN0eWxlKCku dXNlclNlbGVjdCgpID09IFNFTEVDVF9OT05FOwogfQogCi1Db250YWluZXJOb2RlKiBQb3NpdGlv bjo6ZmluZFBhcmVudChjb25zdCBOb2RlKiBub2RlKQorQ29udGFpbmVyTm9kZSogUG9zaXRpb246 OmZpbmRQYXJlbnQoY29uc3QgTm9kZSYgbm9kZSkKIHsKLSAgICByZXR1cm4gbm9kZS0+bm9uU2hh ZG93Qm91bmRhcnlQYXJlbnROb2RlKCk7CisgICAgcmV0dXJuIG5vZGUubm9uU2hhZG93Qm91bmRh cnlQYXJlbnROb2RlKCk7CiB9CiAKICNpZiBFTkFCTEUoVVNFUlNFTEVDVF9BTEwpCmRpZmYgLS1n aXQgYS9Tb3VyY2UvV2ViQ29yZS9kb20vUG9zaXRpb24uaCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9Q b3NpdGlvbi5oCmluZGV4IGNiMzVlMjJlNTE0MTNiMWM4ZTg0NzU4ZDMxOWViNzBhNGYyNzgxOTEu LjBmMzE2ZGE4NTg4YTUxOTI5ZmQ0NzUxYzlmY2IzM2I5OTE2NzMzZWIgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL2RvbS9Qb3NpdGlvbi5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL2RvbS9Qb3Np dGlvbi5oCkBAIC0xOTgsNyArMTk4LDcgQEAgcHVibGljOgogICAgIHN0YXRpYyBib29sIG5vZGVJ c1VzZXJTZWxlY3RBbGwoY29uc3QgTm9kZSopIHsgcmV0dXJuIGZhbHNlOyB9CiAgICAgc3RhdGlj IE5vZGUqIHJvb3RVc2VyU2VsZWN0QWxsRm9yTm9kZShOb2RlKikgeyByZXR1cm4gMDsgfQogI2Vu ZGlmCi0gICAgc3RhdGljIENvbnRhaW5lck5vZGUqIGZpbmRQYXJlbnQoY29uc3QgTm9kZSopOwor ICAgIHN0YXRpYyBDb250YWluZXJOb2RlKiBmaW5kUGFyZW50KGNvbnN0IE5vZGUmKTsKICAgICAK ICAgICB2b2lkIGRlYnVnUG9zaXRpb24oY29uc3QgY2hhciogbXNnID0gIiIpIGNvbnN0OwogCg==