Summary: | Crash on hover with certain styles on the text applied | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jorge Salvador Caffarena <jorge> | ||||||||||
Component: | JavaScriptCore | Assignee: | Justin Garcia <justin.garcia> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | alamaz, joost, mrowe | ||||||||||
Priority: | P1 | ||||||||||||
Version: | 412 | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Attachments: |
|
Description
Jorge Salvador Caffarena
2005-06-08 10:57:16 PDT
i can't reproduce this bug, not in Webkit 412 nor in the current Safari shipped with Tiger. Created attachment 2172 [details]
Test case. The crash occurs on mouse over of the link.
The attached test case causes 100% reproducable crash. Verified with ToT
WebKit.
*** Bug 3389 has been marked as a duplicate of this bug. *** K, can reproduce with this testcase. Changing it to javascript since the crash seems to be in there, and changed it to p1 since it's a reproducable crash. I've been looking into this, so I went ahead and assigned it to myself. Is that the proper Bugzilla etiquette? It is proper etiquette :) It's not yet fixed tho? why not? :P The crash occurs when a TextIterator returns a length 1 item with no characters. But the fault lies with Bidi, which constructs an InlineTextBox of length 1 for the node whose nodeValue was set to "" (TextIterators iterate over InlineTextBoxes) Created attachment 3014 [details]
Patch
In the test case, bidi adds a run of length 1 for an empty text node. It's
length 1 because it's at the end of a line, and bidi creates the last run in a
line using "appendRunsForObject(start, bidi.eor.pos+1, obj, bidi);"
But Bidi probably shouldn't even encounter zero length RenderObjects (from
empty text nodes) anyway, so one fix is to kill a text node's RenderObject if
its nodeValue is set to "".
This fix detaches a node if its nodeValue is set to "", and reattaches it if
nodeValue becomes non-empty. Any suggestions?
Comment on attachment 3014 [details]
Patch
I don't think this patch is quite right. When the document as a whole is
attached, individual nodes should not be detached, even if they do not need a
renderer. If a node changes to a state where it should no longer have a
renderer, then the right thing to do is to detach and reattach it, if it is
already attached. Then createRendererIfNeeded will do the right thing and make
a new renderer or not. Note that CharacterDataImpl::rendererIsNeeded will
already refuse to create a renderer if the string is empty.
Conversely, you can't just unconditionally attach a text node if its text is
getting changed to non-empty. If style hasn't been resolved yet, it wouldn't
have been attached in the first place.
And finally, note that the EditingTextImpl subclass of TextImpl can validly
have a renderer even if empty. This is the type of text node that gets inserted
in preparation for user typing in a space where there is no text already
present. So in addition to straightening out whether empty text nodes have a
renderer, you may also have to address the specific circumastances that cause a
crash in this case.
Created attachment 3103 [details]
New Patch
Maciej's right, it seems OK for zero length render objects to exist. This
patch just prevents runs associated with a zero length render object from being
larger than they should be.
Created attachment 3104 [details]
layout test for patch
You'll have to save and view the layout test in a text editor, since it crashes Safari without the patch applied. Comment on attachment 3103 [details]
New Patch
This looks like a good fix, r=me, but since zero-length text nromally doesn't
get a renderer, it independently seems like a good idea to maintain this
property dynamically. Maybe talk to hyatt about it.
Landing this ... |