45358 2010-09-07 19:25:04 -0700 DatasetDOMStringMap does not have the right memory model 2010-12-14 01:58:52 -0800 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 arv arv eric jamesr mjs sam oldest_to_newest 275546 0 arv 2010-09-07 19:25:04 -0700 This came up in irc today when we talked about classList DOMTokenList which has the same behavior/issue: The dataset needs to not fail when the element goes away. othermaciej: I think the following would work: [7:06pm] othermaciej: - DOMTokenList is a normal RefCounted [7:06pm] othermaciej: - Element keeps a ref to it via rare data struct [7:06pm] othermaciej: - Element destructor presumably already cleans up any rare data that may exist, so rare data can clear out DOMTokenList's backpointer to its element [7:07pm] othermaciej: - At JS/GC level, Element wrapper keeps DOMTokenList wrapper alive [7:15pm] arv: jamesr, othermaciej: DatasetDOMStringMap uses the same pattern. I assume that needs to be updated too? [7:15pm] othermaciej: arv: ask and/or yell at weinig 275612 1 66843 sam 2010-09-07 22:46:03 -0700 Created attachment 66843 Patch 275613 2 sam 2010-09-07 22:47:48 -0700 I disagree with Maciej that we need to completely alter the memory management of DOMStringMap. The model I used is idiomatic in WebCore. For instance, It is used by CanvasRenderingContext and ValidityState. That said, I was forgetting to mark the dataset, so I have attached a patch to fix it. 275614 3 66843 mitz 2010-09-07 22:52:18 -0700 Comment on attachment 66843 Patch > + DOMStringMap* optionalDataset() const; That’s an awful use of “optional”. r+ only because it’s already being used like this. 275617 4 sam 2010-09-07 22:59:57 -0700 Fixed in r66954. Filed https://bugs.webkit.org/show_bug.cgi?id=45367 for a better name. 275959 5 66843 arv 2010-09-08 11:32:05 -0700 Comment on attachment 66843 Patch This test does not seem correct to me. Your test seems to test that we get back the same wrapper object? Don't you want to GC the element and ensure that the dataset does not blow up when the element goes away? (and that the element can be GC'd when there are only references to the dataset) Is there a reason why you use __proto__ here. Could you have used any property name? 276073 6 sam 2010-09-08 14:29:16 -0700 (In reply to comment #5) > (From update of attachment 66843 [details]) > This test does not seem correct to me. > Can you elaborate on what is incorrect about it? > Your test seems to test that we get back the same wrapper object? Yes. > > Don't you want to GC the element and ensure that the dataset does not blow up when the element goes away? (and that the element can be GC'd when there are only references to the dataset) I am not entirely sure what you are getting at. Can you provide a test case? > Is there a reason why you use __proto__ here. Could you have used any property name? Yeah, I was being sneaky due to the fact that dataset intercepts all property sets except for a few built in properties and puts them in the DOM as attributes, so it wouldn't be testing the right thing? 280940 7 arv 2010-09-17 15:42:49 -0700 Would it not be better to tie the lifetime of the dataset to the lifetime of the element it came from? Also see Bug 45426 281216 8 sam 2010-09-18 16:55:55 -0700 (In reply to comment #7) > Would it not be better to tie the lifetime of the dataset to the lifetime of the element it came from? > > Also see Bug 45426 That is what is done. 281644 9 arv 2010-09-20 10:29:49 -0700 Forgive my ignorance. I think what is confusing is that markDOMObjectWrapper takes an argument called globalData. 321839 10 eric 2010-12-14 01:58:52 -0800 Looks like this was landed but just not closed. 66843 2010-09-07 22:46:03 -0700 2010-09-08 11:32:05 -0700 Patch dataset.diff text/plain 5512 sam SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2Njk1MikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjIgQEAKKzIwMTAtMDktMDcgIFNhbSBXZWluaWcgIDxzYW1Ad2Via2l0Lm9yZz4K KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBEYXRhc2V0 RE9NU3RyaW5nTWFwIGRvZXMgbm90IGhhdmUgdGhlIHJpZ2h0IG1lbW9yeSBtb2RlbAorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDUzNTgKKworICAgICAg ICBUZXN0OiBmYXN0L2RvbS9kYXRhc2V0LWdjLmh0bWwKKworICAgICAgICAqIGJpbmRpbmdzL2pz L0pTRWxlbWVudEN1c3RvbS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpKU0VsZW1lbnQ6Om1hcmtD aGlsZHJlbik6CisgICAgICAgIE1hcmsgdGhlIGRhdGFzZXQgaWYgaXQgZXhpc3RzLgorCisgICAg ICAgICogZG9tL0VsZW1lbnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RWxlbWVudDo6b3B0aW9u YWxEYXRhc2V0KToKKyAgICAgICAgKiBkb20vRWxlbWVudC5oOgorICAgICAgICBFeHBvc2UgYSB3 YXkgdG8gZ2V0IHRoZSBkYXRhc2V0IG9yIG51bGwgKGRlcGVuZGluZyBvbiBpZiBhbnlvbmUgdGhp bmcKKyAgICAgICAgaGFzIGZvcmNlZCBpdHMgY3JlYXRpb24geWV0KS4KKwogMjAxMC0wOS0wNyAg SmFuIEUgSGFuc3NlbiAgPGpoYW5zc2VuQHNlbmNoYS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQg YnkgQW5kcmVhcyBLbGluZy4KSW5kZXg6IFdlYkNvcmUvYmluZGluZ3MvanMvSlNFbGVtZW50Q3Vz dG9tLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2JpbmRpbmdzL2pzL0pTRWxlbWVudEN1c3Rv bS5jcHAJKHJldmlzaW9uIDY2OTM2KQorKysgV2ViQ29yZS9iaW5kaW5ncy9qcy9KU0VsZW1lbnRD dXN0b20uY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02MCw2ICs2MCw4IEBAIHZvaWQgSlNFbGVtZW50 OjptYXJrQ2hpbGRyZW4oTWFya1N0YWNrJiAKICAgICBKU0dsb2JhbERhdGEmIGdsb2JhbERhdGEg PSAqSGVhcDo6aGVhcCh0aGlzKS0+Z2xvYmFsRGF0YSgpOwogCiAgICAgbWFya0RPTU9iamVjdFdy YXBwZXIobWFya1N0YWNrLCBnbG9iYWxEYXRhLCBlbGVtZW50LT5hdHRyaWJ1dGVNYXAoKSk7Cisg ICAgbWFya0RPTU9iamVjdFdyYXBwZXIobWFya1N0YWNrLCBnbG9iYWxEYXRhLCBlbGVtZW50LT5v cHRpb25hbERhdGFzZXQoKSk7CisKICAgICBpZiAoZWxlbWVudC0+aXNTdHlsZWRFbGVtZW50KCkp CiAgICAgICAgIG1hcmtET01PYmplY3RXcmFwcGVyKG1hcmtTdGFjaywgZ2xvYmFsRGF0YSwgc3Rh dGljX2Nhc3Q8U3R5bGVkRWxlbWVudCo+KGVsZW1lbnQpLT5pbmxpbmVTdHlsZURlY2woKSk7CiB9 CkluZGV4OiBXZWJDb3JlL2RvbS9FbGVtZW50LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2Rv bS9FbGVtZW50LmNwcAkocmV2aXNpb24gNjY5MzYpCisrKyBXZWJDb3JlL2RvbS9FbGVtZW50LmNw cAkod29ya2luZyBjb3B5KQpAQCAtMTU1OCw2ICsxNTU4LDEzIEBAIERPTVN0cmluZ01hcCogRWxl bWVudDo6ZGF0YXNldCgpCiAgICAgcmV0dXJuIGRhdGEtPm1fZGF0YXNldERPTVN0cmluZ01hcC5n ZXQoKTsKIH0KIAorRE9NU3RyaW5nTWFwKiBFbGVtZW50OjpvcHRpb25hbERhdGFzZXQoKSBjb25z dAoreworICAgIGlmICghaGFzUmFyZURhdGEoKSkKKyAgICAgICAgcmV0dXJuIDA7CisgICAgcmV0 dXJuIHJhcmVEYXRhKCktPm1fZGF0YXNldERPTVN0cmluZ01hcC5nZXQoKTsKK30KKwogS1VSTCBF bGVtZW50OjpnZXRVUkxBdHRyaWJ1dGUoY29uc3QgUXVhbGlmaWVkTmFtZSYgbmFtZSkgY29uc3QK IHsKICNpZiAhQVNTRVJUX0RJU0FCTEVECkluZGV4OiBXZWJDb3JlL2RvbS9FbGVtZW50LmgKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gV2ViQ29yZS9kb20vRWxlbWVudC5oCShyZXZpc2lvbiA2NjkzNikKKysrIFdl YkNvcmUvZG9tL0VsZW1lbnQuaAkod29ya2luZyBjb3B5KQpAQCAtMjcwLDYgKzI3MCw3IEBAIHB1 YmxpYzoKICAgICBib29sIHdlYmtpdE1hdGNoZXNTZWxlY3Rvcihjb25zdCBTdHJpbmcmIHNlbGVj dG9ycywgRXhjZXB0aW9uQ29kZSYpOwogCiAgICAgRE9NU3RyaW5nTWFwKiBkYXRhc2V0KCk7Cisg ICAgRE9NU3RyaW5nTWFwKiBvcHRpb25hbERhdGFzZXQoKSBjb25zdDsKIAogICAgIHZpcnR1YWwg Ym9vbCBpc0Zvcm1Db250cm9sRWxlbWVudCgpIGNvbnN0IHsgcmV0dXJuIGZhbHNlOyB9CiAgICAg dmlydHVhbCBib29sIGlzRW5hYmxlZEZvcm1Db250cm9sKCkgY29uc3QgeyByZXR1cm4gdHJ1ZTsg fQpJbmRleDogTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3Rz L0NoYW5nZUxvZwkocmV2aXNpb24gNjY5NTIpCisrKyBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHdv cmtpbmcgY29weSkKQEAgLTEsMyArMSwxNiBAQAorMjAxMC0wOS0wNyAgU2FtIFdlaW5pZyAgPHNh bUB3ZWJraXQub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIERhdGFzZXRET01TdHJpbmdNYXAgZG9lcyBub3QgaGF2ZSB0aGUgcmlnaHQgbWVtb3J5 IG1vZGVsCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00 NTM1OAorCisgICAgICAgIFRlc3QgdGhhdCB0aGUgZGF0YXNldCBwZXJzaXN0cyBnYy4KKworICAg ICAgICAqIGZhc3QvZG9tL2RhdGFzZXQtZ2MtZXhwZWN0ZWQudHh0OiBBZGRlZC4KKyAgICAgICAg KiBmYXN0L2RvbS9kYXRhc2V0LWdjLmh0bWw6IEFkZGVkLgorICAgICAgICAqIGZhc3QvZG9tL3Nj cmlwdC10ZXN0cy9kYXRhc2V0LWdjLmpzOiBBZGRlZC4KKwogMjAxMC0wOS0wNyAgSmFuIEUgSGFu c3NlbiAgPGpoYW5zc2VuQHNlbmNoYS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQW5kcmVh cyBLbGluZy4KSW5kZXg6IExheW91dFRlc3RzL2Zhc3QvZG9tL2RhdGFzZXQtZ2MtZXhwZWN0ZWQu dHh0Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Zhc3QvZG9tL2RhdGFzZXQtZ2MtZXhwZWN0 ZWQudHh0CShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMvZmFzdC9kb20vZGF0YXNldC1nYy1l eHBlY3RlZC50eHQJKHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMTIgQEAKK1RoaXMgdGVzdHMgdGhh dCBjdXN0b20gcHJvcGVydGllcyBvbiBlbGVtZW50LmRhdGFzZXQgcGVyc2lzdCBHQy4KKworT24g c3VjY2VzcywgeW91IHdpbGwgc2VlIGEgc2VyaWVzIG9mICJQQVNTIiBtZXNzYWdlcywgZm9sbG93 ZWQgYnkgIlRFU1QgQ09NUExFVEUiLgorCisKK1BBU1MgZC5kYXRhc2V0Ll9fcHJvdG9fXyBpcyBu b24tbnVsbC4KK1BBU1MgZC5kYXRhc2V0Ll9fcHJvdG9fXyBpcyBudWxsCitQQVNTIGQuZGF0YXNl dC5fX3Byb3RvX18gaXMgbnVsbAorUEFTUyBzdWNjZXNzZnVsbHlQYXJzZWQgaXMgdHJ1ZQorCitU RVNUIENPTVBMRVRFCisKSW5kZXg6IExheW91dFRlc3RzL2Zhc3QvZG9tL2RhdGFzZXQtZ2MuaHRt bAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9mYXN0L2RvbS9kYXRhc2V0LWdjLmh0bWwJKHJl dmlzaW9uIDApCisrKyBMYXlvdXRUZXN0cy9mYXN0L2RvbS9kYXRhc2V0LWdjLmh0bWwJKHJldmlz aW9uIDApCkBAIC0wLDAgKzEsMTMgQEAKKzwhRE9DVFlQRSBIVE1MIFBVQkxJQyAiLS8vSUVURi8v RFREIEhUTUwvL0VOIj4KKzxodG1sPgorPGhlYWQ+Cis8bGluayByZWw9InN0eWxlc2hlZXQiIGhy ZWY9Ii4uL2pzL3Jlc291cmNlcy9qcy10ZXN0LXN0eWxlLmNzcyI+Cis8c2NyaXB0IHNyYz0iLi4v anMvcmVzb3VyY2VzL2pzLXRlc3QtcHJlLmpzIj48L3NjcmlwdD4KKzwvaGVhZD4KKzxib2R5Pgor PHAgaWQ9ImRlc2NyaXB0aW9uIj48L3A+Cis8ZGl2IGlkPSJjb25zb2xlIj48L2Rpdj4KKzxzY3Jp cHQgc3JjPSJzY3JpcHQtdGVzdHMvZGF0YXNldC1nYy5qcyI+PC9zY3JpcHQ+Cis8c2NyaXB0IHNy Yz0iLi4vanMvcmVzb3VyY2VzL2pzLXRlc3QtcG9zdC5qcyI+PC9zY3JpcHQ+Cis8L2JvZHk+Cis8 L2h0bWw+CkluZGV4OiBMYXlvdXRUZXN0cy9mYXN0L2RvbS9zY3JpcHQtdGVzdHMvZGF0YXNldC1n Yy5qcwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9mYXN0L2RvbS9zY3JpcHQtdGVzdHMvZGF0 YXNldC1nYy5qcwkocmV2aXNpb24gMCkKKysrIExheW91dFRlc3RzL2Zhc3QvZG9tL3NjcmlwdC10 ZXN0cy9kYXRhc2V0LWdjLmpzCShyZXZpc2lvbiAwKQpAQCAtMCwwICsxLDM0IEBACitkZXNjcmlw dGlvbigiVGhpcyB0ZXN0cyB0aGF0IGN1c3RvbSBwcm9wZXJ0aWVzIG9uIGVsZW1lbnQuZGF0YXNl dCBwZXJzaXN0IEdDLiIpOworCitmdW5jdGlvbiBnYygpCit7CisgICAgaWYgKHdpbmRvdy5HQ0Nv bnRyb2xsZXIpCisgICAgICAgIHJldHVybiBHQ0NvbnRyb2xsZXIuY29sbGVjdCgpOworCisgICAg Zm9yICh2YXIgaSA9IDA7IGkgPCAxMDAwMDsgaSsrKSB7CisgICAgICAgIHZhciBzID0gbmV3IFN0 cmluZygiIik7CisgICAgfQorfQorCisKK3ZhciBkID0gZG9jdW1lbnQuY3JlYXRlRWxlbWVudCgi ZGl2Iik7CisKKy8vIEVuc3VyZSB0aGUgZGF0YXNldCBpcyBjcmVhdGVkLgordmFyIGRhdGFzZXQg PSBkLmRhdGFzZXQ7CisKKy8vIEVuc3VyZSB0aGVyZSBpcyBhIF9fcHJvdG9fXyBiZWZvcmUgdGVz dC4KK3Nob3VsZEJlTm9uTnVsbCgiZC5kYXRhc2V0Ll9fcHJvdG9fXyIpOworCisvLyBTZXQgX19w cm90b19fIHRvIG5vbi1zdGFydGluZyB2YWx1ZS4gKE11c3QgYmUgbnVsbCBkbyB0byBfX3Byb3Rv X18gcmVzdHJpY3Rpb25zKS4KK2QuZGF0YXNldC5fX3Byb3RvX18gPSBudWxsOworc2hvdWxkQmVO dWxsKCJkLmRhdGFzZXQuX19wcm90b19fIik7CisKKy8vIE51bGwgb3V0IHJlZmVyZW5jZSB0byB0 aGUgZGF0YXNldC4gCitkYXRhc2V0ID0gbnVsbDsKKworZ2MoKTsKKworLy8gVGVzdCB0aGF0IHRo ZSBudWxsIHBlcnNpc3RlZCB0aGUgR0MuCitzaG91bGRCZU51bGwoImQuZGF0YXNldC5fX3Byb3Rv X18iKTsKKwordmFyIHN1Y2Nlc3NmdWxseVBhcnNlZCA9IHRydWU7Cg==