Bug 137202 – Make StringView check the lifetime of the StringImpl it's created from

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

[Darin Adler]

Make StringView check the lifetime of the StringImpl it's created from

[Darin Adler]

Created attachment 238929 [details]
Patch

[Darin Adler]

Comment on attachment 238929 [details]
Patch

This initial version isn’t quite right; I am getting a crash on launch that I have to fix.

[Anders Carlsson]

Comment on attachment 238929 [details]
Patch

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

> Source/WTF/wtf/text/StringView.cpp:53
> +static HashMap<const StringImpl*, StringView::UnderlyingString*>& underlyingStrings()
> +{
> +    static NeverDestroyed<HashMap<const StringImpl*, StringView::UnderlyingString*>> map;
> +    return map;
> +}

I think access to and initialization of this data structure needs to be thread safe.

[Darin Adler]

Comment on attachment 238929 [details]
Patch

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

>> Source/WTF/wtf/text/StringView.cpp:53
>> +}
> 
> I think access to and initialization of this data structure needs to be thread safe.

Yes, I have that mostly done in my local copy of the patch.

[Darin Adler]

Created attachment 239262 [details]
Patch

[Darin Adler]

Created attachment 239264 [details]
Patch

[Darin Adler]

Anders, you asked me to do this, so would you review?

[Anders Carlsson]

Comment on attachment 239264 [details]
Patch

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

> Source/WTF/wtf/text/StringView.cpp:41
> +    std::atomic_uint refCount { 1 };

I think this should be std::atomic<unsigned> instead.

> Source/WTF/wtf/text/StringView.cpp:56
> +static std::mutex& underlyingStringsMutex()
> +{
> +    static NeverDestroyed<std::mutex> mutex;
> +    return mutex;
> +}

This mutex initialization still isn't thread safe. You need to do something like:

    static std::once_flag onceFlag;
    static LazyNeverDestroyed<std::mutex> mutex;

    std::call_once(onceFlag, []{
        mutex.construct();
    });

    return mutex;

> Source/WTF/wtf/text/StringView.cpp:107
> +        auto result = underlyingStrings().add(string, nullptr);
> +        if (result.isNewEntry)
> +            result.iterator->value = new UnderlyingString(*string);
> +        else
> +            ++result.iterator->value->refCount;

I'd write this as

auto& underlyingStringSlot = underlyingStrings().add(string, nullptr).iterator->value;
if (!underlyingStringSlot)
    underlyingStringSlot = new UnderlyingString(*string);
else
    ++underlyingString->refCount;
underlyingStringSlot = underlyingStringSlot;

> Source/WTF/wtf/text/StringView.h:142
> +    // FIXME: It's peculiar that null strings are 16-bit and empty strings return 8-bit (according to the is8Bit function).

I agree - we should make them 8-bit strings as well.

> Source/WTF/wtf/text/StringView.h:154
> +    ASSERT(other.underlyingStringIsValid());

Should probably ASSERT that this != &other here.

> Source/WTF/wtf/text/StringView.h:174
> +    ASSERT(other.underlyingStringIsValid());

Should probably ASSERT(this != &other) here since that's already broken.

[Darin Adler]

Comment on attachment 239264 [details]
Patch

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

>> Source/WTF/wtf/text/StringView.cpp:107
>> +            ++result.iterator->value->refCount;
> 
> I'd write this as
> 
> auto& underlyingStringSlot = underlyingStrings().add(string, nullptr).iterator->value;
> if (!underlyingStringSlot)
>     underlyingStringSlot = new UnderlyingString(*string);
> else
>     ++underlyingString->refCount;
> underlyingStringSlot = underlyingStringSlot;

The one downside of that version is that there is no iterator lifetime checking on the reference. So if the UnderlyingString constructor, for example, modified the hash table, we would not get a runtime assertion. But I do like it better, and often wrote it that way before, so I’ll go with it.

[Darin Adler]

Created attachment 239398 [details]
Patch

[Darin Adler]

Comment on attachment 239398 [details]
Patch

Damn, I meant to land this, not upload it.

[Darin Adler]

Committed r174388: <http://trac.webkit.org/changeset/174388>

[Carlos Alberto Lopez Perez]

(In reply to comment #12)
> Committed r174388: <http://trac.webkit.org/changeset/174388>

This has caused assertions on a number of layout tests on the Debug bots: https://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK2%20%28Tests%29/builds/19510

03:01:14.945 93722 worker/2 animations/change-keyframes-name.html crashed, (stderr lines):
03:01:14.945 93722   ASSERTION FAILED: underlyingStringIsValid()
03:01:14.945 93722   /Volumes/Data/slave/mountainlion-debug/build/WebKitBuild/Debug/usr/local/include/wtf/text/StringView.h(260) : const LChar *WTF::StringView::characters8() const
03:01:14.945 93722   1   0x10ee56270 WTFCrash
03:01:14.945 93722   2   0x1105c80df WTF::StringView::characters8() const
03:01:14.945 93722   3   0x110eed058 WebCore::TextRun::data8(unsigned int) const
03:01:14.945 93722   4   0x11244395f WebCore::WidthIterator::advance(int, WebCore::GlyphBuffer*)
03:01:14.945 93722   5   0x110d93357 WebCore::Font::getGlyphsAndAdvancesForSimpleText(WebCore::TextRun const&, int, int, WebCore::GlyphBuffer&, WebCore::Font::ForTextEmphasisOrNot) const
03:01:14.945 93722   6   0x110d93501 WebCore::Font::drawSimpleText(WebCore::GraphicsContext*, WebCore::TextRun const&, WebCore::FloatPoint const&, int, int) const
03:01:14.945 93722   7   0x110d6d341 WebCore::Font::drawText(WebCore::GraphicsContext*, WebCore::TextRun const&, WebCore::FloatPoint const&, int, int, WebCore::Font::CustomFontNotReadyAction) const
03:01:14.945 93722   8   0x110ee4d7e WebCore::GraphicsContext::drawText(WebCore::Font const&, WebCore::TextRun const&, WebCore::FloatPoint const&, int, int)
03:01:14.945 93722   9   0x112039b0f WebCore::SimpleLineLayout::paintFlow(WebCore::RenderBlockFlow const&, WebCore::SimpleLineLayout::Layout const&, WebCore::PaintInfo&, WebCore::LayoutPoint const&)
03:01:14.945 93722   10  0x111baf5a8 WebCore::RenderBlockFlow::paintInlineChildren(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
03:01:14.945 93722   11  0x111b6973c WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
03:01:14.945 93722   12  0x111b6a40e WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
03:01:14.946 93722   13  0x111b6957c WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
03:01:14.946 93722   14  0x111b69c80 WebCore::RenderBlock::paintChild(WebCore::RenderBox&, WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool)
03:01:14.946 93722   15  0x111b698b7 WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool)
03:01:14.946 93722   16  0x111b69838 WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
03:01:14.946 93722   17  0x111b6a40e WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
03:01:14.946 93722   18  0x111b6957c WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
03:01:14.946 93722   19  0x111b69c80 WebCore::RenderBlock::paintChild(WebCore::RenderBox&, WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool)
03:01:14.946 93722   20  0x111b698b7 WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool)
03:01:14.946 93722   21  0x111b69838 WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
03:01:14.946 93722   22  0x111b6a40e WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
03:01:14.946 93722   23  0x111b6957c WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
03:01:14.946 93722   24  0x111cbcbae WebCore::RenderLayer::paintForegroundForFragmentsWithPhase(WebCore::PaintPhase, WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow> const&, WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int, WebCore::RenderObject*)
03:01:14.946 93722   25  0x111cba735 WebCore::RenderLayer::paintForegroundForFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow> const&, WebCore::GraphicsContext*, WebCore::GraphicsContext*, WebCore::LayoutRect const&, bool, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int, WebCore::RenderObject*, bool)
03:01:14.947 93722   26  0x111cb69b2 WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
03:01:14.947 93722   27  0x111cb601a WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
03:01:14.947 93722   28  0x111cb4d43 WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
03:01:14.947 93722   29  0x111cba423 WebCore::RenderLayer::paintList(WTF::Vector<WebCore::RenderLayer*, 0ul, WTF::CrashOnOverflow>*, WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
03:01:14.947 93722   30  0x111cb6a8b WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
03:01:14.947 93722   31  0x111ce5361 WebCore::RenderLayerBacking::paintIntoLayer(WebCore::GraphicsLayer const*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, unsigned int)
03:01:14.955 93688 [1039/34451] animations/change-keyframes-name.html failed unexpectedly (WebProcess crashed [pid=93900])

[Darin Adler]

(In reply to comment #13)
> This has caused assertions on a number of layout tests on the Debug bots:

We’re turning the checking off until we can resolve this; also, that looks like we uncovered a real bug! I didn’t see that locally when I ran the tests, not sure why.