94335 2012-08-17 04:59:52 -0700 Remove RefPtr from HTMLMeterElement::m_value 2022-08-11 22:22:24 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Unspecified Unspecified RESOLVED LATER P2 Normal --- 94324 1 haraken haraken ahmad.saleem792 ap bfulgham morrita rniwa tkent oldest_to_newest 698212 0 haraken 2012-08-17 04:59:52 -0700 To avoid reference cycles of RefPtr<Node>s, we want to remove unnecessary RefPtr<Node>s. The rationale is describe in bug 94324. HTMLMeterElement::m_value does not need to be a RefPtr<Node>, because it is guaranteed to point to a shadow DOM tree of the HTMLMeterElement node, which is guaranteed to exist in the subtree of the HTMLMeterElement node. 698214 1 159091 haraken 2012-08-17 05:02:25 -0700 Created attachment 159091 Patch 698224 2 159095 haraken 2012-08-17 05:11:43 -0700 Created attachment 159095 Patch 699134 3 159091 morrita 2012-08-19 18:22:36 -0700 Comment on attachment 159091 Patch I prefer just replacing m_value with some accessor that traverses shadow subtree and return specified node. Having raw pointer is too error-prone and the source of cryptic crashes even if its existence is theoretically safe under certain circumstances. 699137 4 tkent 2012-08-19 18:27:26 -0700 (In reply to comment #3) > (From update of attachment 159091 [details]) > I prefer just replacing m_value with some accessor that traverses shadow subtree and return specified node. > Having raw pointer is too error-prone and the source of cryptic crashes even if its existence is theoretically safe > under certain circumstances. We should resolve Bug 87815 first. 699140 5 haraken 2012-08-19 18:31:03 -0700 (In reply to comment #3) > (From update of attachment 159091 [details]) > I prefer just replacing m_value with some accessor that traverses shadow subtree and return specified node. - Isn't there any perf concern on traversing the shadow subtree every time? - A bunch of other code already use raw pointers. Replacing all raw pointers with accessors might not make sense. > Having raw pointer is too error-prone and the source of cryptic crashes even if its existence is theoretically safe > under certain circumstances. As I explained in bug 94324, if this change caused any crash, it means that you are creating a reference cycle (i.e. memory leak). 699148 6 morrita 2012-08-19 18:45:25 -0700 (In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 159091 [details] [details]) > > I prefer just replacing m_value with some accessor that traverses shadow subtree and return specified node. > > - Isn't there any perf concern on traversing the shadow subtree every time? In this case, no. It is only touched when specific attribute value is changed and it results massive traversals for style recalculation anyway. > > - A bunch of other code already use raw pointers. Replacing all raw pointers with accessors might not make sense. And they occasionally causes crashes. I hope we had better way for living with it. I agree that there is no definitely better alternative for now though. > As I explained in bug 94324, if this change caused any crash, it means that you are creating a reference cycle (i.e. memory leak). Leaks are better than crashes, in particular touching dangling pointers which can result a security hole. I'm not saying we're leaking nor crashing. I'm talking about how we can minimize the damage from future bugs. 699151 7 haraken 2012-08-19 18:56:26 -0700 (In reply to comment #6) > Leaks are better than crashes, in particular touching dangling pointers > which can result a security hole. > > I'm not saying we're leaking nor crashing. > I'm talking about how we can minimize the damage from future bugs. Got it. - I believe this patch is safe and that we should remove "just in case" RefPtrs because they can potentially create reference cycles. But if there is any concern about future crashes, I'm not intending to land this patch. - The first objective of this patch is to remove unnecessary RefPtrs from the code base, because they make it difficult to reason about reference cycles. - The second objective is that these RefPtrs cause a memory leak in the new DOM lifetime management (bug 88834). However, because I noticed that it is almost impossible to completely remove all reference cycles from the code base, I'm now developing a new lifetime algorithm that can live with reference cycles. So this bug is not blocking bug 88834. 1890692 8 ahmad.saleem792 2022-08-11 13:40:30 -0700 This still exist: https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/WebCore/html/HTMLMeterElement.h#L79 rniwa@webkit.org - Is this needed anymore? Or this can be marked as "RESOLVED LATER"? Thanks! 1890812 9 rniwa 2022-08-11 22:22:24 -0700 yeah, this is Later. no need to keep this bug open at this point since there isn't any leak or anything. 159091 2012-08-17 05:02:25 -0700 2012-08-19 18:22:36 -0700 Patch bug-94335-20120817210149.patch text/plain 2732 haraken U3VidmVyc2lvbiBSZXZpc2lvbjogMTI1ODc3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDkyYWYxODdhM2ZiZjA1 NGRjMmU2NTdiODQwNjIxNmUwMmZhNDdkOS4uNGM3YWRlMzY3MzQwYmJmMWM4NGRiMzVhMWU0N2Zm M2MzMzE2ODcyNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDEyLTA4LTE3ICBLZW50 YXJvIEhhcmEgIDxoYXJha2VuQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZW1vdmUgUmVmUHRy IGZyb20gSFRNTE1ldGVyRWxlbWVudDo6bV92YWx1ZQorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTQzMzUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBUbyBhdm9pZCByZWZlcmVuY2UgY3ljbGVzIG9mIFJlZlB0 cjxOb2RlPnMsIHdlIHdhbnQgdG8gcmVtb3ZlIHVubmVjZXNzYXJ5CisgICAgICAgIFJlZlB0cjxO b2RlPnMuIFRoZSByYXRpb25hbGUgaXMgZGVzY3JpYmVkIGluIGJ1ZyA5NDMyNC4KKworICAgICAg ICBIVE1MTWV0ZXJFbGVtZW50OjptX3ZhbHVlIGRvZXMgbm90IG5lZWQgdG8gYmUgYSBSZWZQdHI8 Tm9kZT4sIGJlY2F1c2UgaXQKKyAgICAgICAgaXMgZ3VhcmFudGVlZCB0byBwb2ludCB0byBhIHNo YWRvdyBET00gdHJlZSBvZiB0aGUgSFRNTE1ldGVyRWxlbWVudCBub2RlLAorICAgICAgICB3aGlj aCBpcyBndWFyYW50ZWVkIHRvIGV4aXN0IGluIHRoZSBzdWJ0cmVlIG9mIHRoZSBIVE1MTWV0ZXJF bGVtZW50IG5vZGUuCisKKyAgICAgICAgTm8gdGVzdHMuIE5vIGNoYW5nZSBpbiBiZWhhdmlvci4K KworICAgICAgICAqIGh0bWwvSFRNTE1ldGVyRWxlbWVudC5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpIVE1MTWV0ZXJFbGVtZW50OjpIVE1MTWV0ZXJFbGVtZW50KToKKyAgICAgICAgKFdlYkNvcmU6 OkhUTUxNZXRlckVsZW1lbnQ6OmNyZWF0ZVNoYWRvd1N1YnRyZWUpOgorICAgICAgICAqIGh0bWwv SFRNTE1ldGVyRWxlbWVudC5oOgorICAgICAgICAoSFRNTE1ldGVyRWxlbWVudCk6CisKIDIwMTIt MDgtMTcgIFlvc2hpZnVtaSBJbm91ZSAgPHlvc2luQGNocm9taXVtLm9yZz4KIAogICAgICAgICBb Rm9ybXNdIFJlbmFtZSBTcGluQnV0dG9uRWxlbWVudDo6U3RlcEFjdGlvbkhhbmRsZXIgdG8gU3Bp bkJ1dHRvbk93bmVyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxNZXRlckVs ZW1lbnQuY3BwIGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MTWV0ZXJFbGVtZW50LmNwcAppbmRl eCBkYmY0NmMxNWZhY2MzMjE4MzNmY2Y4ODhkYWZkNjEyNGZjNDQ3MzQ2Li5mZWVlMzNlYmViNWU4 NGNhOGYyYmIxZDAwNjZkZmE1NGUyZmZjZDMxIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9o dG1sL0hUTUxNZXRlckVsZW1lbnQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTE1l dGVyRWxlbWVudC5jcHAKQEAgLTQzLDYgKzQzLDcgQEAgdXNpbmcgbmFtZXNwYWNlIEhUTUxOYW1l czsKIAogSFRNTE1ldGVyRWxlbWVudDo6SFRNTE1ldGVyRWxlbWVudChjb25zdCBRdWFsaWZpZWRO YW1lJiB0YWdOYW1lLCBEb2N1bWVudCogZG9jdW1lbnQpCiAgICAgOiBMYWJlbGFibGVFbGVtZW50 KHRhZ05hbWUsIGRvY3VtZW50KQorICAgICwgbV92YWx1ZSgwKQogICAgICwgbV9oYXNBdXRob3JT aGFkb3dSb290KGZhbHNlKQogewogICAgIEFTU0VSVChoYXNUYWdOYW1lKG1ldGVyVGFnKSk7CkBA IC0yNTUsNyArMjU2LDggQEAgdm9pZCBIVE1MTWV0ZXJFbGVtZW50OjpjcmVhdGVTaGFkb3dTdWJ0 cmVlKCkKICAgICByb290LT5hcHBlbmRDaGlsZChpbm5lcik7CiAKICAgICBSZWZQdHI8TWV0ZXJC YXJFbGVtZW50PiBiYXIgPSBNZXRlckJhckVsZW1lbnQ6OmNyZWF0ZShkb2N1bWVudCgpKTsKLSAg ICBtX3ZhbHVlID0gTWV0ZXJWYWx1ZUVsZW1lbnQ6OmNyZWF0ZShkb2N1bWVudCgpKTsKKyAgICBS ZWZQdHI8TWV0ZXJWYWx1ZUVsZW1lbnQ+IHZhbHVlID0gTWV0ZXJWYWx1ZUVsZW1lbnQ6OmNyZWF0 ZShkb2N1bWVudCgpKTsKKyAgICBtX3ZhbHVlID0gdmFsdWUuZ2V0KCk7CiAgICAgbV92YWx1ZS0+ c2V0V2lkdGhQZXJjZW50YWdlKDApOwogICAgIGJhci0+YXBwZW5kQ2hpbGQobV92YWx1ZSwgQVNT RVJUX05PX0VYQ0VQVElPTik7CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2h0bWwvSFRN TE1ldGVyRWxlbWVudC5oIGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MTWV0ZXJFbGVtZW50LmgK aW5kZXggOGU2M2M2NGE4YWQwNTdkMzEwNjk1NzljYmVlMmMxNzhlNGI1ZTIwZS4uNThjZjg2NmNh NDBjNDljNWJmNWY1ZDhhZDA2ZWQ4MWVmYjRhMGVjOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvaHRtbC9IVE1MTWV0ZXJFbGVtZW50LmgKKysrIGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1M TWV0ZXJFbGVtZW50LmgKQEAgLTg1LDcgKzg1LDcgQEAgcHJpdmF0ZToKICAgICB2b2lkIGRpZEVs ZW1lbnRTdGF0ZUNoYW5nZSgpOwogICAgIHZvaWQgY3JlYXRlU2hhZG93U3VidHJlZSgpOwogCi0g ICAgUmVmUHRyPE1ldGVyVmFsdWVFbGVtZW50PiBtX3ZhbHVlOworICAgIE1ldGVyVmFsdWVFbGVt ZW50KiBtX3ZhbHVlOwogICAgIGJvb2wgbV9oYXNBdXRob3JTaGFkb3dSb290OwogfTsKIAo= 159095 2012-08-17 05:11:43 -0700 2012-08-19 18:26:17 -0700 Patch bug-94335-20120817211107.patch text/plain 3038 haraken U3VidmVyc2lvbiBSZXZpc2lvbjogMTI1ODc3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDkyYWYxODdhM2ZiZjA1 NGRjMmU2NTdiODQwNjIxNmUwMmZhNDdkOS4uNGM3YWRlMzY3MzQwYmJmMWM4NGRiMzVhMWU0N2Zm M2MzMzE2ODcyNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDEyLTA4LTE3ICBLZW50 YXJvIEhhcmEgIDxoYXJha2VuQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZW1vdmUgUmVmUHRy IGZyb20gSFRNTE1ldGVyRWxlbWVudDo6bV92YWx1ZQorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTQzMzUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBUbyBhdm9pZCByZWZlcmVuY2UgY3ljbGVzIG9mIFJlZlB0 cjxOb2RlPnMsIHdlIHdhbnQgdG8gcmVtb3ZlIHVubmVjZXNzYXJ5CisgICAgICAgIFJlZlB0cjxO b2RlPnMuIFRoZSByYXRpb25hbGUgaXMgZGVzY3JpYmVkIGluIGJ1ZyA5NDMyNC4KKworICAgICAg ICBIVE1MTWV0ZXJFbGVtZW50OjptX3ZhbHVlIGRvZXMgbm90IG5lZWQgdG8gYmUgYSBSZWZQdHI8 Tm9kZT4sIGJlY2F1c2UgaXQKKyAgICAgICAgaXMgZ3VhcmFudGVlZCB0byBwb2ludCB0byBhIHNo YWRvdyBET00gdHJlZSBvZiB0aGUgSFRNTE1ldGVyRWxlbWVudCBub2RlLAorICAgICAgICB3aGlj aCBpcyBndWFyYW50ZWVkIHRvIGV4aXN0IGluIHRoZSBzdWJ0cmVlIG9mIHRoZSBIVE1MTWV0ZXJF bGVtZW50IG5vZGUuCisKKyAgICAgICAgTm8gdGVzdHMuIE5vIGNoYW5nZSBpbiBiZWhhdmlvci4K KworICAgICAgICAqIGh0bWwvSFRNTE1ldGVyRWxlbWVudC5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpIVE1MTWV0ZXJFbGVtZW50OjpIVE1MTWV0ZXJFbGVtZW50KToKKyAgICAgICAgKFdlYkNvcmU6 OkhUTUxNZXRlckVsZW1lbnQ6OmNyZWF0ZVNoYWRvd1N1YnRyZWUpOgorICAgICAgICAqIGh0bWwv SFRNTE1ldGVyRWxlbWVudC5oOgorICAgICAgICAoSFRNTE1ldGVyRWxlbWVudCk6CisKIDIwMTIt MDgtMTcgIFlvc2hpZnVtaSBJbm91ZSAgPHlvc2luQGNocm9taXVtLm9yZz4KIAogICAgICAgICBb Rm9ybXNdIFJlbmFtZSBTcGluQnV0dG9uRWxlbWVudDo6U3RlcEFjdGlvbkhhbmRsZXIgdG8gU3Bp bkJ1dHRvbk93bmVyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxNZXRlckVs ZW1lbnQuY3BwIGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MTWV0ZXJFbGVtZW50LmNwcAppbmRl eCBkYmY0NmMxNWZhY2MzMjE4MzNmY2Y4ODhkYWZkNjEyNGZjNDQ3MzQ2Li5hYTg2ZTdhODAzYmE4 ZDZiOTM2MDIwYzVlNmUyN2E2YzIyNmM1NGFkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9o dG1sL0hUTUxNZXRlckVsZW1lbnQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTE1l dGVyRWxlbWVudC5jcHAKQEAgLTQzLDYgKzQzLDcgQEAgdXNpbmcgbmFtZXNwYWNlIEhUTUxOYW1l czsKIAogSFRNTE1ldGVyRWxlbWVudDo6SFRNTE1ldGVyRWxlbWVudChjb25zdCBRdWFsaWZpZWRO YW1lJiB0YWdOYW1lLCBEb2N1bWVudCogZG9jdW1lbnQpCiAgICAgOiBMYWJlbGFibGVFbGVtZW50 KHRhZ05hbWUsIGRvY3VtZW50KQorICAgICwgbV92YWx1ZSgwKQogICAgICwgbV9oYXNBdXRob3JT aGFkb3dSb290KGZhbHNlKQogewogICAgIEFTU0VSVChoYXNUYWdOYW1lKG1ldGVyVGFnKSk7CkBA IC0yNDgsNiArMjQ5LDcgQEAgUmVuZGVyTWV0ZXIqIEhUTUxNZXRlckVsZW1lbnQ6OnJlbmRlck1l dGVyKCkgY29uc3QKIHZvaWQgSFRNTE1ldGVyRWxlbWVudDo6Y3JlYXRlU2hhZG93U3VidHJlZSgp CiB7CiAgICAgQVNTRVJUKCF1c2VyQWdlbnRTaGFkb3dSb290KCkpOworICAgIEFTU0VSVCghbV92 YWx1ZSk7CiAgICAgICAgICAgIAogICAgIFJlZlB0cjxTaGFkb3dSb290PiByb290ID0gU2hhZG93 Um9vdDo6Y3JlYXRlKHRoaXMsIFNoYWRvd1Jvb3Q6OlVzZXJBZ2VudFNoYWRvd1Jvb3QsIEFTU0VS VF9OT19FWENFUFRJT04pOwogCkBAIC0yNTUsNyArMjU3LDggQEAgdm9pZCBIVE1MTWV0ZXJFbGVt ZW50OjpjcmVhdGVTaGFkb3dTdWJ0cmVlKCkKICAgICByb290LT5hcHBlbmRDaGlsZChpbm5lcik7 CiAKICAgICBSZWZQdHI8TWV0ZXJCYXJFbGVtZW50PiBiYXIgPSBNZXRlckJhckVsZW1lbnQ6OmNy ZWF0ZShkb2N1bWVudCgpKTsKLSAgICBtX3ZhbHVlID0gTWV0ZXJWYWx1ZUVsZW1lbnQ6OmNyZWF0 ZShkb2N1bWVudCgpKTsKKyAgICBSZWZQdHI8TWV0ZXJWYWx1ZUVsZW1lbnQ+IHZhbHVlID0gTWV0 ZXJWYWx1ZUVsZW1lbnQ6OmNyZWF0ZShkb2N1bWVudCgpKTsKKyAgICBtX3ZhbHVlID0gdmFsdWUu Z2V0KCk7CiAgICAgbV92YWx1ZS0+c2V0V2lkdGhQZXJjZW50YWdlKDApOwogICAgIGJhci0+YXBw ZW5kQ2hpbGQobV92YWx1ZSwgQVNTRVJUX05PX0VYQ0VQVElPTik7CiAKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL2h0bWwvSFRNTE1ldGVyRWxlbWVudC5oIGIvU291cmNlL1dlYkNvcmUvaHRt bC9IVE1MTWV0ZXJFbGVtZW50LmgKaW5kZXggOGU2M2M2NGE4YWQwNTdkMzEwNjk1NzljYmVlMmMx NzhlNGI1ZTIwZS4uNThjZjg2NmNhNDBjNDljNWJmNWY1ZDhhZDA2ZWQ4MWVmYjRhMGVjOCAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MTWV0ZXJFbGVtZW50LmgKKysrIGIvU291 cmNlL1dlYkNvcmUvaHRtbC9IVE1MTWV0ZXJFbGVtZW50LmgKQEAgLTg1LDcgKzg1LDcgQEAgcHJp dmF0ZToKICAgICB2b2lkIGRpZEVsZW1lbnRTdGF0ZUNoYW5nZSgpOwogICAgIHZvaWQgY3JlYXRl U2hhZG93U3VidHJlZSgpOwogCi0gICAgUmVmUHRyPE1ldGVyVmFsdWVFbGVtZW50PiBtX3ZhbHVl OworICAgIE1ldGVyVmFsdWVFbGVtZW50KiBtX3ZhbHVlOwogICAgIGJvb2wgbV9oYXNBdXRob3JT aGFkb3dSb290OwogfTsKIAo=