Bug 160384 - Re-enable StringView life-cycle checking (CHECK_STRINGVIEW_LIFETIME).
Summary: Re-enable StringView life-cycle checking (CHECK_STRINGVIEW_LIFETIME).
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
: 162722 (view as bug list)
Depends on: 160584 162702 162722 162732 162743
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-30 14:18 PDT by Mark Lam
Modified: 2016-09-29 12:20 PDT (History)
13 users (show)

See Also:


Attachments
proposed patch. (1.16 KB, patch)
2016-07-30 14:23 PDT, Mark Lam
mark.lam: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-yosemite (491.22 KB, application/zip)
2016-07-30 15:20 PDT, Build Bot
no flags Details
Let's try the EWS again. (1.12 KB, patch)
2016-08-23 10:59 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
rebased. (1.24 KB, patch)
2016-08-23 14:21 PDT, Mark Lam
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (1.61 MB, application/zip)
2016-08-23 15:49 PDT, Build Bot
no flags Details
proposed patch. (1.19 KB, patch)
2016-09-28 14:37 PDT, Mark Lam
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-07-30 14:18:40 PDT
Let's flip the switch and test it on the EWS bots.
Comment 1 Mark Lam 2016-07-30 14:23:44 PDT
Created attachment 284955 [details]
proposed patch.
Comment 2 Build Bot 2016-07-30 15:20:16 PDT
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.
Comment 3 Build Bot 2016-07-30 15:20:21 PDT
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
Comment 4 Mark Lam 2016-07-30 15:23:46 PDT
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.
Comment 5 Darin Adler 2016-07-31 20:53:33 PDT
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.
Comment 6 Alexey Proskuryakov 2016-08-01 08:55:11 PDT
All crash logs that I spot checked asserted under ComplexTextController::collectComplexTextRuns().

An example of affected test is accessibility/content-editable-as-textarea.html.
Comment 7 Darin Adler 2016-08-01 10:58:02 PDT
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.
Comment 8 Alexey Proskuryakov 2016-08-01 12:48:16 PDT
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
Comment 9 Darin Adler 2016-08-01 17:24:27 PDT
OK, that indicates a bug in InlineTextBox::constructTextRun that can easily be fixed. It should be a small performance boost too.
Comment 10 Darin Adler 2016-08-02 09:06:46 PDT
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.
Comment 11 Myles C. Maxfield 2016-08-03 15:13:28 PDT
(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.
Comment 12 Myles C. Maxfield 2016-08-05 10:42:01 PDT
(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.
Comment 13 Mark Lam 2016-08-23 10:59:29 PDT
Created attachment 286736 [details]
Let's try the EWS again.
Comment 14 Mark Lam 2016-08-23 14:21:55 PDT
Created attachment 286774 [details]
rebased.
Comment 15 Build Bot 2016-08-23 15:49:51 PDT
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
Comment 16 Build Bot 2016-08-23 15:49:54 PDT
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
Comment 17 David Kilzer (:ddkilzer) 2016-09-26 12:13:16 PDT
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.
Comment 18 Radar WebKit Bug Importer 2016-09-26 12:13:37 PDT
<rdar://problem/28479434>
Comment 19 Mark Lam 2016-09-28 14:37:58 PDT
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.
Comment 20 Mark Lam 2016-09-28 17:01:30 PDT
Comment on attachment 290122 [details]
proposed patch.

EWS bots confirm that we're good to go.
Comment 21 Mark Lam 2016-09-28 17:06:04 PDT
Thanks for the review.  Landed in r206563: <http://trac.webkit.org/r206563>.
Comment 22 David Kilzer (:ddkilzer) 2016-09-28 18:49:18 PDT
(In reply to comment #21)
> Thanks for the review.  Landed in r206563: <http://trac.webkit.org/r206563>.

Nice work, Mark!
Comment 23 Csaba Osztrogonác 2016-09-29 03:30:49 PDT
(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.
Comment 24 WebKit Commit Bot 2016-09-29 10:04:28 PDT
Re-opened since this is blocked by bug 162732
Comment 25 Ryan Haddad 2016-09-29 10:12:01 PDT
*** Bug 162722 has been marked as a duplicate of this bug. ***
Comment 26 Mark Lam 2016-09-29 12:20:05 PDT
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>.