169189 2017-03-05 16:59:10 -0800 Replace debug assertion with release one in Frame::setView() 2017-03-07 09:22:43 -0800 1 1 1 Unclassified WebKit DOM WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 cdumez cdumez barraclough bfulgham cdumez commit-queue dbates esprehn+autocc kangil.han rniwa oldest_to_newest 1283652 0 cdumez 2017-03-05 16:59:10 -0800 Replace debug assertion with release one in Frame::setView() to make make sure the document does not have a living render tree. 1283653 1 303486 cdumez 2017-03-05 17:01:00 -0800 Created attachment 303486 Patch 1283654 2 cdumez 2017-03-05 17:02:34 -0800 I discussed this with Ryosuke and Gavin and they'd prefer to use a RELEASE_ASSERT here, so do I. 1283864 3 bfulgham 2017-03-06 12:01:55 -0800 I'm a little worried about this proposed change. We have crash trace reports that indicate that we do get into this state (though perhaps not after your recent changes). Is it really better to crash the user process than it is to do this cleanup? CurrentlY: 1) If we never encounter this case, the cleanup code doesn't run. 2) If we do encounter this case, the cleanup code will put us back into a good state. You are proposing we change to a user-visible crash, which doesn't seem like an obvious improvement to me. 1284007 4 rniwa 2017-03-06 15:28:48 -0800 (In reply to comment #3) > I'm a little worried about this proposed change. We have crash trace reports > that indicate that we do get into this state (though perhaps not after your > recent changes). Is it really better to crash the user process than it is to > do this cleanup? > > CurrentlY: > 1) If we never encounter this case, the cleanup code doesn't run. > 2) If we do encounter this case, the cleanup code will put us back into a > good state. Given that we don't believe we should ever get into this state, a release assert would be better. If we do get into this state, then we'd know from crash reports. > You are proposing we change to a user-visible crash, which doesn't seem like > an obvious improvement to me. Given that this "cleanup" has caused regressions elsewhere, I'm not certain that crashing is necessarily worse. 1284012 5 cdumez 2017-03-06 15:32:55 -0800 (In reply to comment #3) > I'm a little worried about this proposed change. We have crash trace reports > that indicate that we do get into this state (though perhaps not after your > recent changes). Is it really better to crash the user process than it is to > do this cleanup? > > CurrentlY: > 1) If we never encounter this case, the cleanup code doesn't run. > 2) If we do encounter this case, the cleanup code will put us back into a > good state. > > You are proposing we change to a user-visible crash, which doesn't seem like > an obvious improvement to me. My opinion is that if it still happens, we want to know sooner rather than later. A release assertion on trunk makes it way more likely for us to find possible remaining cases where this happens after my recent fix. If it becomes too crashy, we can always roll out this patch to restore stability. 1284017 6 cdumez 2017-03-06 15:37:01 -0800 (In reply to comment #5) > (In reply to comment #3) > > I'm a little worried about this proposed change. We have crash trace reports > > that indicate that we do get into this state (though perhaps not after your > > recent changes). Is it really better to crash the user process than it is to > > do this cleanup? > > > > CurrentlY: > > 1) If we never encounter this case, the cleanup code doesn't run. > > 2) If we do encounter this case, the cleanup code will put us back into a > > good state. > > > > You are proposing we change to a user-visible crash, which doesn't seem like > > an obvious improvement to me. > > My opinion is that if it still happens, we want to know sooner rather than > later. A release assertion on trunk makes it way more likely for us to find > possible remaining cases where this happens after my recent fix. > > If it becomes too crashy, we can always roll out this patch to restore > stability. Also note that it was easy to get the original test case to crash by modifying it slightly, despite the code reconstructing the render tree. If we arrive at this point with a render tree, it is still likely we'll crash despite reconstructing a render tree since we would be in a bad state and other places in the code do not handle this bad state. 1284358 7 303486 commit-queue 2017-03-07 09:22:38 -0800 Comment on attachment 303486 Patch Clearing flags on attachment: 303486 Committed r213521: <http://trac.webkit.org/changeset/213521> 1284359 8 commit-queue 2017-03-07 09:22:43 -0800 All reviewed patches have been landed. Closing bug. 303486 2017-03-05 17:01:00 -0800 2017-03-07 09:22:38 -0800 Patch bug-169189-20170305170058.patch text/plain 3197 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjEzNDMxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDcyNTBlMWY2Mzk0ZDI3 YjRmMDNjMDZlZmMxMzljZDE0MGE4Njg5Yy4uODhhMTRmMjYyMjY5OGI3ZjM1NTc2YWFiMGVkNTEy MGQwMzlhNjRkYSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDE3LTAzLTA1ICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CisKKyAgICAgICAgUmVwbGFjZSBkZWJ1ZyBhc3Nl cnRpb24gd2l0aCByZWxlYXNlIG9uZSBpbiBGcmFtZTo6c2V0VmlldygpCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjkxODkKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBSZXBsYWNlIGRlYnVnIGFzc2VydGlv biB3aXRoIHJlbGVhc2Ugb25lIGluIEZyYW1lOjpzZXRWaWV3KCkgdG8gbWFrZSBtYWtlIHN1cmUg dGhlCisgICAgICAgIGRvY3VtZW50IGRvZXMgbm90IGhhdmUgYSBsaXZpbmcgcmVuZGVyIHRyZWUu IFRoaXMgd2lsbCBoZWxwIGlkZW50aWZ5IHBvc3NpYmxlCisgICAgICAgIHJlbWFpbmluZyBjYXNl cyB3aGVyZSB0aGlzIGNhbiBoYXBwZW4uCisKKyAgICAgICAgKiBkb20vRG9jdW1lbnQuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6RG9jdW1lbnQ6OmRpZEJlY29tZUN1cnJlbnREb2N1bWVudEluVmll dyk6IERlbGV0ZWQuCisgICAgICAgICogZG9tL0RvY3VtZW50Lmg6CisgICAgICAgICogcGFnZS9G cmFtZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpGcmFtZTo6c2V0Vmlldyk6CisKIDIwMTctMDMt MDQgIFNpbW9uIEZyYXNlciAgPHNpbW9uLmZyYXNlckBhcHBsZS5jb20+CiAKICAgICAgICAgQ2xh cmlmeSBzb21lIHRlcm1pbm9sb2d5IGluIFJlbmRlckxheWVyQmFja2luZwpkaWZmIC0tZ2l0IGEv U291cmNlL1dlYkNvcmUvZG9tL0RvY3VtZW50LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9Eb2N1 bWVudC5jcHAKaW5kZXggOWM1MjUzNjc1ZGQ0NjIxZmY1ZDg5NTU5NTFmOWM4ZTgwMWM0NmY1Mi4u ZTlhZjBjNzJhNDRhMGE1YTk1YmI5MTgyZWM1YzYxMTg4ZThkMGI4ZCAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYkNvcmUvZG9tL0RvY3VtZW50LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9kb20vRG9j dW1lbnQuY3BwCkBAIC0yMTgyLDEzICsyMTgyLDYgQEAgdm9pZCBEb2N1bWVudDo6ZnJhbWVEZXN0 cm95ZWQoKQogICAgIEZyYW1lRGVzdHJ1Y3Rpb25PYnNlcnZlcjo6ZnJhbWVEZXN0cm95ZWQoKTsK IH0KIAotdm9pZCBEb2N1bWVudDo6ZGlkQmVjb21lQ3VycmVudERvY3VtZW50SW5WaWV3KCkKLXsK LSAgICBBU1NFUlQodmlldygpKTsKLSAgICBpZiAoIWhhc0xpdmluZ1JlbmRlclRyZWUoKSkKLSAg ICAgICAgY3JlYXRlUmVuZGVyVHJlZSgpOwotfQotCiB2b2lkIERvY3VtZW50OjphdHRhY2hUb0Nh Y2hlZEZyYW1lKENhY2hlZEZyYW1lQmFzZSYgY2FjaGVkRnJhbWUpCiB7CiAgICAgQVNTRVJUX1dJ VEhfU0VDVVJJVFlfSU1QTElDQVRJT04oY2FjaGVkRnJhbWUuZG9jdW1lbnQoKSA9PSB0aGlzKTsK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2RvbS9Eb2N1bWVudC5oIGIvU291cmNlL1dlYkNv cmUvZG9tL0RvY3VtZW50LmgKaW5kZXggNzdlMWYzNGZmOTFiMWVlYmJmOGQ2YzYwMmM0YzU2YWU4 ZWE0MmNjMi4uZmQxMDkwMjJlYjJjNzhhNGVhOWU1NzM2ODc4ZDRiMGE5MDVkMzYyYSAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvZG9tL0RvY3VtZW50LmgKKysrIGIvU291cmNlL1dlYkNvcmUv ZG9tL0RvY3VtZW50LmgKQEAgLTU1Miw3ICs1NTIsNiBAQCBwdWJsaWM6CiAgICAgdm9pZCBkaWRC ZWNvbWVDdXJyZW50RG9jdW1lbnRJbkZyYW1lKCk7CiAgICAgdm9pZCBkZXN0cm95UmVuZGVyVHJl ZSgpOwogICAgIHZvaWQgcHJlcGFyZUZvckRlc3RydWN0aW9uKCk7Ci0gICAgdm9pZCBkaWRCZWNv bWVDdXJyZW50RG9jdW1lbnRJblZpZXcoKTsKIAogICAgIC8vIE92ZXJyaWRlIFNjcmlwdEV4ZWN1 dGlvbkNvbnRleHQgbWV0aG9kcyB0byBkbyBhZGRpdGlvbmFsIHdvcmsKICAgICBib29sIHNob3Vs ZEJ5cGFzc01haW5Xb3JsZENvbnRlbnRTZWN1cml0eVBvbGljeSgpIGNvbnN0IGZpbmFsOwpkaWZm IC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGFnZS9GcmFtZS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9w YWdlL0ZyYW1lLmNwcAppbmRleCA0MGUzODdiYjU3YzA1MzdlMmY1ZjhlZTNiYzY2YTBjOGVlYjhi YTVlLi44ZTA1MTJiOWFhYTAxYWE2ODE1ZGFjMTlmNjA5NDMzOTBkZDRjOTA3IDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViQ29yZS9wYWdlL0ZyYW1lLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wYWdl L0ZyYW1lLmNwcApAQCAtMjU1LDE2ICsyNTUsOSBAQCB2b2lkIEZyYW1lOjpzZXRWaWV3KFJlZlB0 cjxGcmFtZVZpZXc+JiYgdmlldykKICAgICBpZiAobV9ldmVudEhhbmRsZXIpCiAgICAgICAgIG1f ZXZlbnRIYW5kbGVyLT5jbGVhcigpOwogCi0gICAgYm9vbCBoYWRMaXZpbmdSZW5kZXJUcmVlID0g bV9kb2MgPyBtX2RvYy0+aGFzTGl2aW5nUmVuZGVyVHJlZSgpIDogZmFsc2U7Ci0gICAgLy8gVHJ5 IGFuZCBjYXRjaCBjYXNlcyB3aGVyZSB0aGlzIG1pZ2h0IHN0aWxsIGJlIGhhcHBlbmluZyBhZnRl ciByMjEzMzExLgotICAgIEFTU0VSVF9XSVRIX1NFQ1VSSVRZX0lNUExJQ0FUSU9OKCFoYWRMaXZp bmdSZW5kZXJUcmVlKTsKLSAgICBpZiAoaGFkTGl2aW5nUmVuZGVyVHJlZSkKLSAgICAgICAgbV9k b2MtPmRlc3Ryb3lSZW5kZXJUcmVlKCk7CisgICAgUkVMRUFTRV9BU1NFUlQoIW1fZG9jIHx8ICFt X2RvYy0+aGFzTGl2aW5nUmVuZGVyVHJlZSgpKTsKIAogICAgIG1fdmlldyA9IFdURk1vdmUodmll dyk7Ci0KLSAgICBpZiAoaGFkTGl2aW5nUmVuZGVyVHJlZSAmJiBtX3ZpZXcpCi0gICAgICAgIG1f ZG9jLT5kaWRCZWNvbWVDdXJyZW50RG9jdW1lbnRJblZpZXcoKTsKICAgICAKICAgICAvLyBPbmx5 IG9uZSBmb3JtIHN1Ym1pc3Npb24gaXMgYWxsb3dlZCBwZXIgdmlldyBvZiBhIHBhcnQuCiAgICAg Ly8gU2luY2UgdGhpcyBwYXJ0IG1heSBiZSBnZXR0aW5nIHJldXNlZCBhcyBhIHJlc3VsdCBvZiBi ZWluZwo=