Bug 85266

Summary: ASSERTION FAILED: m_purgePreventCount when clicking text with emphasis marks
Product: WebKit Reporter: Koji Ishii <kojiishi>
Component: Layout and RenderingAssignee: yosin
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, buildbot, mark.lam, mitz, rniwa, roger_fong, webkit.review.bot, yosin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
ASSERTION FAILED: m_purgePreventCount then crash in Font::emphasisMarkHeight
none
Patch
none
Changed variable name to purgePreventer
none
Patch
none
Patch none

Description Koji Ishii 2012-04-30 23:23:45 PDT
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&)
Comment 1 Koji Ishii 2012-05-01 00:09:01 PDT
Created attachment 139590 [details]
Patch
Comment 2 Koji Ishii 2012-05-01 20:08:06 PDT
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.
Comment 3 Alexey Proskuryakov 2012-05-01 23:36:16 PDT
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.
Comment 4 Koji Ishii 2012-05-02 00:46:35 PDT
(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.
Comment 5 Brent Fulgham 2012-11-20 13:43:23 PST
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.
Comment 6 Alexey Proskuryakov 2012-11-20 16:18:49 PST
Comment on attachment 139590 [details]
Patch

I think that Mitz should review this.
Comment 7 Brent Fulgham 2012-11-20 16:19:40 PST
(In reply to comment #6)
> (From update of attachment 139590 [details])
> I think that Mitz should review this.

Sounds good.
Comment 8 Build Bot 2013-01-15 22:39:34 PST
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
Comment 9 Darin Adler 2013-01-16 15:17:47 PST
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".
Comment 10 Darin Adler 2013-01-16 15:18:29 PST
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.
Comment 11 Koji Ishii 2013-01-20 04:56:27 PST
Created attachment 183665 [details]
Changed variable name to purgePreventer
Comment 12 Build Bot 2013-01-20 05:22:53 PST
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
Comment 13 Build Bot 2013-01-20 06:17:12 PST
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
Comment 14 WebKit Review Bot 2013-03-31 23:35:08 PDT
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
Comment 15 Ryosuke Niwa 2013-03-31 23:35:15 PDT
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).
Comment 16 yosin 2013-04-01 00:19:19 PDT
Created attachment 195932 [details]
Patch
Comment 17 yosin 2013-04-01 00:19:56 PDT
Comment on attachment 195932 [details]
Patch

Update ChangeLogs.
Comment 18 yosin 2013-04-01 00:39:39 PDT
Created attachment 195936 [details]
Patch
Comment 19 yosin 2013-04-01 00:40:39 PDT
Comment on attachment 195936 [details]
Patch

Add test and text expectation. I forgot to add them in previous patch.
Comment 20 WebKit Review Bot 2013-04-01 03:34:02 PDT
Comment on attachment 195936 [details]
Patch

Clearing flags on attachment: 195936

Committed r147317: <http://trac.webkit.org/changeset/147317>
Comment 21 WebKit Review Bot 2013-04-01 03:34:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Roger Fong 2013-04-01 14:17:20 PDT
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.
Comment 23 Mark Lam 2013-04-01 17:10:01 PDT
(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>.