Log Message
StringView should have an explicit m_is8Bit field. https://bugs.webkit.org/show_bug.cgi?id=160282 <rdar://problem/27327943>
Reviewed by Benjamin Poulain. Source/_javascript_Core: * tests/stress/string-joining-long-strings-should-not-crash.js: Added. (catch): Source/WTF: The current implementation reserves 1 bit in the 32-bit m_length field as an is16Bit flag. As a result, a StringView is incapable of handling strings that have a length of 32-bit in size. This results in a mismatch with the expectations of String, StringImpl, and _javascript_Core's JSString which all support a 32-bit unsigned length. This patch fixes this issue by introducing an explicit m_is8Bit field, thereby allowing m_length to be a full 32-bit again. We also introduced a clear() convenience method to set the fields of StringView to empty values. Previously, we were duplicating the code for clearing those fields. We now call clear() in all those places instead. Note: in clear(), we set m_is8Bit to true because we want an empty StringView to be 8-bit rather than 16-bit. This is consistent with what the empty() method returns. * wtf/text/StringView.h: (WTF::StringView::setUnderlyingString): (WTF::StringView::StringView): (WTF::StringView::operator=): (WTF::StringView::initialize): (WTF::StringView::clear): (WTF::StringView::empty): (WTF::StringView::length): (WTF::StringView::operator bool): (WTF::StringView::is8Bit): (WTF::StringView::substring): (WTF::StringView::getCharactersWithUpconvert): (WTF::StringView::toString): (WTF::StringView::toAtomicString): (WTF::StringView::toFloat): (WTF::StringView::toInt): (WTF::StringView::toIntStrict): (WTF::StringView::toStringWithoutCopying): (WTF::StringView::find):
Modified Paths
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/WTF/ChangeLog
- trunk/Source/WTF/wtf/text/StringView.h
Added Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (203833 => 203834)
--- trunk/Source/_javascript_Core/ChangeLog 2016-07-28 20:21:33 UTC (rev 203833)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-07-28 20:28:47 UTC (rev 203834)
@@ -1,3 +1,14 @@
+2016-07-28 Mark Lam <mark....@apple.com>
+
+ StringView should have an explicit m_is8Bit field.
+ https://bugs.webkit.org/show_bug.cgi?id=160282
+ <rdar://problem/27327943>
+
+ Reviewed by Benjamin Poulain.
+
+ * tests/stress/string-joining-long-strings-should-not-crash.js: Added.
+ (catch):
+
2016-07-28 Csaba Osztrogonác <o...@webkit.org>
[ARM] Typo fix after r121885
Added: trunk/Source/_javascript_Core/tests/stress/string-joining-long-strings-should-not-crash.js (0 => 203834)
--- trunk/Source/_javascript_Core/tests/stress/string-joining-long-strings-should-not-crash.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/string-joining-long-strings-should-not-crash.js 2016-07-28 20:28:47 UTC (rev 203834)
@@ -0,0 +1,15 @@
+//@ runDefault
+// This test should not crash.
+
+var error;
+let str = '';
+let arr = [{}, 2, 3];
+try {
+ for (let z = 0; z < 30; z++)
+ str = arr.join(str); // exponentially grow length of string.
+} catch(e) {
+ error = e;
+}
+
+if (!error)
+ throw Error("Failed");
Modified: trunk/Source/WTF/ChangeLog (203833 => 203834)
--- trunk/Source/WTF/ChangeLog 2016-07-28 20:21:33 UTC (rev 203833)
+++ trunk/Source/WTF/ChangeLog 2016-07-28 20:28:47 UTC (rev 203834)
@@ -1,3 +1,48 @@
+2016-07-28 Mark Lam <mark....@apple.com>
+
+ StringView should have an explicit m_is8Bit field.
+ https://bugs.webkit.org/show_bug.cgi?id=160282
+ <rdar://problem/27327943>
+
+ Reviewed by Benjamin Poulain.
+
+ The current implementation reserves 1 bit in the 32-bit m_length field as an
+ is16Bit flag. As a result, a StringView is incapable of handling strings that
+ have a length of 32-bit in size. This results in a mismatch with the
+ expectations of String, StringImpl, and _javascript_Core's JSString which all
+ support a 32-bit unsigned length.
+
+ This patch fixes this issue by introducing an explicit m_is8Bit field, thereby
+ allowing m_length to be a full 32-bit again.
+
+ We also introduced a clear() convenience method to set the fields of StringView
+ to empty values. Previously, we were duplicating the code for clearing those
+ fields. We now call clear() in all those places instead.
+
+ Note: in clear(), we set m_is8Bit to true because we want an empty StringView
+ to be 8-bit rather than 16-bit. This is consistent with what the empty() method
+ returns.
+
+ * wtf/text/StringView.h:
+ (WTF::StringView::setUnderlyingString):
+ (WTF::StringView::StringView):
+ (WTF::StringView::operator=):
+ (WTF::StringView::initialize):
+ (WTF::StringView::clear):
+ (WTF::StringView::empty):
+ (WTF::StringView::length):
+ (WTF::StringView::operator bool):
+ (WTF::StringView::is8Bit):
+ (WTF::StringView::substring):
+ (WTF::StringView::getCharactersWithUpconvert):
+ (WTF::StringView::toString):
+ (WTF::StringView::toAtomicString):
+ (WTF::StringView::toFloat):
+ (WTF::StringView::toInt):
+ (WTF::StringView::toIntStrict):
+ (WTF::StringView::toStringWithoutCopying):
+ (WTF::StringView::find):
+
2016-07-24 Filip Pizlo <fpi...@apple.com>
B3 should support multiple entrypoints
Modified: trunk/Source/WTF/wtf/text/StringView.h (203833 => 203834)
--- trunk/Source/WTF/wtf/text/StringView.h 2016-07-28 20:21:33 UTC (rev 203833)
+++ trunk/Source/WTF/wtf/text/StringView.h 2016-07-28 20:28:47 UTC (rev 203834)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2014-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2016 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -156,11 +156,11 @@
void setUnderlyingString(const StringImpl*) { }
void setUnderlyingString(const StringView&) { }
#endif
+ void clear();
- static const unsigned is16BitStringFlag = 1u << 31;
-
const void* m_characters { nullptr };
unsigned m_length { 0 };
+ bool m_is8Bit { true };
#if CHECK_STRINGVIEW_LIFETIME
void adoptUnderlyingString(UnderlyingString*);
@@ -210,11 +210,11 @@
inline StringView::StringView(StringView&& other)
: m_characters(other.m_characters)
, m_length(other.m_length)
+ , m_is8Bit(other.m_is8Bit)
{
ASSERT(other.underlyingStringIsValid());
- other.m_characters = nullptr;
- other.m_length = 0;
+ other.clear();
setUnderlyingString(other);
other.setUnderlyingString(nullptr);
@@ -223,6 +223,7 @@
inline StringView::StringView(const StringView& other)
: m_characters(other.m_characters)
, m_length(other.m_length)
+ , m_is8Bit(other.m_is8Bit)
{
ASSERT(other.underlyingStringIsValid());
@@ -235,9 +236,9 @@
m_characters = other.m_characters;
m_length = other.m_length;
+ m_is8Bit = other.m_is8Bit;
- other.m_characters = nullptr;
- other.m_length = 0;
+ other.clear();
setUnderlyingString(other);
other.setUnderlyingString(nullptr);
@@ -251,6 +252,7 @@
m_characters = other.m_characters;
m_length = other.m_length;
+ m_is8Bit = other.m_is8Bit;
setUnderlyingString(other);
@@ -259,20 +261,16 @@
inline void StringView::initialize(const LChar* characters, unsigned length)
{
- // FIXME: We need a better solution here, because there is no guarantee that
- // the length here won't be too long. Maybe at least a RELEASE_ASSERT?
- ASSERT(!(length & is16BitStringFlag));
m_characters = characters;
m_length = length;
+ m_is8Bit = true;
}
inline void StringView::initialize(const UChar* characters, unsigned length)
{
- // FIXME: We need a better solution here, because there is no guarantee that
- // the length here won't be too long. Maybe at least a RELEASE_ASSERT?
- ASSERT(!(length & is16BitStringFlag));
m_characters = characters;
- m_length = is16BitStringFlag | length;
+ m_length = length;
+ m_is8Bit = false;
}
inline StringView::StringView(const LChar* characters, unsigned length)
@@ -310,8 +308,7 @@
{
setUnderlyingString(string.impl());
if (!string.impl()) {
- m_characters = nullptr;
- m_length = 0;
+ clear();
return;
}
if (string.is8Bit()) {
@@ -321,6 +318,13 @@
initialize(string.characters16(), string.length());
}
+inline void StringView::clear()
+{
+ m_characters = nullptr;
+ m_length = 0;
+ m_is8Bit = true;
+}
+
inline StringView StringView::empty()
{
return StringView(reinterpret_cast<const LChar*>(""), 0);
@@ -367,7 +371,7 @@
inline unsigned StringView::length() const
{
- return m_length & ~is16BitStringFlag;
+ return m_length;
}
inline StringView::operator bool() const
@@ -377,7 +381,7 @@
inline bool StringView::is8Bit() const
{
- return !(m_length & is16BitStringFlag);
+ return m_is8Bit;
}
inline StringView StringView::substring(unsigned start, unsigned length) const
@@ -432,8 +436,7 @@
return;
}
auto characters16 = this->characters16();
- unsigned length = this->length();
- for (unsigned i = 0; i < length; ++i)
+ for (unsigned i = 0; i < m_length; ++i)
destination[i] = characters16[i];
}
@@ -455,7 +458,7 @@
{
if (is8Bit())
return String(characters8(), m_length);
- return String(characters16(), length());
+ return String(characters16(), m_length);
}
inline AtomicString StringView::toAtomicString() const
@@ -462,7 +465,7 @@
{
if (is8Bit())
return AtomicString(characters8(), m_length);
- return AtomicString(characters16(), length());
+ return AtomicString(characters16(), m_length);
}
inline float StringView::toFloat(bool& isValid) const
@@ -469,7 +472,7 @@
{
if (is8Bit())
return charactersToFloat(characters8(), m_length, &isValid);
- return charactersToFloat(characters16(), length(), &isValid);
+ return charactersToFloat(characters16(), m_length, &isValid);
}
inline int StringView::toInt() const
@@ -482,7 +485,7 @@
{
if (is8Bit())
return charactersToInt(characters8(), m_length, &isValid);
- return charactersToInt(characters16(), length(), &isValid);
+ return charactersToInt(characters16(), m_length, &isValid);
}
inline int StringView::toIntStrict(bool& isValid) const
@@ -489,7 +492,7 @@
{
if (is8Bit())
return charactersToIntStrict(characters8(), m_length, &isValid);
- return charactersToIntStrict(characters16(), length(), &isValid);
+ return charactersToIntStrict(characters16(), m_length, &isValid);
}
inline String StringView::toStringWithoutCopying() const
@@ -496,7 +499,7 @@
{
if (is8Bit())
return StringImpl::createWithoutCopying(characters8(), m_length);
- return StringImpl::createWithoutCopying(characters16(), length());
+ return StringImpl::createWithoutCopying(characters16(), m_length);
}
inline size_t StringView::find(UChar character, unsigned start) const
@@ -503,7 +506,7 @@
{
if (is8Bit())
return WTF::find(characters8(), m_length, character, start);
- return WTF::find(characters16(), length(), character, start);
+ return WTF::find(characters16(), m_length, character, start);
}
inline size_t StringView::find(CharacterMatchFunction matchFunction, unsigned start) const
@@ -510,7 +513,7 @@
{
if (is8Bit())
return WTF::find(characters8(), m_length, matchFunction, start);
- return WTF::find(characters16(), length(), matchFunction, start);
+ return WTF::find(characters16(), m_length, matchFunction, start);
}
#if !CHECK_STRINGVIEW_LIFETIME
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-changes
