8753 2006-05-05 15:27:29 -0700 REGRESSION (r14032): Incorrect and non-uniform text zoom applied after going forward and backward 2013-06-04 11:21:44 -0700 1 1 1 Unclassified WebKit Layout and Rendering 420+ Mac OS X 10.4 RESOLVED INVALID InRadar, NeedsReduction, Regression P2 Normal --- 1 mitz webkit-unassigned darin emacemac7 ian mjs robert oldest_to_newest 41364 0 mitz 2006-05-05 15:27:29 -0700 Steps to reproduce: 1. Go to http://bugzilla.opendarwin.org/show_bug.cgi?id=8132 2. Click the first attachment (Proposed patch) 3. Choose View > Make Text Smaller 4. Choose History > Back 5. Choose History > Forward 6. Choose History > Back Expected result: Text should be smaller in the bug page both in steps 4 and 6. Text zoom should affect both the text and the form controls. Actual result: In step 4, text and form controls are zoomed out as expected. In step 6, text appears in default size (bigger than expected), while form controls are smaller. 41385 1 mitz 2006-05-06 05:57:47 -0700 I noticed two problems with the new form element state code: 1) In Document::formElementsState, Vector<String> stateVector(m_formElementsWithState.size() * 3); should be Vector<String> stateVector; stateVector.reserveCapacity(m_formElementsWithState.size() * 3); 2) Both the keys and the values in the FormElementStateMap get "double-freed". I'm not sure if this is a problem in the hash map code or in how it's used here. Here's what happens: when you remove an entry from the map, it calls the FormElementKey destructor (which derefs the name and type) then it assigns the deletedValue to the FormElementKey (so the = operator derefs the name and type again). You can work around this by setting name and type to 0 in the destructor. Then I noticed that the same happens with the values: ~Vector is called (which calls the impl's destructor), then the empty Vector is assigned to the vector (which will eventually call the impl's destructor again). Perhaps the solution is as simple as setting the correct traits (needsDestruction = false) but I don't really understand how it's supposed to work. 41386 2 mitz 2006-05-06 06:02:38 -0700 (In reply to comment #1) > Perhaps the solution is as simple as setting the correct traits > (needsDestruction = false) It can't be exactly that, since HashMap::remove doesn't even check needsDestruction. 41398 3 darin 2006-05-06 11:41:57 -0700 Aggh! I'll take care of this right away. 41399 4 darin 2006-05-06 11:44:00 -0700 (In reply to comment #1) > 2) Both the keys and the values in the FormElementStateMap get "double-freed". > I'm not sure if this is a problem in the hash map code or in how it's used > here. It's definitely a problem in the hash map code. 41400 5 darin 2006-05-06 11:59:11 -0700 (In reply to comment #4) > It's definitely a problem in the hash map code. It's wrong for HashMap::remove to call the value's destructor. Instead it needs to use the smart "RefCounter" template to do whatever extra work is needed because of differences between the value's destructor and the storage value's destructor. The code that's in there now is just totally wrong! I have a fix and I'll attach it shortly. What I'm not sure about is how to make a test for this. 41403 6 darin 2006-05-06 12:10:24 -0700 I'm confused now. I have a patch that I believe fixes the double destruction. But the incorrect text zooming still seems to be occuring. Mitz, I don't know how you made the connection between the bug and the problems in the form element code, so I'm not sure what else to try to diagnose this. For the time being, attaching my patch. 41404 7 8137 darin 2006-05-06 12:11:29 -0700 Created attachment 8137 patch based on Mitz's analysis, not sure if it fixes bug or what he observed 41405 8 mitz 2006-05-06 12:28:10 -0700 (In reply to comment #6) > Mitz, I don't know > how you made the connection between the bug and the problems in the form > element code, so I'm not sure what else to try to diagnose this. It was wrong of me to report the problems with the form element code under this bug, as I hadn't observed any connection. Not sure what to do now about the mess I made. Should I file a new bug? 41410 9 mitz 2006-05-06 13:57:58 -0700 This bug is a regression for r14032 (fix for bug 8562). 41418 10 darin 2006-05-06 16:40:39 -0700 So that means that the anomalies with the form element hash table are a separate issue. And they still need to be fixed. Maybe I should file a separate bug about those. 41419 11 darin 2006-05-06 16:41:08 -0700 (In reply to comment #8) > Should I file a new bug? Yes. 41425 12 mjs 2006-05-07 00:49:25 -0700 I can reproduce the crash reliably so I'll test if the patch works and file a new bug if needed. 41435 13 mitz 2006-05-07 04:47:20 -0700 (In reply to comment #11) > (In reply to comment #8) > > Should I file a new bug? > > Yes. > Maciej filed bug 8765. 44842 14 alice.barraclough 2006-06-06 07:38:21 -0700 <rdar://problem/4575086> 49770 15 darin 2006-07-14 06:04:19 -0700 This bug no longer happens with the test case here, because the text view is now an HTML view, and the cause of the bug had something to do with switching view types. That doesn't mean the bug has gone away, but it's almost certainly less common now and I no longer know how to reproduce it. Not a P1 any more. 896968 16 robert 2013-06-04 11:07:30 -0700 I don't think this bug is meaningful any more. Can it be closed? 896978 17 darin 2013-06-04 11:21:44 -0700 Not sure if this bug really exists any more. We’ll just close this. 8137 2006-05-06 12:11:29 -0700 2006-05-07 19:50:21 -0700 patch based on Mitz's analysis, not sure if it fixes bug or what he observed FormElementHashPatch.txt text/plain 2380 darin SW5kZXg6IFdlYkNvcmUvZG9tL0RvY3VtZW50LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2Rv bS9Eb2N1bWVudC5jcHAJKHJldmlzaW9uIDE0MjE2KQorKysgV2ViQ29yZS9kb20vRG9jdW1lbnQu Y3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zMDc4LDcgKzMwNzgsOCBAQAogCiBWZWN0b3I8U3RyaW5n PiBEb2N1bWVudDo6Zm9ybUVsZW1lbnRzU3RhdGUoKSBjb25zdAogewotICAgIFZlY3RvcjxTdHJp bmc+IHN0YXRlVmVjdG9yKG1fZm9ybUVsZW1lbnRzV2l0aFN0YXRlLnNpemUoKSAqIDMpOworICAg IFZlY3RvcjxTdHJpbmc+IHN0YXRlVmVjdG9yOworICAgIHN0YXRlVmVjdG9yLnJlc2VydmVDYXBh Y2l0eShtX2Zvcm1FbGVtZW50c1dpdGhTdGF0ZS5zaXplKCkgKiAzKTsKICAgICB0eXBlZGVmIEhh c2hTZXQ8SFRNTEdlbmVyaWNGb3JtRWxlbWVudCo+Ojpjb25zdF9pdGVyYXRvciBJdGVyYXRvcjsK ICAgICBJdGVyYXRvciBlbmQgPSBtX2Zvcm1FbGVtZW50c1dpdGhTdGF0ZS5lbmQoKTsKICAgICBm b3IgKEl0ZXJhdG9yIGl0ID0gbV9mb3JtRWxlbWVudHNXaXRoU3RhdGUuYmVnaW4oKTsgaXQgIT0g ZW5kOyArK2l0KSB7CkluZGV4OiBXZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJD b3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTQyMTYpCisrKyBXZWJDb3JlL0NoYW5nZUxvZwkod29y a2luZyBjb3B5KQpAQCAtMSwzICsxLDExIEBACisyMDA2LTA1LTA2ICBEYXJpbiBBZGxlciAgPGRh cmluQGFwcGxlLmNvbT4KKworICAgICAgICBTdWdnZXN0ZWQgYnkgTWl0ei4KKworICAgICAgICAq IGRvbS9Eb2N1bWVudC5jcHA6IChXZWJDb3JlOjpEb2N1bWVudDo6Zm9ybUVsZW1lbnRzU3RhdGUp OiBGaXhlZCBtaXN0YWtlIHdoZXJlIHRoZQorICAgICAgICB2ZWN0b3IgaGFzIGFuIGluaXRpYWwg c2l6ZSBhbmQgaW5zdGVhZCBzaG91bGQgaGF2ZSBhbiBpbml0aWFsIGNhcGFjaXR5LiBIYXJtbGVz cyBpbgorICAgICAgICBhIHdheSwgYnV0IGh1cnRzIHBlcmZvcm1hbmNlLgorCiAyMDA2LTA1LTA2 ICBEYXZpZCBIeWF0dCAgPGh5YXR0QGFwcGxlLmNvbT4KIAogICAgICAgICBSZW5hbWUgV2ViQ29y ZUZvbnQgdG8gRm9udFBsYXRmb3JtRGF0YSBhbmQgV2ViVGV4dFJlbmRlcmVyIHRvIEZvbnREYXRh LiAgTWVyZ2UgdGhlbSBpbnRvIHRoZQpJbmRleDogSmF2YVNjcmlwdENvcmUva3htbGNvcmUvSGFz aFNldC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIEphdmFTY3JpcHRDb3JlL2t4bWxjb3JlL0hhc2hTZXQuaAko cmV2aXNpb24gMTQyMTYpCisrKyBKYXZhU2NyaXB0Q29yZS9reG1sY29yZS9IYXNoU2V0LmgJKHdv cmtpbmcgY29weSkKQEAgLTI3NCw4ICsyNzQsNyBAQAogICAgIHsKICAgICAgICAgaWYgKGl0Lm1f aW1wbCA9PSBtX2ltcGwuZW5kKCkpCiAgICAgICAgICAgICByZXR1cm47Ci0gICAgICAgIC8vIFVz ZSAiKCppdCkuIiBpbnN0ZWFkIG9mICJpdC0+IiB0byB3b3JrIGFyb3VuZCBhIHByb2JsZW0gc2Vl biB3aXRoIHRoZSBWaXN1YWwgQysrIGNvbXBpbGVyLgotICAgICAgICAoKml0KS5+VmFsdWVUeXBl KCk7CisgICAgICAgIFJlZkNvdW50ZXI8VmFsdWVUcmFpdHMsIFN0b3JhZ2VUcmFpdHM+OjpkZXJl ZigqaXQubV9pbXBsKTsKICAgICAgICAgbV9pbXBsLnJlbW92ZShpdC5tX2ltcGwpOwogICAgIH0K IApJbmRleDogSmF2YVNjcmlwdENvcmUva3htbGNvcmUvSGFzaE1hcC5oCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IEphdmFTY3JpcHRDb3JlL2t4bWxjb3JlL0hhc2hNYXAuaAkocmV2aXNpb24gMTQyMTYpCisrKyBK YXZhU2NyaXB0Q29yZS9reG1sY29yZS9IYXNoTWFwLmgJKHdvcmtpbmcgY29weSkKQEAgLTI4OCw4 ICsyODgsNyBAQAogICAgIHsKICAgICAgICAgaWYgKGl0Lm1faW1wbCA9PSBtX2ltcGwuZW5kKCkp CiAgICAgICAgICAgICByZXR1cm47Ci0gICAgICAgIC8vIFVzZSAiKCppdCkuIiBpbnN0ZWFkIG9m ICJpdC0+IiB0byB3b3JrIGFyb3VuZCBhIHByb2JsZW0gc2VlbiB3aXRoIHRoZSBWaXN1YWwgQysr IGNvbXBpbGVyLgotICAgICAgICAoKml0KS5+VmFsdWVUeXBlKCk7CisgICAgICAgIFJlZkNvdW50 ZXI8VmFsdWVUcmFpdHMsIFZhbHVlU3RvcmFnZVRyYWl0cz46OmRlcmVmKCppdC5tX2ltcGwpOwog ICAgICAgICBtX2ltcGwucmVtb3ZlKGl0Lm1faW1wbCk7CiAgICAgfQogCg==