11336 2006-10-17 17:57:34 -0700 HashTable.h not zeroing pointers in constructor 2007-09-30 06:37:06 -0700 1 1 1 Unclassified WebKit JavaScriptCore 420+ PC OS X 10.4 RESOLVED WONTFIX P2 Normal --- 1 mike.emmel webkit-unassigned oldest_to_newest 52234 0 mike.emmel 2006-10-17 17:57:34 -0700 In debug mode the new m_next and m_previous pointers are net set to zero leadin to crashes. 52235 1 11126 mike.emmel 2006-10-17 17:58:49 -0700 Created attachment 11126 zero pointers in constructor HashTable.h 52211 2 11127 mike.emmel 2006-10-17 18:02:50 -0700 Created attachment 11127 Missed the second constructor 52162 3 mjs 2006-10-17 20:49:44 -0700 I don't think this is necessary because the constructors all call addIterator, which initializes both m_next and m_previous. Will ask for Darin's input. 52094 4 mike.emmel 2006-10-17 21:24:39 -0700 Okay the problem is in the assert assert(table->m_iterators != it); it->m_next = table->m_iterators; If their is a problem it triggers before we initalize the m_next so its still garbage and makes it hard to debug. Been a while since I looked at this. The assert could be moved below and still work. 51941 5 darin 2006-10-18 11:35:57 -0700 (In reply to comment #3) > I don't think this is necessary because the constructors all call addIterator, > which initializes both m_next and m_previous. Will ask for Darin's input. Maciej's correct about my original intent. 51921 6 mike.emmel 2006-10-18 12:08:45 -0700 Can you move the assert below the setting of the value then ? So the pointer is not garbage when the assert triggers. From this assert(table->m_iterators != it); it->m_next = table->m_iterators; To this //set pointer so its not garbage it->m_next = table->m_iterators; //assert now if its incorrect. assert(table->m_iterators != it); Its not completely obvious the next pointer is garbage if you trigger this in a debugger. 51522 7 11127 darin 2006-10-21 18:48:52 -0700 Comment on attachment 11127 Missed the second constructor This patch won't compile (missing colon in the second constructor). And I'm not convinced the change is helpful. I understand that this confused you once while debugging, but I'd prefer to just leave this as-is unless you present a more compelling argument to change it. 51342 8 mike.emmel 2006-10-22 00:02:38 -0700 I don't think the patch is the right answer in the first place. If you do hit the assert your dealing with some intresting info in a debugger. And it does its job below. I've got no strong feelings either way it was just confusing at the time. The reason I hit it the first time is actually a bit more involved I had compiled JavaScriptCore and WebKit with different debug flags but this is more a build/linking issue. From my experience this does not seem to work. 11126 2006-10-17 17:58:49 -0700 2006-10-17 18:02:50 -0700 zero pointers in constructor HashTable.h hash.diff text/plain 544 mike.emmel ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL3d0Zi9IYXNoVGFibGUuaCBiL0phdmFTY3JpcHRD b3JlL3d0Zi9IYXNoVGFibGUuaAppbmRleCA0ZTA3ZWVjLi40Zjk2ZDU2IDEwMDc1NQotLS0gYS9K YXZhU2NyaXB0Q29yZS93dGYvSGFzaFRhYmxlLmgKKysrIGIvSmF2YVNjcmlwdENvcmUvd3RmL0hh c2hUYWJsZS5oCkBAIC05OCw2ICs5OCwxMCBAQCAjZW5kaWYKIAogICAgICAgICBIYXNoVGFibGVD b25zdEl0ZXJhdG9yKGNvbnN0IEhhc2hUYWJsZVR5cGUqIHRhYmxlLCBQb2ludGVyVHlwZSBwb3Np dGlvbiwgUG9pbnRlclR5cGUgZW5kUG9zaXRpb24pCiAgICAgICAgICAgICA6IG1fcG9zaXRpb24o cG9zaXRpb24pLCBtX2VuZFBvc2l0aW9uKGVuZFBvc2l0aW9uKQorI2lmIENIRUNLX0hBU0hUQUJM RV9JVEVSQVRPUlMKKyAgICAgICAgICAgICwgbV9uZXh0KDApLCBtX3ByZXZpb3VzKDApCisjZW5k aWYKKwogICAgICAgICB7CiAgICAgICAgICAgICBhZGRJdGVyYXRvcih0YWJsZSwgdGhpcyk7CiAg ICAgICAgICAgICBza2lwRW1wdHlCdWNrZXRzKCk7Cg== 11127 2006-10-17 18:02:50 -0700 2006-10-21 18:48:52 -0700 Missed the second constructor hash.diff text/plain 752 mike.emmel ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL3d0Zi9IYXNoVGFibGUuaCBiL0phdmFTY3JpcHRD b3JlL3d0Zi9IYXNoVGFibGUuaAppbmRleCA0ZTA3ZWVjLi4zZDYwNTVlIDEwMDc1NQotLS0gYS9K YXZhU2NyaXB0Q29yZS93dGYvSGFzaFRhYmxlLmgKKysrIGIvSmF2YVNjcmlwdENvcmUvd3RmL0hh c2hUYWJsZS5oCkBAIC05OCw2ICs5OCwxMCBAQCAjZW5kaWYKIAogICAgICAgICBIYXNoVGFibGVD b25zdEl0ZXJhdG9yKGNvbnN0IEhhc2hUYWJsZVR5cGUqIHRhYmxlLCBQb2ludGVyVHlwZSBwb3Np dGlvbiwgUG9pbnRlclR5cGUgZW5kUG9zaXRpb24pCiAgICAgICAgICAgICA6IG1fcG9zaXRpb24o cG9zaXRpb24pLCBtX2VuZFBvc2l0aW9uKGVuZFBvc2l0aW9uKQorI2lmIENIRUNLX0hBU0hUQUJM RV9JVEVSQVRPUlMKKyAgICAgICAgICAgICwgbV9uZXh0KDApLCBtX3ByZXZpb3VzKDApCisjZW5k aWYKKwogICAgICAgICB7CiAgICAgICAgICAgICBhZGRJdGVyYXRvcih0YWJsZSwgdGhpcyk7CiAg ICAgICAgICAgICBza2lwRW1wdHlCdWNrZXRzKCk7CkBAIC0xMDUsNiArMTA5LDkgQEAgI2VuZGlm CiAKICAgICBwdWJsaWM6CiAgICAgICAgIEhhc2hUYWJsZUNvbnN0SXRlcmF0b3IoKQorI2lmIENI RUNLX0hBU0hUQUJMRV9JVEVSQVRPUlMKKyAgICAgICAgICBtX25leHQoMCksIG1fcHJldmlvdXMo MCkKKyNlbmRpZgogICAgICAgICB7CiAgICAgICAgICAgICBhZGRJdGVyYXRvcigwLCB0aGlzKTsK ICAgICAgICAgfQo=