Bug 85266 – ASSERTION FAILED: m_purgePreventCount when clicking text with emphasis marks

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Koji Ishii]

Created attachment 139587 [details]
ASSERTION FAILED: m_purgePreventCount then crash in Font::emphasisMarkHeight

When text has emphasis marks, and the glyph for the emphasis mark does not exist in the font (i.e., system font fallback occurs,) ASSERTION FAILED: m_purgePreventCount. In my Debug builds of Mac OS Lion and Windows, Safari then crashes.

Reproduce steps:
1. Open the attached HTML file
2. Click on the text
-- OR --
1. Open the attached HTML file in DumpRenderTree

ASSERTION FAILED: m_purgePreventCount
/Users/kojiishi/WebKit/Source/WebCore/platform/graphics/FontCache.cpp(280) : WebCore::SimpleFontData *WebCore::FontCache::getCachedFontData(const WebCore::FontPlatformData *, WebCore::FontCache::ShouldRetain)
1   0x10cadd1ab WebCore::FontCache::getFontDataForCharacters(WebCore::Font const&, unsigned short const*, int)
2   0x10cae3875 WebCore::Font::glyphDataAndPageForCharacter(int, bool, WebCore::FontDataVariant) const
3   0x10cae27dc WebCore::Font::glyphDataForCharacter(int, bool, WebCore::FontDataVariant) const
4   0x10cae3f01 WebCore::Font::getEmphasisMarkGlyphData(WTF::AtomicString const&, WebCore::GlyphData&) const
5   0x10cae4156 WebCore::Font::emphasisMarkHeight(WTF::AtomicString const&) const
6   0x10cd435de WebCore::InlineFlowBox::computeOverAnnotationAdjustment(int) const
7   0x10d7ee58d WebCore::RootInlineBox::selectionTop() const
8   0x10d574434 WebCore::RenderBlock::positionForPointWithInlineChildren(WebCore::IntPoint const&)
9   0x10d574c41 WebCore::RenderBlock::positionForPoint(WebCore::IntPoint const&)
10  0x10ca5e133 WebCore::EventHandler::handleMousePressEventSingleClick(WebCore::MouseEventWithHitTestResults const&)
11  0x10ca5e878 WebCore::EventHandler::handleMousePressEvent(WebCore::MouseEventWithHitTestResults const&)
12  0x10ca62f73 WebCore::EventHandler::handleMousePressEvent(WebCore::PlatformMouseEvent const&)
13  0x10ca72262 WebCore::EventHandler::mouseDown(NSEvent*)
14  0x10bf45a97 -[WebHTMLView mouseDown:]
15  0x10b12777f -[EventSendingController mouseDown:withModifiers:]
16  0x7fff886a4f4c __invoking___
17  0x7fff886a4de4 -[NSInvocation invoke]
18  0x10d4b6ee0 JSC::Bindings::ObjcInstance::invokeObjcMethod(JSC::ExecState*, JSC::Bindings::ObjcMethod*)
19  0x10d4b6717 JSC::Bindings::ObjcInstance::invokeMethod(JSC::ExecState*, JSC::RuntimeMethod*)
20  0x10d7fd806 _ZN3JSCL17callRuntimeMethodEPNS_9ExecStateE
21  0x10b678b64 _ZN3JSC5LLIntL14handleHostCallEPNS_9ExecStateEPNS_11InstructionENS_7JSValueENS_22CodeSpecializationKindE
22  0x10b67979c JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*)
23  0x10b679721 JSC::LLInt::genericCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind)
24  0x10b67716c llint_slow_path_call
25  0x10b67dc54 llint_op_call
26  0x10b49d679 JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*)
27  0x10b4992c7 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*)
28  0x10b3a8a42 JSC::evaluate(JSC::ExecState*, JSC::ScopeChainNode*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*)
29  0x10d0cfe0d WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::ScopeChainNode*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*)
30  0x10d817543 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*)
31  0x10d817674 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&)

[Koji Ishii]

Created attachment 139590 [details]
Patch

[Koji Ishii]

I keep forgetting the fact that debug build crashes because ASSERT calls CRASH, and it will not crash release build, sorry about that.

Given that, and that purgeInactiveFontData() isn't on this code path as of now, I wonder this may not be important enough to fix.

The fix has a risk to introduce a new problem because ~FontCachePurgePreventer() may call purgeInactiveFontDataIfNeeded(), so if its callers are not preventing purging and assuming no one calls purgeInactiveFontData(), their data may be purged. I looked for source files and seems safe to me, but didn't look all ports.

Since I don't have good idea how we want to handle possible issues like this (ASSERT only, no actual harm,) will someone please advice? I'm fine with WONTFIX if the someone thinks so.

[Alexey Proskuryakov]

We do not want assertions to fire in debug builds. Either this assertion tells us that there is a bug (with symptoms in release builds) that needs to be fixed, or the assertion is incorrect, and needs to be corrected or removed.

[Koji Ishii]

(In reply to comment #3)
> We do not want assertions to fire in debug builds. Either this assertion tells us that there is a bug (with symptoms in release builds) that needs to be fixed, or the assertion is incorrect, and needs to be corrected or removed.

Thank you Alexey for the advice. I then think this fix is correct. The current code doesn't look to do anything harmful, but it has possible access violations if someone adds calls to purgeInactiveFontData(), or the way FontCache purges its cache changes.

[Brent Fulgham]

This change looks reasonable to me.  ap, was there any reason you had not r+'d it yet?  I'm inclined to approve it. We can always roll it out if something breaks.

[Alexey Proskuryakov]

Comment on attachment 139590 [details]
Patch

I think that Mitz should review this.

[Brent Fulgham]

(In reply to comment #6)
> (From update of attachment 139590 [details])
> I think that Mitz should review this.

Sounds good.

[Build Bot]

Comment on attachment 139590 [details]
Patch

Attachment 139590 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15912157

New failing tests:
fast/text/emphasis-height-crash.html

[Darin Adler]

Comment on attachment 139590 [details]
Patch

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

> Source/WebCore/platform/graphics/FontFastPath.cpp:286
> +    FontCachePurgePreventer fontCachePurge;

The name of the local variable should be preventer or purgePreventer, not "font cache purge".

[Darin Adler]

Comment on attachment 139590 [details]
Patch

Oops, Alexey wanted Mitz to review this. But I’m not sure this has to wait for that. This seems clearly right to me.

[Koji Ishii]

Created attachment 183665 [details]
Changed variable name to purgePreventer

[Build Bot]

Comment on attachment 183665 [details]
Changed variable name to purgePreventer

Attachment 183665 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15966552

New failing tests:
fast/text/emphasis-height-crash.html

[Build Bot]

Comment on attachment 183665 [details]
Changed variable name to purgePreventer

Attachment 183665 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15964584

New failing tests:
fast/text/emphasis-height-crash.html

[WebKit Review Bot]

Comment on attachment 183665 [details]
Changed variable name to purgePreventer

Rejecting attachment 183665 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'validate-changelog', '--non-interactive', 183665, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/17320598

[Ryosuke Niwa]

Comment on attachment 183665 [details]
Changed variable name to purgePreventer

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

This is not going to work. You need to replace NOBODY (OOPS!) with the reviewer's name (Darin Adler in this case).

Created attachment 195932 [details]
Patch

Comment on attachment 195932 [details]
Patch

Update ChangeLogs.

Created attachment 195936 [details]
Patch

Comment on attachment 195936 [details]
Patch

Add test and text expectation. I forgot to add them in previous patch.

[WebKit Review Bot]

Comment on attachment 195936 [details]
Patch

Clearing flags on attachment: 195936

Committed r147317: <http://trac.webkit.org/changeset/147317>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

Shouldn't the test checked in here uses testRunner? not layoutTestController?
It's failing here: http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r147317%20(34342)/results.html
and on some of the mac bots. Not quite sure why it isn't failing on all of them or why EWS didn't find it failing.

[Mark Lam]

(In reply to comment #22)
> Shouldn't the test checked in here uses testRunner? not layoutTestController?
> It's failing here: http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r147317%20(34342)/results.html
> and on some of the mac bots. Not quite sure why it isn't failing on all of them or why EWS didn't find it failing.

Yes, the test was bad. Fixed in r147372: <http://trac.webkit.org/changeset/147372>.