Bug 61556

I think you meant parentRenderObject->node() is null because of anonymous nodes :)

Right right

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.

Created attachment 96393 [details] Repro

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

Created attachment 96669 [details] A very short test case for this crash

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.

Created attachment 96691 [details] Revise patch style

Created attachment 96692 [details] Style-updated patch which actually applies (sorry!)

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

Created attachment 96757 [details] Revision of the last patch, with a crash test and revised ChangeLog entry

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

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

All reviewed patches have been landed. Closing bug.

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.

Created attachment 97336 [details] Follow up patch, fixing Darin's comments

Comment on attachment 97336 [details] Follow up patch, fixing Darin's comments LGTM

Reopening for Tim's follow up fix.

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

Looks like Niko's big SVG Fonts/GlyphPage rework obsoleted this.