Bug 61556

Summary: REGRESSION(87152): Crash on page with svg fonts
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, hyatt, inferno, koivisto, mdelaney7, rwlbuis, simon.fraser, thorton, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.upokecenter.com/dex/
Attachments:
Description Flags
Repro
none
Preliminary patch
none
A very short test case for this crash
none
Revise patch style
none
Style-updated patch which actually applies (sorry!)
krit: review-
Revision of the last patch, with a crash test and revised ChangeLog entry
none
Follow up patch, fixing Darin's comments none

Description James Robinson 2011-05-26 13:52:59 PDT
The URL in the bug crashes after r87152 with a null pointer deref.  The crash is here (http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp?rev=87152#L268):

RenderObject* parentRenderObject = firstParentRendererForNonTextNode(renderObject); 
	
String language = toElement(parentRenderObject->node())->getAttribute(XMLNames::langAttr);

parentRenderObject is null.  I'm pretty sure that the definition of firstParentRendererForNonTextNode() is wrong.
Comment 1 Abhishek Arya 2011-05-26 13:54:54 PDT
I think you meant parentRenderObject->node() is null because of anonymous nodes :)
Comment 2 James Robinson 2011-05-26 13:56:24 PDT
Right right
Comment 3 Rob Buis 2011-05-31 09:19:39 PDT
This happens on OS X but not QtWebKit. However maybe the latter has something not implemented that causes the crash. Anyway, Niko said he is close to landing improved SVG Fonts support, so maybe we should check after that work.
Cheers,

Rob.
Comment 4 Berend-Jan Wever 2011-06-08 00:58:38 PDT
Created attachment 96393 [details]
Repro
Comment 5 Tim Horton 2011-06-09 17:15:07 PDT
Created attachment 96667 [details]
Preliminary patch

We were assuming that the parent of a SVG-font-styled block had an actual node, but it could have an anonymous node.

This fixes both the URL given in this bug's description, and my test case (which I'll post momentarily), but does not fix 96393: Repro (it gets past that crash, and to something else entirely).
Comment 6 Tim Horton 2011-06-09 17:16:53 PDT
Created attachment 96669 [details]
A very short test case for this crash
Comment 7 WebKit Review Bot 2011-06-09 17:19:06 PDT
Attachment 96667 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:271:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:353:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Tim Horton 2011-06-09 19:38:57 PDT
Created attachment 96691 [details]
Revise patch style
Comment 9 Tim Horton 2011-06-09 19:58:48 PDT
Created attachment 96692 [details]
Style-updated patch which actually applies (sorry!)
Comment 10 Dirk Schulze 2011-06-10 00:05:25 PDT
Comment on attachment 96692 [details]
Style-updated patch which actually applies (sorry!)

View in context: https://bugs.webkit.org/attachment.cgi?id=96692&action=review

This definitely needs a crash and regression test (both can be one test in this case).

> Source/WebCore/ChangeLog:9
> +        We can't assume that the parent of a SVG-font-styled
> +        text node won't be an anonymous block.
> +        http://bugs.webkit.org/show_bug.cgi?id=61556
> +
> +        No new tests. (OOPS!)

The style is wrong:

bugtitle
bugURL

text

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:-80
> -    ASSERT(newRenderer->node());
> -    ASSERT(newRenderer->node()->isElementNode());

Why did you remove these asserts?

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:270
> +        String language;

What happens if language is empty? Seems that we assumed that we have lang set.
Comment 11 Tim Horton 2011-06-10 09:50:26 PDT
Comment on attachment 96692 [details]
Style-updated patch which actually applies (sorry!)

View in context: https://bugs.webkit.org/attachment.cgi?id=96692&action=review

I'll fix the one comment here, add a test, and resubmit.

>> Source/WebCore/ChangeLog:9
>> +        No new tests. (OOPS!)
> 
> The style is wrong:
> 
> bugtitle
> bugURL
> 
> text

Ok, Will Fix.

>> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:-80
>> -    ASSERT(newRenderer->node()->isElementNode());
> 
> Why did you remove these asserts?

These asserts assume that the parent of a text node is never anonymous, which is incorrect. These were added in r87152, and are the root cause of this bug.

>> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:270
>> +        String language;
> 
> What happens if language is empty? Seems that we assumed that we have lang set.

The only function outside of this file that language is passed to is isCompatibleGlyph, which correctly handles the empty language case.
Comment 12 Tim Horton 2011-06-10 10:45:21 PDT
Created attachment 96757 [details]
Revision of the last patch, with a crash test and revised ChangeLog entry
Comment 13 Tim Horton 2011-06-10 12:40:54 PDT
The remaining issue (once it gets past this crash) with attachment #96393 [details] is equivalent to https://bugs.webkit.org/show_bug.cgi?id=60237
Comment 14 WebKit Review Bot 2011-06-13 10:58:10 PDT
The commit-queue encountered the following flaky tests while processing attachment 96757 [details]:

fast/dom/NodeList/5725058-crash-scenario-1.html bug 62577 (authors: mrowe@apple.com and sam@webkit.org)
The commit-queue is continuing to process your patch.
Comment 15 WebKit Review Bot 2011-06-13 10:59:39 PDT
Comment on attachment 96757 [details]
Revision of the last patch, with a crash test and revised ChangeLog entry

Clearing flags on attachment: 96757

Committed r88652: <http://trac.webkit.org/changeset/88652>
Comment 16 WebKit Review Bot 2011-06-13 10:59:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Darin Adler 2011-06-13 11:02:30 PDT
Comment on attachment 96757 [details]
Revision of the last patch, with a crash test and revised ChangeLog entry

View in context: https://bugs.webkit.org/attachment.cgi?id=96757&action=review

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:272
> +        String language;
> +        if (SVGElement* element = static_cast<SVGElement*>(parentRenderObject->node()))
> +            language = element->getAttribute(XMLNames::langAttr);

Attribute values are of type AtomicString. Normally it would be best to use that type for the local variable containing the attribute value too; depending on how the value is actually used, you could save a bit of work knowing at compile time that it was already uniqued.

Seems unnecessarily risky to cast to SVGElement here where all we want to do is getAttribute. Could be a cast to Element* instead. But I suppose this is OK if there is an ironclad guarantee this will be an SVG element.

Also, this code can use fastGetAttribute.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:354
> +        String language;
> +        if (SVGElement* element = static_cast<SVGElement*>(parentRenderObject->node()))
> +            language = element->getAttribute(XMLNames::langAttr);

Same comments.
Comment 18 Tim Horton 2011-06-15 11:41:40 PDT
Created attachment 97336 [details]
Follow up patch, fixing Darin's comments
Comment 19 Rob Buis 2011-06-15 12:05:52 PDT
Comment on attachment 97336 [details]
Follow up patch, fixing Darin's comments

LGTM
Comment 20 Adele Peterson 2011-07-01 15:59:03 PDT
Reopening for Tim's follow up fix.
Comment 21 WebKit Review Bot 2011-07-01 16:01:47 PDT
Comment on attachment 97336 [details]
Follow up patch, fixing Darin's comments

Rejecting attachment 97336 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
 u'--force']" exit_code: 1

Parsed 2 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp
Hunk #1 FAILED at 267.
Hunk #2 FAILED at 349.
2 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Rob Buis', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8976178
Comment 22 Tim Horton 2011-07-01 16:13:40 PDT
Looks like Niko's big SVG Fonts/GlyphPage rework obsoleted this.