Bug 160384 – Re-enable StringView life-cycle checking (CHECK_STRINGVIEW_LIFETIME).

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Mark Lam]

Let's flip the switch and test it on the EWS bots.

[Mark Lam]

Created attachment 284955 [details]
proposed patch.

[Build Bot]

Comment on attachment 284955 [details]
proposed patch.

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 284956 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Mark Lam]

Comment on attachment 284955 [details]
proposed patch.

We're definitely still getting assertion failures in many tests.  Will need to fix them all before we can re-enable the StringView life-cycle checks.

[Darin Adler]

Looking forward to learning details about tests that trigger assertion failures. If there are too many then I guess we can leave this alone and come back to it later. I’m hoping there are just a few mistakes either in the usage of StringView or in the checking that affect many tests.

[Alexey Proskuryakov]

All crash logs that I spot checked asserted under ComplexTextController::collectComplexTextRuns().

An example of affected test is accessibility/content-editable-as-textarea.html.

[Darin Adler]

Could you point me at one of those? Scanning the source code of ComplexTextController::collectComplexTextRuns, I can’t spot where StringView is used at all. Maybe I can try testing this myself at some point.

[Alexey Proskuryakov]

The StringView is used here:

#1	0x11d15db82 in WTF::StringView::characters8() const at OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/StringView.h:337
#2	0x11d92070c in WebCore::TextRun::characters8() const at OpenSource/Source/WebCore/platform/graphics/TextRun.h:82
#3	0x11d91021d in WebCore::ComplexTextController::collectComplexTextRuns() at OpenSource/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:327
#4	0x11d90f9e7 in WebCore::ComplexTextController::ComplexTextController(WebCore::FontCascade const&, WebCore::TextRun const&, bool, WTF::HashSet<WebCore::Font const*, WTF::PtrHash<WebCore::Font const*>, WTF::HashTraits<WebCore::Font const*> >*, bool) at OpenSource/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:144
#5	0x11d917074 in WebCore::ComplexTextController::ComplexTextController(WebCore::FontCascade const&, WebCore::TextRun const&, bool, WTF::HashSet<WebCore::Font const*, WTF::PtrHash<WebCore::Font const*>, WTF::HashTraits<WebCore::Font const*> >*, bool) at OpenSource/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:132
#6	0x11ec775b8 in WebCore::FontCascade::adjustSelectionRectForComplexText(WebCore::TextRun const&, WebCore::LayoutRect&, int, int) const at OpenSource/Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:532
#7	0x11ec443c1 in WebCore::FontCascade::adjustSelectionRectForText(WebCore::TextRun const&, WebCore::LayoutRect&, int, int) const at OpenSource/Source/WebCore/platform/graphics/FontCascade.cpp:506
#8	0x11fa55396 in WebCore::InlineTextBox::positionForOffset(int) const at OpenSource/Source/WebCore/rendering/InlineTextBox.cpp:1015
#9	0x1228c9089 in WebCore::RenderText::localCaretRect(WebCore::InlineBox*, int, WebCore::LayoutUnit*) at OpenSource/Source/WebCore/rendering/RenderText.cpp:451
#10	0x11d577e48 in WebCore::AXObjectCache::localCaretRectForCharacterOffset(WebCore::RenderObject*&, WebCore::CharacterOffset const&) at OpenSource/Source/WebCore/accessibility/AXObjectCache.cpp:2470
#11	0x11d578387 in WebCore::AXObjectCache::absoluteCaretBoundsForCharacterOffset(WebCore::CharacterOffset const&) at OpenSource/Source/WebCore/accessibility/AXObjectCache.cpp:2479
#12	0x11d18f7f5 in WebCore::AccessibilityRenderObject::boundsForRange(WTF::RefPtr<WebCore::Range>) const at OpenSource/Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2008

I think that it is invalidated here:

#0	0x1130fa8d2 in WTF::StringView::invalidate(WTF::StringImpl const&) at OpenSource/Source/WTF/wtf/text/StringView.cpp:213
#1	0x1130cb8c1 in WTF::StringImpl::~StringImpl() at OpenSource/Source/WTF/wtf/text/StringImpl.cpp:110
#2	0x1130cbba5 in WTF::StringImpl::~StringImpl() at OpenSource/Source/WTF/wtf/text/StringImpl.cpp:107
#3	0x1130cbbc5 in WTF::StringImpl::destroy(WTF::StringImpl*) at OpenSource/Source/WTF/wtf/text/StringImpl.cpp:138
#4	0x11d0c3a61 in WTF::StringImpl::deref() at OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/StringImpl.h:599
#5	0x11d0c39d1 in void WTF::derefIfNotNull<WTF::StringImpl>(WTF::StringImpl*) at OpenSource/WebKitBuild/Debug/usr/local/include/wtf/PassRefPtr.h:40
#6	0x11d0c392a in WTF::RefPtr<WTF::StringImpl>::~RefPtr() at OpenSource/WebKitBuild/Debug/usr/local/include/wtf/RefPtr.h:62
#7	0x11d0c3725 in WTF::RefPtr<WTF::StringImpl>::~RefPtr() at OpenSource/WebKitBuild/Debug/usr/local/include/wtf/RefPtr.h:62
#8	0x11d0c3705 in WTF::String::~String() at OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/WTFString.h:137
#9	0x11d0c36e5 in WTF::String::~String() at OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/WTFString.h:137
#10	0x11fa4270e in WebCore::InlineTextBox::constructTextRun(WebCore::RenderStyle const&, WTF::String*) const at OpenSource/Source/WebCore/rendering/InlineTextBox.cpp:1031
#11	0x11fa5536f in WebCore::InlineTextBox::positionForOffset(int) const at OpenSource/Source/WebCore/rendering/InlineTextBox.cpp:1014
#12	0x1228c9089 in WebCore::RenderText::localCaretRect(WebCore::InlineBox*, int, WebCore::LayoutUnit*) at OpenSource/Source/WebCore/rendering/RenderText.cpp:451
#13	0x11d577e48 in WebCore::AXObjectCache::localCaretRectForCharacterOffset(WebCore::RenderObject*&, WebCore::CharacterOffset const&) at OpenSource/Source/WebCore/accessibility/AXObjectCache.cpp:2470
#14	0x11d578387 in WebCore::AXObjectCache::absoluteCaretBoundsForCharacterOffset(WebCore::CharacterOffset const&) at OpenSource/Source/WebCore/accessibility/AXObjectCache.cpp:2479
#15	0x11d18f7f5 in WebCore::AccessibilityRenderObject::boundsForRange(WTF::RefPtr<WebCore::Range>) const at OpenSource/Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2008
#16	0x123d2c21b in ::-[WebAccessibilityObjectWrapper accessibilityAttributeValue:forParameter:](NSString *, id) at OpenSource/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4123

[Darin Adler]

OK, that indicates a bug in InlineTextBox::constructTextRun that can easily be fixed. It should be a small performance boost too.

[Darin Adler]

Myles and I talked about how to fix that InlineTextBox::constructTextRun issue and he’s planning to make a patch that does it in the next couple of days.

[Myles C. Maxfield]

(In reply to comment #10)
> Myles and I talked about how to fix that InlineTextBox::constructTextRun
> issue and he’s planning to make a patch that does it in the next couple of
> days.

Starting on this now.

[Myles C. Maxfield]

(In reply to comment #11)
> (In reply to comment #10)
> > Myles and I talked about how to fix that InlineTextBox::constructTextRun
> > issue and he’s planning to make a patch that does it in the next couple of
> > days.
> 
> Starting on this now.

I've been doing the work for this at https://bugs.webkit.org/show_bug.cgi?id=160584.

[Mark Lam]

Created attachment 286736 [details]
Let's try the EWS again.

[Mark Lam]

Created attachment 286774 [details]
rebased.

[Build Bot]

Comment on attachment 286774 [details]
rebased.

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

New failing tests:
js/regress/put-by-id-transition-with-indexing-header.html

[Build Bot]

Created attachment 286796 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[David Kilzer (:ddkilzer)]

Adding CHECK_STRINGVIEW_LIFETIME to the bug title and in a comment so anyone searching bugs.webkit.org for that string will be able to find this easily.

[Radar WebKit Bug Importer]

<rdar://problem/28479434>

[Mark Lam]

Created attachment 290122 [details]
proposed patch.

I think I've resolved the remaining issues that prevents us from re-enabling this.  Let's have the EWS confirm my test results.

[Mark Lam]

Comment on attachment 290122 [details]
proposed patch.

EWS bots confirm that we're good to go.

[Mark Lam]

Thanks for the review.  Landed in r206563: <http://trac.webkit.org/r206563>.

[David Kilzer (:ddkilzer)]

(In reply to comment #21)
> Thanks for the review.  Landed in r206563: <http://trac.webkit.org/r206563>.

Nice work, Mark!

[Csaba Osztrogonác]

(In reply to comment #21)
> Thanks for the review.  Landed in r206563: <http://trac.webkit.org/r206563>.

It made some tests timeout on debug bots, see bug162722 for details.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 162732

[Ryan Haddad]

*** Bug 162722 has been marked as a duplicate of this bug. ***

[Mark Lam]

I've sped up the slow tests in r206600: <http://trac.webkit.org/r206600>.

This patch is re-landed in r206601: <http://trac.webkit.org/r206601>.