Bug 61556 - REGRESSION(87152): Crash on page with svg fonts
Summary: REGRESSION(87152): Crash on page with svg fonts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://www.upokecenter.com/dex/
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-26 13:52 PDT by James Robinson
Modified: 2011-07-01 16:13 PDT (History)
10 users (show)

See Also:


Attachments
Repro (303 bytes, image/svg+xml)
2011-06-08 00:58 PDT, Berend-Jan Wever
no flags Details
Preliminary patch (3.54 KB, patch)
2011-06-09 17:15 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
A very short test case for this crash (34.82 KB, application/zip)
2011-06-09 17:16 PDT, Tim Horton
no flags Details
Revise patch style (3.60 KB, patch)
2011-06-09 19:38 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Style-updated patch which actually applies (sorry!) (3.60 KB, patch)
2011-06-09 19:58 PDT, Tim Horton
krit: review-
Details | Formatted Diff | Diff
Revision of the last patch, with a crash test and revised ChangeLog entry (6.08 KB, patch)
2011-06-10 10:45 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Follow up patch, fixing Darin's comments (3.26 KB, patch)
2011-06-15 11:41 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.