191193 2018-11-02 08:03:45 -0700 Metadata should not be copyable 2018-11-04 20:51:50 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 tzagallo tzagallo commit-queue ews-watchlist keith_miller mark.lam msaboff saam webkit-bug-importer oldest_to_newest 1474487 0 tzagallo 2018-11-02 08:03:45 -0700 ... 1474488 1 353702 tzagallo 2018-11-02 08:07:07 -0700 Created attachment 353702 Patch 1474489 2 keith_miller 2018-11-02 08:09:26 -0700 r=me. 1474504 3 353702 commit-queue 2018-11-02 08:47:04 -0700 Comment on attachment 353702 Patch Clearing flags on attachment: 353702 Committed r237734: <https://trac.webkit.org/changeset/237734> 1474505 4 commit-queue 2018-11-02 08:47:06 -0700 All reviewed patches have been landed. Closing bug. 1474506 5 webkit-bug-importer 2018-11-02 08:48:33 -0700 <rdar://problem/45762874> 1474515 6 ddkilzer 2018-11-02 09:30:19 -0700 <rdar://problem/45682358> 1474821 7 353702 saam 2018-11-03 15:37:48 -0700 Comment on attachment 353702 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353702&action=review > Source/JavaScriptCore/ChangeLog:8 > + We should only ever hold references to the entry in the metadata table. Why? What about a test for this? 1474847 8 353702 tzagallo 2018-11-03 23:07:06 -0700 Comment on attachment 353702 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353702&action=review >> Source/JavaScriptCore/ChangeLog:8 >> + We should only ever hold references to the entry in the metadata table. > > Why? What about a test for this? Because the metadata is mutable, so it's nice to avoid cases like in CodeBlock below, where I was accidentally copying and then mutating a local copy. You're right, I should have added a test. I'll add it now. 1474853 9 353702 mark.lam 2018-11-04 00:48:33 -0700 Comment on attachment 353702 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353702&action=review >>> Source/JavaScriptCore/ChangeLog:8 >>> + We should only ever hold references to the entry in the metadata table. >> >> Why? What about a test for this? > > Because the metadata is mutable, so it's nice to avoid cases like in CodeBlock below, where I was accidentally copying and then mutating a local copy. You're right, I should have added a test. I'll add it now. I think this is Saam’s way of asking you to add the “why” to the ChangeLog to document this reasoning for posterity. Can you add your explanation in the ChangeLog here, particularly the part about the sort of bugs this prevents? Thanks. 1474854 10 353702 tzagallo 2018-11-04 00:50:51 -0700 Comment on attachment 353702 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353702&action=review >>>> Source/JavaScriptCore/ChangeLog:8 >>>> + We should only ever hold references to the entry in the metadata table. >>> >>> Why? What about a test for this? >> >> Because the metadata is mutable, so it's nice to avoid cases like in CodeBlock below, where I was accidentally copying and then mutating a local copy. You're right, I should have added a test. I'll add it now. > > I think this is Saam’s way of asking you to add the “why” to the ChangeLog to document this reasoning for posterity. Can you add your explanation in the ChangeLog here, particularly the part about the sort of bugs this prevents? Thanks. Yeah, I thought that might be the case. Is it ok to edit a previous ChangeLog entry? Should I add a new ChangeLog entry to say that I'm editing a previous ChangeLog entry? 1474948 11 saam 2018-11-04 20:51:50 -0800 (In reply to Tadeu Zagallo from comment #10) > Comment on attachment 353702 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353702&action=review > > >>>> Source/JavaScriptCore/ChangeLog:8 > >>>> + We should only ever hold references to the entry in the metadata table. > >>> > >>> Why? What about a test for this? > >> > >> Because the metadata is mutable, so it's nice to avoid cases like in CodeBlock below, where I was accidentally copying and then mutating a local copy. You're right, I should have added a test. I'll add it now. > > > > I think this is Saam’s way of asking you to add the “why” to the ChangeLog to document this reasoning for posterity. Can you add your explanation in the ChangeLog here, particularly the part about the sort of bugs this prevents? Thanks. > > Yeah, I thought that might be the case. Is it ok to edit a previous > ChangeLog entry? Should I add a new ChangeLog entry to say that I'm editing > a previous ChangeLog entry? Sorry for the ambiguity. I did mean the “why” as a way of asking for this to be documented in the change log. Personally, I’ve never gone back and edited previous change log entries. I’m not sure what the official WebKit stance on that is. I think this bugzilla will act as a point of reference if anyone needs to dig into this more in the future. You could also document it in the changelog for the added test, but that won’t be found as easily as the original commit. 353702 2018-11-02 08:07:07 -0700 2018-11-02 08:47:04 -0700 Patch bug-191193-20181102160657.patch text/plain 3843 tzagallo U3VidmVyc2lvbiBSZXZpc2lvbjogMjM3NzAyCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA2 MWRiOTNiY2FhM2M1MjYxNTNkOGY5ZGU1OWVjZGQ1NWEzZjM5YzdmLi5iMjI2NDNiZDFhNWY0OGFk ZDRiODMxN2FiYWQ5OTZiYzI3MTI4NmVmIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOCBAQAorMjAxOC0xMS0wMiAgVGFkZXUgWmFnYWxsbyAgPHR6YWdhbGxvQGFwcGxlLmNv bT4KKworICAgICAgICBNZXRhZGF0YSBzaG91bGQgbm90IGJlIGNvcHlhYmxlCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTExOTMKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXZSBzaG91bGQgb25seSBldmVy IGhvbGQgcmVmZXJlbmNlcyB0byB0aGUgZW50cnkgaW4gdGhlIG1ldGFkYXRhIHRhYmxlLgorCisg ICAgICAgICogYnl0ZWNvZGUvQ29kZUJsb2NrLmNwcDoKKyAgICAgICAgKEpTQzo6Q29kZUJsb2Nr OjpmaW5hbGl6ZUxMSW50SW5saW5lQ2FjaGVzKToKKyAgICAgICAgKiBkZmcvREZHQnl0ZUNvZGVQ YXJzZXIuY3BwOgorICAgICAgICAoSlNDOjpERkc6OkJ5dGVDb2RlUGFyc2VyOjpwYXJzZUJsb2Nr KToKKyAgICAgICAgKiBnZW5lcmF0b3IvTWV0YWRhdGEucmI6CisKIDIwMTgtMTAtMzEgIERldmlu IFJvdXNzbyAgPGRyb3Vzc29AYXBwbGUuY29tPgogCiAgICAgICAgIFdlYiBJbnNwZWN0b3I6IENh bnZhczogY3JlYXRlIGEgc2V0dGluZyBmb3IgYXV0by1yZWNvcmRpbmcgbmV3bHkgY3JlYXRlZCBj b250ZXh0cwpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2J5dGVjb2RlL0NvZGVC bG9jay5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYnl0ZWNvZGUvQ29kZUJsb2NrLmNwcApp bmRleCBlZGZlYzFhOTNmNDZjYWZlMTgwMWFkM2ExY2Y0MGMxZDBiMDRkNjJkLi5hZjY5MGI5NDFi YjEwYzkxMmYwM2RkMmU2ZGY2ZWE0NzU5YTU0ZWY2IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvYnl0ZWNvZGUvQ29kZUJsb2NrLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvYnl0ZWNvZGUvQ29kZUJsb2NrLmNwcApAQCAtMTI4MCw3ICsxMjgwLDcgQEAgdm9pZCBDb2Rl QmxvY2s6OmZpbmFsaXplTExJbnRJbmxpbmVDYWNoZXMoKQogICAgICAgICAgICAgLy8gUmlnaHQg bm93IHRoaXMgaXNuJ3Qgc3RyaWN0bHkgbmVjZXNzYXJ5LiBBbnkgc3ltYm9sIHRhYmxlcyB0aGF0 IHRoaXMgd2lsbCByZWZlciB0bwogICAgICAgICAgICAgLy8gYXJlIGZvciBvdXRlciBmdW5jdGlv bnMsIGFuZCB3ZSByZWZlciB0byB0aG9zZSBmdW5jdGlvbnMgc3Ryb25nbHksIGFuZCB0aGV5IHJl ZmVyCiAgICAgICAgICAgICAvLyB0byB0aGUgc3ltYm9sIHRhYmxlIHN0cm9uZ2x5LiBCdXQgaXQn cyBuaWNlIHRvIGJlIG9uIHRoZSBzYWZlIHNpZGUuCi0gICAgICAgICAgICBhdXRvIG1ldGFkYXRh ID0gY3VySW5zdHJ1Y3Rpb24tPmFzPE9wUmVzb2x2ZVNjb3BlPigpLm1ldGFkYXRhKHRoaXMpOwor ICAgICAgICAgICAgYXV0byYgbWV0YWRhdGEgPSBjdXJJbnN0cnVjdGlvbi0+YXM8T3BSZXNvbHZl U2NvcGU+KCkubWV0YWRhdGEodGhpcyk7CiAgICAgICAgICAgICBXcml0ZUJhcnJpZXJCYXNlPFN5 bWJvbFRhYmxlPiYgc3ltYm9sVGFibGUgPSBtZXRhZGF0YS5zeW1ib2xUYWJsZTsKICAgICAgICAg ICAgIGlmICghc3ltYm9sVGFibGUgfHwgSGVhcDo6aXNNYXJrZWQoc3ltYm9sVGFibGUuZ2V0KCkp KQogICAgICAgICAgICAgICAgIGJyZWFrOwpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRD b3JlL2RmZy9ERkdCeXRlQ29kZVBhcnNlci5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZn L0RGR0J5dGVDb2RlUGFyc2VyLmNwcAppbmRleCA3ZGVkODNmMDQ5NzRhZTA4N2YxNjEyMGRkNzAw MDdmZDY1OTI2OWJmLi40M2VjMGZkZGQ4MWZjNjYxYWM3ZjY1NzNhN2UxZjU1MTM4NDEyMDc5IDEw MDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5dGVDb2RlUGFyc2VyLmNw cAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5dGVDb2RlUGFyc2VyLmNwcApA QCAtNDcxMCw3ICs0NzEwLDcgQEAgdm9pZCBCeXRlQ29kZVBhcnNlcjo6cGFyc2VCbG9jayh1bnNp Z25lZCBsaW1pdCkKICAgICAgICAgY2FzZSBvcF90b190aGlzOiB7CiAgICAgICAgICAgICBOb2Rl KiBvcDEgPSBnZXRUaGlzKCk7CiAgICAgICAgICAgICBpZiAob3AxLT5vcCgpICE9IFRvVGhpcykg ewotICAgICAgICAgICAgICAgIGF1dG8gbWV0YWRhdGEgPSBjdXJyZW50SW5zdHJ1Y3Rpb24tPmFz PE9wVG9UaGlzPigpLm1ldGFkYXRhKGNvZGVCbG9jayk7CisgICAgICAgICAgICAgICAgYXV0byYg bWV0YWRhdGEgPSBjdXJyZW50SW5zdHJ1Y3Rpb24tPmFzPE9wVG9UaGlzPigpLm1ldGFkYXRhKGNv ZGVCbG9jayk7CiAgICAgICAgICAgICAgICAgU3RydWN0dXJlKiBjYWNoZWRTdHJ1Y3R1cmUgPSBt ZXRhZGF0YS5jYWNoZWRTdHJ1Y3R1cmUuZ2V0KCk7CiAgICAgICAgICAgICAgICAgaWYgKG1ldGFk YXRhLnRvVGhpc1N0YXR1cyAhPSBUb1RoaXNPSwogICAgICAgICAgICAgICAgICAgICB8fCAhY2Fj aGVkU3RydWN0dXJlCkBAIC02MTYzLDcgKzYxNjMsNyBAQCB2b2lkIEJ5dGVDb2RlUGFyc2VyOjpw YXJzZUJsb2NrKHVuc2lnbmVkIGxpbWl0KQogCiAgICAgICAgIGNhc2Ugb3BfZ2V0X2Zyb21fc2Nv cGU6IHsKICAgICAgICAgICAgIGF1dG8gYnl0ZWNvZGUgPSBjdXJyZW50SW5zdHJ1Y3Rpb24tPmFz PE9wR2V0RnJvbVNjb3BlPigpOwotICAgICAgICAgICAgYXV0byBtZXRhZGF0YSA9IGJ5dGVjb2Rl Lm1ldGFkYXRhKGNvZGVCbG9jayk7CisgICAgICAgICAgICBhdXRvJiBtZXRhZGF0YSA9IGJ5dGVj b2RlLm1ldGFkYXRhKGNvZGVCbG9jayk7CiAgICAgICAgICAgICB1bnNpZ25lZCBpZGVudGlmaWVy TnVtYmVyID0gbV9pbmxpbmVTdGFja1RvcC0+bV9pZGVudGlmaWVyUmVtYXBbYnl0ZWNvZGUudmFy XTsKICAgICAgICAgICAgIFVuaXF1ZWRTdHJpbmdJbXBsKiB1aWQgPSBtX2dyYXBoLmlkZW50aWZp ZXJzKClbaWRlbnRpZmllck51bWJlcl07CiAgICAgICAgICAgICBSZXNvbHZlVHlwZSByZXNvbHZl VHlwZSA9IG1ldGFkYXRhLmdldFB1dEluZm8ucmVzb2x2ZVR5cGUoKTsKZGlmZiAtLWdpdCBhL1Nv dXJjZS9KYXZhU2NyaXB0Q29yZS9nZW5lcmF0b3IvTWV0YWRhdGEucmIgYi9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvZ2VuZXJhdG9yL01ldGFkYXRhLnJiCmluZGV4IGJmMDZjNGM2OWI5MDk1NmJhMjk0 NzEzN2FhMzgxY2VmYmM3NDllN2UuLjZmNDJmNzcwOGRhOTUyMmM1ODg3NmQyYTY0NzkwMTM1YjE1 NDk5NzYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9nZW5lcmF0b3IvTWV0YWRh dGEucmIKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2dlbmVyYXRvci9NZXRhZGF0YS5yYgpA QCAtNjMsNiArNjMsOSBAQCBjbGFzcyBNZXRhZGF0YQogCiAgICAgICAgIDw8LUVPRgogICAgICAg ICBzdHJ1Y3QgTWV0YWRhdGEgeworICAgICAgICAgICAgV1RGX01BS0VfTk9OQ09QWUFCTEUoTWV0 YWRhdGEpOworCisgICAgICAgIHB1YmxpYzoKICAgICAgICAgICAgIE1ldGFkYXRhKGNvbnN0ICN7 b3AuY2FwaXRhbGl6ZWRfbmFtZX0mI3siIF9fb3AiIGlmIGluaXRzfSkKICAgICAgICAgICAgICN7 aW5pdHN9CiAgICAgICAgICAgICB7IH0K