123763 2013-11-04 16:21:37 -0800 FrameView destructor is worried about being retained by a renderer. 2013-11-04 17:38:25 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 kling kling commit-queue darin kling oldest_to_newest 946877 0 kling 2013-11-04 16:21:37 -0800 // FIXME: How could this ever happen when RenderWidget retains the Widget!? Indeed. 946878 1 215965 kling 2013-11-04 16:22:04 -0800 Created attachment 215965 Patch 946914 2 215965 darin 2013-11-04 17:21:54 -0800 Comment on attachment 215965 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215965&action=review > Source/WebCore/ChangeLog:8 > + There's no way we can be in ~FrameView() while also being owned by > + a RenderWidget. Remove some overly paranoid code that was making sure > + the renderer didn't have a reference on us. Could you have put in an assertion or is this so obviously true that an assertion would be a waste? 946915 3 kling 2013-11-04 17:23:26 -0800 (In reply to comment #2) > (From update of attachment 215965 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215965&action=review > > > Source/WebCore/ChangeLog:8 > > + There's no way we can be in ~FrameView() while also being owned by > > + a RenderWidget. Remove some overly paranoid code that was making sure > > + the renderer didn't have a reference on us. > > Could you have put in an assertion or is this so obviously true that an assertion would be a waste? I thought about it, and it seemed to me that asserting in ~Foo() that someone's RefPtr<Foo> isn't pointing to |this| is overly paranoid. WDYT? 946925 4 darin 2013-11-04 17:30:30 -0800 (In reply to comment #3) > I thought about it, and it seemed to me that asserting in ~Foo() that someone's RefPtr<Foo> isn't pointing to |this| is overly paranoid. WDYT? Yes, and no. Paranoid, yes. But catching overrelease in the destructor might be great instead of having to debug use of the deallocated memory later. I wish every class could have a check like this one. If this would help us debug a case where we someone does an extra deref, then it would be great. Not really sure how to judge. 946928 5 215965 commit-queue 2013-11-04 17:31:39 -0800 Comment on attachment 215965 Patch Clearing flags on attachment: 215965 Committed r158625: <http://trac.webkit.org/changeset/158625> 946929 6 commit-queue 2013-11-04 17:31:40 -0800 All reviewed patches have been landed. Closing bug. 946930 7 kling 2013-11-04 17:33:28 -0800 (In reply to comment #4) > (In reply to comment #3) > > I thought about it, and it seemed to me that asserting in ~Foo() that someone's RefPtr<Foo> isn't pointing to |this| is overly paranoid. WDYT? > > Yes, and no. Paranoid, yes. But catching overrelease in the destructor might be great instead of having to debug use of the deallocated memory later. I wish every class could have a check like this one. If this would help us debug a case where we someone does an extra deref, then it would be great. Doesn't RefCounted already do something like this with the deletion-has-begun assertions? 946935 8 darin 2013-11-04 17:38:25 -0800 (In reply to comment #7) > Doesn't RefCounted already do something like this with the deletion-has-begun assertions? Maybe it does! 215965 2013-11-04 16:22:04 -0800 2013-11-04 17:31:39 -0800 Patch bug-123763.diff text/plain 1338 kling ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBhMTU1YmE5Li5jZGUzMzE1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMTYg QEAKIDIwMTMtMTEtMDQgIEFuZHJlYXMgS2xpbmcgIDxha2xpbmdAYXBwbGUuY29tPgogCisgICAg ICAgIEZyYW1lVmlldyBkZXN0cnVjdG9yIGlzIHdvcnJpZWQgYWJvdXQgYmVpbmcgcmV0YWluZWQg YnkgYSByZW5kZXJlci4KKyAgICAgICAgPGh0dHBzOi8vd2Via2l0Lm9yZy9iLzEyMzc2Mz4KKwor ICAgICAgICBUaGVyZSdzIG5vIHdheSB3ZSBjYW4gYmUgaW4gfkZyYW1lVmlldygpIHdoaWxlIGFs c28gYmVpbmcgb3duZWQgYnkKKyAgICAgICAgYSBSZW5kZXJXaWRnZXQuIFJlbW92ZSBzb21lIG92 ZXJseSBwYXJhbm9pZCBjb2RlIHRoYXQgd2FzIG1ha2luZyBzdXJlCisgICAgICAgIHRoZSByZW5k ZXJlciBkaWRuJ3QgaGF2ZSBhIHJlZmVyZW5jZSBvbiB1cy4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworMjAxMy0xMS0wNCAgQW5kcmVhcyBLbGluZyAgPGFrbGluZ0Bh cHBsZS5jb20+CisKICAgICAgICAgVXNlIFJlbmRlckFuY2VzdG9ySXRlcmF0b3IgaW4gYSBjb3Vw bGUgb2YgcGxhY2VzLgogICAgICAgICA8aHR0cHM6Ly93ZWJraXQub3JnL2IvMTIzNzI1PgogCmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wYWdlL0ZyYW1lVmlldy5jcHAgYi9Tb3VyY2UvV2Vi Q29yZS9wYWdlL0ZyYW1lVmlldy5jcHAKaW5kZXggNjkzOTU0MS4uMjI1ODMyZiAxMDA2NDQKLS0t IGEvU291cmNlL1dlYkNvcmUvcGFnZS9GcmFtZVZpZXcuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3Jl L3BhZ2UvRnJhbWVWaWV3LmNwcApAQCAtMjQ1LDExICsyNDUsNiBAQCBGcmFtZVZpZXc6On5GcmFt ZVZpZXcoKQogICAgIEFTU0VSVChtX3NjaGVkdWxlZEV2ZW50cy5pc0VtcHR5KCkpOwogCiAgICAg QVNTRVJUKGZyYW1lKCkudmlldygpICE9IHRoaXMgfHwgIWZyYW1lKCkuY29udGVudFJlbmRlcmVy KCkpOwotCi0gICAgLy8gRklYTUU6IEhvdyBjb3VsZCB0aGlzIGV2ZXIgaGFwcGVuIHdoZW4gUmVu ZGVyV2lkZ2V0IHJldGFpbnMgdGhlIFdpZGdldCE/Ci0gICAgUmVuZGVyV2lkZ2V0KiByZW5kZXJl ciA9IGZyYW1lKCkub3duZXJSZW5kZXJlcigpOwotICAgIGlmIChyZW5kZXJlciAmJiByZW5kZXJl ci0+d2lkZ2V0KCkgPT0gdGhpcykKLSAgICAgICAgcmVuZGVyZXItPnNldFdpZGdldCgwKTsKIH0K IAogdm9pZCBGcmFtZVZpZXc6OnJlc2V0KCkK