Make StringView check the lifetime of the StringImpl it's created from
Created attachment 238929 [details] Patch
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.
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.
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.
Created attachment 239262 [details] Patch
Created attachment 239264 [details] Patch
Anders, you asked me to do this, so would you review?
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.
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.
Created attachment 239398 [details] Patch
Comment on attachment 239398 [details] Patch Damn, I meant to land this, not upload it.
Committed r174388: <http://trac.webkit.org/changeset/174388>
(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])
(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.