Bug 28245 – findNextLineBreak crashes when inserting a blank span in front of a long line

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Patrick Geiller]

Created attachment 34714 [details]
Test case, will crash nightly when clicking the button

When using range.insertNode to add a SPAN in a DIV containing repeating non-breaking characters and a line break, Safari crashes.

[Patrick Geiller]

bug #18282 also crashes in findNextLineBreak, but with a different stack trace. 

Here's this bug's stack trace.

0   com.apple.WebCore             	0x0145bd75 WebCore::RenderBlock::findNextLineBreak(WebCore::BidiResolver<WebCore::InlineIterator, WebCore::BidiRun>&, bool, bool&, bool&, WebCore::EClear*) + 5445
1   com.apple.WebCore             	0x0145dce4 WebCore::RenderBlock::layoutInlineChildren(bool, int&, int&) + 2180
2   com.apple.WebCore             	0x01451e0a WebCore::RenderBlock::layoutBlock(bool) + 570
3   com.apple.WebCore             	0x01443778 WebCore::RenderBlock::layout() + 40
4   com.apple.WebCore             	0x01451352 WebCore::RenderBlock::layoutBlockChildren(bool, int&) + 914
5   com.apple.WebCore             	0x014522dc WebCore::RenderBlock::layoutBlock(bool) + 1804
6   com.apple.WebCore             	0x01443778 WebCore::RenderBlock::layout() + 40
7   com.apple.WebCore             	0x01451352 WebCore::RenderBlock::layoutBlockChildren(bool, int&) + 914
8   com.apple.WebCore             	0x014522dc WebCore::RenderBlock::layoutBlock(bool) + 1804
9   com.apple.WebCore             	0x01443778 WebCore::RenderBlock::layout() + 40
10  com.apple.WebCore             	0x01451352 WebCore::RenderBlock::layoutBlockChildren(bool, int&) + 914
11  com.apple.WebCore             	0x014522dc WebCore::RenderBlock::layoutBlock(bool) + 1804
12  com.apple.WebCore             	0x01443778 WebCore::RenderBlock::layout() + 40
13  com.apple.WebCore             	0x01451352 WebCore::RenderBlock::layoutBlockChildren(bool, int&) + 914
14  com.apple.WebCore             	0x014522dc WebCore::RenderBlock::layoutBlock(bool) + 1804
15  com.apple.WebCore             	0x01443778 WebCore::RenderBlock::layout() + 40
16  com.apple.WebCore             	0x014f8b6a WebCore::RenderView::layout() + 250

[Mike Moretti]

Created attachment 45394 [details]
Fix for bug

The bug was caused by a null/empty string not being checked (and the string getting accessed by a bad index).  I added a check for empty string and the crash no longer happens.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45394 [details] without any errors.

[Darin Adler]

Comment on attachment 45394 [details]
Fix for bug

Looks good. There are tabs in the test case, so we won't be able to land it.

> -                            if (lBreak.obj && shouldPreserveNewline(lBreak.obj) && lBreak.obj->isText() && !toRenderText(lBreak.obj)->isWordBreak() && toRenderText(lBreak.obj)->characters()[lBreak.pos] == '\n') {
> +                            if (lBreak.obj && shouldPreserveNewline(lBreak.obj) && lBreak.obj->isText() && toRenderText(lBreak.obj)->textLength() > 0 && !toRenderText(lBreak.obj)->isWordBreak() && toRenderText(lBreak.obj)->characters()[lBreak.pos] == '\n') {

Another way to write this is:

    (*toRenderText(lBreak.obj)->text())[0] == '\n'

Because subscripting with StringImpl does a range check. It's not super-elegant, but a little less wordy.

Normally in WebKit code we do not write "> 0" for cases like this. It would just be "textLength() &&".

> +        alert(div.innerHTML);

Typically we prefer to put the results of the test into an element rather than using alerts. This makes it smoother to run the test in the web browser.

You can set the textContent of some other element to the innerHTML of this element.

If you are doing the alert to trigger the crash, then there's a chance the layout will not be triggered and the won't happen inside DumpRenderTree, since alert may not force layout. Did you check that the test does indeed crash in DumpRenderTree without the patch? If it does not, you may want to do something else to trigger layout.

review- only because of the tabs -- the patch is otherwise OK, but please consider my comments for possible refinements too

[Mike Moretti]

Created attachment 45431 [details]
Bug fix patch - Removed tabs

Sorry about the tabs.  I removed them now.

As for the indexing, I didn't want to use indexing because "characters" actually returns null, not a valid string (I don't know what "text()" does yet without looking further).

I also wasn't sure if textLength() would ever return a -1; I am still not as up to speed on the webkit code as I would like yet.

I looked at 3 or 4 test cases to find one to use as an example but wasn't sure which was best, so I randomly picked one and it had alerts in it.  If you can point me to a better example that would be great!  I would rather write a proper test case next time.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45431 [details] without any errors.

[Darin Adler]

Comment on attachment 45431 [details]
Bug fix patch - Removed tabs

(In reply to comment #5)
> As for the indexing, I didn't want to use indexing because "characters"
> actually returns null, not a valid string (I don't know what "text()" does yet
> without looking further).

I know that you didn't know that!

But text() returns a StringImpl* that does indeed handle the [0] case, as I said.

> I also wasn't sure if textLength() would ever return a -1

It would not.

It's OK not to know these things when you first wrote the patch. You can learn them during patch review.

What's not as OK is to just leave the patch as-is without responding to the comments. I can understand you not changing if you don't agree, but just saying "I didn't know when I originally wrote this" doesn't justify leaving the patch as is.

> I looked at 3 or 4 test cases to find one to use as an example but wasn't sure
> which was best, so I randomly picked one and it had alerts in it.  If you can
> point me to a better example that would be great!

Your attitude is fine, but your difficulty finding an example without alerts is strange. I started looking in the fast/dom directory alphabetically right where your new test would go and found:

    inner-text.html
    inner-width-height.html

I can provide many other examples that don't use alert.

> I would rather write a proper test case next time.

Please do it this time, lets not wait for next time.

[Mike Moretti]

Created attachment 45466 [details]
Updated fix - removed alerts in test case, minor recommended code change

Sorry for any miscommunication/misunderstanding that may have occurred.  I was working under the assumption that the sole reason the patch was rejected was for the tabs in the test case.  I had considered your suggestions as you requested, but did not know they were required to commit the fix.  Considering that the bug was actually fixed, and the review comment was that it was an OK fix, I chose to move on to fix another bug rather than spend an extra day or so recoding the fix and testing it again, but keeping your comments in mind for future fixes.

After the current review rejection, I went back into the code and tried to implement the text()[] recommendation.  However, unfortunately, it does'nt compile, giving an error about a non-existent == operator for ...->text()[lBreak.pos].  I am not sure I can implement this recommendation without spending much more time trying to research it.

As for the test case without alerts, as was mentioned previously, there were at least 3 completely different test cases I had originally looked at and was not sure which to use to base my test on.  After your suggestion, rather than going and randomly picking another test case that had no alerts but may still not be a good base, I was hoping you could point me to one that was a good example to use for all my future test cases so it would save review time later.

I've updated the test case to remove alerts as per your recommendation, and have removed the > 0.  However, I have not implemented the recommended text()[] fix as I do not currently have the knowledge to get it working.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45466 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 45466 [details]
Updated fix - removed alerts in test case, minor recommended code change

Clearing flags on attachment: 45466

Committed r52607: <http://trac.webkit.org/changeset/52607>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.