Bug 8753 – REGRESSION (r14032): Incorrect and non-uniform text zoom applied after going forward and backward

• Component: Layout and Rendering
• Status: RESOLVED INVALID

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.

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.

(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.

[Darin Adler]

Aggh! I'll take care of this right away.

[Darin Adler]

(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.

[Darin Adler]

(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.

[Darin Adler]

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.

[Darin Adler]

Created attachment 8137 [details]
patch based on Mitz's analysis, not sure if it fixes bug or what he observed

(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?

This bug is a regression for r14032 (fix for bug 8562).

[Darin Adler]

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.

[Darin Adler]

(In reply to comment #8)
> Should I file a new bug?

Yes.

[Maciej Stachowiak]

I can reproduce the crash reliably so I'll test if the patch works and file a new bug if needed.

(In reply to comment #11)
> (In reply to comment #8)
> > Should I file a new bug?
> 
> Yes.
> 

Maciej filed bug 8765.

[Alice Liu]

<rdar://problem/4575086>

[Darin Adler]

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.

[Robert Hogan]

I don't think this bug is meaningful any more. Can it be closed?

[Darin Adler]

Not sure if this bug really exists any more. We’ll just close this.