Diff
Modified: trunk/LayoutTests/ChangeLog (199215 => 199216)
--- trunk/LayoutTests/ChangeLog 2016-04-08 04:21:32 UTC (rev 199215)
+++ trunk/LayoutTests/ChangeLog 2016-04-08 04:26:27 UTC (rev 199216)
@@ -1,3 +1,14 @@
+2016-04-07 Darin Adler <[email protected]>
+
+ FontFaceSet binding does not handle null correctly
+ https://bugs.webkit.org/show_bug.cgi?id=156141
+
+ Reviewed by Youenn Fablet.
+
+ * fast/text/font-face-set-_javascript_-expected.txt: Added expected results for new tests.
+ * fast/text/font-face-set-_javascript_.html: Added tests for handling of null, also added tests for
+ the has function.
+
2016-04-07 Filip Pizlo <[email protected]>
Implementing caching transition puts that need to reallocate with indexing storage
Modified: trunk/LayoutTests/fast/text/font-face-set-_javascript_-expected.txt (199215 => 199216)
--- trunk/LayoutTests/fast/text/font-face-set-_javascript_-expected.txt 2016-04-08 04:21:32 UTC (rev 199215)
+++ trunk/LayoutTests/fast/text/font-face-set-_javascript_-expected.txt 2016-04-08 04:26:27 UTC (rev 199216)
@@ -13,6 +13,7 @@
PASS item.done is true
PASS fontFaceSet.add(fontFace2) is fontFaceSet
PASS fontFaceSet.size is 2
+PASS fontFaceSet.add(null) threw exception Error: TypeMismatchError: DOM Exception 17.
PASS item.done is false
PASS item.value is fontFace1
PASS item.done is false
@@ -20,6 +21,10 @@
PASS item.done is true
PASS fontFaceSet.delete(fontFace1) is true
PASS fontFaceSet.delete(fontFace3) is false
+PASS fontFaceSet.delete(null) threw exception Error: TypeMismatchError: DOM Exception 17.
+PASS fontFaceSet.has(fontFace1) is false
+PASS fontFaceSet.has(fontFace2) is true
+PASS fontFaceSet.has(null) threw exception Error: TypeMismatchError: DOM Exception 17.
PASS fontFaceSet.size is 0
PASS fontFaceSet.values().next().done is true
PASS fontFaceSet.check('garbage') threw exception Error: SyntaxError: DOM Exception 12.
Modified: trunk/LayoutTests/fast/text/font-face-set-_javascript_.html (199215 => 199216)
--- trunk/LayoutTests/fast/text/font-face-set-_javascript_.html 2016-04-08 04:21:32 UTC (rev 199215)
+++ trunk/LayoutTests/fast/text/font-face-set-_javascript_.html 2016-04-08 04:26:27 UTC (rev 199216)
@@ -5,7 +5,7 @@
<body>
<script>
if (window.internals)
- window.internals.clearMemoryCache();
+ internals.clearMemoryCache();
var fontFace1 = new FontFace("family1", "url('asdf')", {});
var fontFace2 = new FontFace("family2", "url('asdf')", {});
@@ -44,6 +44,8 @@
shouldBe("fontFaceSet.add(fontFace2)", "fontFaceSet");
shouldBe("fontFaceSet.size", "2");
+shouldThrow("fontFaceSet.add(null)");
+
iterator = fontFaceSet.keys();
item = iterator.next();
shouldBeFalse("item.done");
@@ -56,7 +58,15 @@
shouldBe("fontFaceSet.delete(fontFace1)", "true");
shouldBe("fontFaceSet.delete(fontFace3)", "false");
+
+shouldThrow("fontFaceSet.delete(null)");
+
+shouldBeFalse("fontFaceSet.has(fontFace1)");
+shouldBeTrue("fontFaceSet.has(fontFace2)");
+shouldThrow("fontFaceSet.has(null)");
+
fontFaceSet.clear();
+
shouldBe("fontFaceSet.size", "0");
shouldBeTrue("fontFaceSet.values().next().done");
shouldThrow("fontFaceSet.check('garbage')");
Modified: trunk/Source/WebCore/ChangeLog (199215 => 199216)
--- trunk/Source/WebCore/ChangeLog 2016-04-08 04:21:32 UTC (rev 199215)
+++ trunk/Source/WebCore/ChangeLog 2016-04-08 04:26:27 UTC (rev 199216)
@@ -1,3 +1,34 @@
+2016-04-07 Darin Adler <[email protected]>
+
+ FontFaceSet binding does not handle null correctly
+ https://bugs.webkit.org/show_bug.cgi?id=156141
+
+ Reviewed by Youenn Fablet.
+
+ * css/FontFaceSet.cpp:
+ (WebCore::FontFaceSet::FontFaceSet): Pass a reference to add rather than a pointer.
+ (WebCore::FontFaceSet::has): Take a reference rather than a pointer.
+ (WebCore::FontFaceSet::add): Ditto.
+ (WebCore::FontFaceSet::remove): Ditto.
+ (WebCore::FontFaceSet::load): Initialize ec since we check it. Caller is not required
+ to do this, nor is the matchingFaces function. Rearranged function to avoid needless
+ creation/destruction of PendingPromise for the immediate failure case. Removed some
+ unneeded type casts and local variables.
+ (WebCore::FontFaceSet::status): Use ASCIILiteral instead of ConstructFromLiteral.
+ No reason to use the more aggressive optimization.
+ (WebCore::FontFaceSet::faceFinished): Factored out a common hasReachedTerminalState
+ check to streamline the logic a bit.
+ (WebCore::FontFaceSet::load): Moved overload without a string in here; not critical
+ to inline it.
+ (WebCore::FontFaceSet::check): Ditto.
+
+ * css/FontFaceSet.h: Removed many unneeded includes and forward declarations.
+ Changed functions to take FontFace& instead of RefPtr<FontFace>. Removed unneeded
+ WebCore namespace prefixes. Use final instead of override for virtual functions.
+
+ * css/FontFaceSet.idl: Removed UsePointersEvenForNonNullableObjectArguments, which
+ was preserving incorrect behavior for null as demonstrated by the test cases.
+
2016-04-07 Joseph Pecoraro <[email protected]>
Remove ENABLE(ENABLE_ES6_CLASS_SYNTAX) guards
Modified: trunk/Source/WebCore/css/FontFaceSet.cpp (199215 => 199216)
--- trunk/Source/WebCore/css/FontFaceSet.cpp 2016-04-08 04:21:32 UTC (rev 199215)
+++ trunk/Source/WebCore/css/FontFaceSet.cpp 2016-04-08 04:26:27 UTC (rev 199216)
@@ -62,7 +62,7 @@
{
m_backing->addClient(*this);
for (auto& face : initialFaces)
- add(face.get());
+ add(*face);
}
FontFaceSet::FontFaceSet(Document& document, CSSFontFaceSet& backing)
@@ -99,11 +99,9 @@
{
}
-bool FontFaceSet::has(RefPtr<WebCore::FontFace> face) const
+bool FontFaceSet::has(FontFace& face) const
{
- if (!face)
- return false;
- return m_backing->hasFace(face->backing());
+ return m_backing->hasFace(face.backing());
}
size_t FontFaceSet::size() const
@@ -111,21 +109,18 @@
return m_backing->faceCount();
}
-FontFaceSet& FontFaceSet::add(RefPtr<WebCore::FontFace> face)
+FontFaceSet& FontFaceSet::add(FontFace& face)
{
- if (face && !m_backing->hasFace(face->backing()))
- m_backing->add(face->backing());
+ if (!m_backing->hasFace(face.backing()))
+ m_backing->add(face.backing());
return *this;
}
-bool FontFaceSet::remove(RefPtr<WebCore::FontFace> face)
+bool FontFaceSet::remove(FontFace& face)
{
- if (!face)
- return false;
-
- bool result = m_backing->hasFace(face->backing());
+ bool result = m_backing->hasFace(face.backing());
if (result)
- m_backing->remove(face->backing());
+ m_backing->remove(face.backing());
return result;
}
@@ -137,6 +132,7 @@
void FontFaceSet::load(JSC::ExecState& execState, const String& font, const String& text, DeferredWrapper&& promise, ExceptionCode& ec)
{
+ ec = 0;
auto matchingFaces = m_backing->matchingFaces(font, text, ec);
if (ec)
return;
@@ -149,23 +145,22 @@
for (auto& face : matchingFaces)
face.get().load();
- auto pendingPromise = PendingPromise::create(WTFMove(promise));
- bool waiting = false;
-
for (auto& face : matchingFaces) {
if (face.get().status() == CSSFontFace::Status::Failure) {
- pendingPromise->promise.reject(DOMCoreException::create(ExceptionCodeDescription(NETWORK_ERR)));
+ promise.reject(DOMCoreException::create(ExceptionCodeDescription(NETWORK_ERR)).ptr());
return;
}
}
+ auto pendingPromise = PendingPromise::create(WTFMove(promise));
+ bool waiting = false;
+
for (auto& face : matchingFaces) {
pendingPromise->faces.append(face.get().wrapper(execState));
if (face.get().status() == CSSFontFace::Status::Success)
continue;
waiting = true;
- auto& vector = m_pendingPromises.add(RefPtr<CSSFontFace>(&face.get()), Vector<Ref<PendingPromise>>()).iterator->value;
- vector.append(pendingPromise.copyRef());
+ m_pendingPromises.add(&face.get(), Vector<Ref<PendingPromise>>()).iterator->value.append(pendingPromise.copyRef());
}
if (!waiting)
@@ -191,12 +186,12 @@
{
switch (m_backing->status()) {
case CSSFontFaceSet::Status::Loading:
- return String("loading", String::ConstructFromLiteral);
+ return ASCIILiteral("loading");
case CSSFontFaceSet::Status::Loaded:
- return String("loaded", String::ConstructFromLiteral);
+ return ASCIILiteral("loaded");
}
ASSERT_NOT_REACHED();
- return String("loaded", String::ConstructFromLiteral);
+ return ASCIILiteral("loaded");
}
bool FontFaceSet::canSuspendForDocumentSuspension() const
@@ -235,21 +230,31 @@
return;
for (auto& pendingPromise : iterator->value) {
+ if (pendingPromise->hasReachedTerminalState)
+ continue;
if (newStatus == CSSFontFace::Status::Success) {
- if (pendingPromise->hasOneRef() && !pendingPromise->hasReachedTerminalState) {
+ if (pendingPromise->hasOneRef()) {
pendingPromise->promise.resolve(pendingPromise->faces);
pendingPromise->hasReachedTerminalState = true;
}
} else {
ASSERT(newStatus == CSSFontFace::Status::Failure);
- if (!pendingPromise->hasReachedTerminalState) {
- pendingPromise->promise.reject(DOMCoreException::create(ExceptionCodeDescription(NETWORK_ERR)));
- pendingPromise->hasReachedTerminalState = true;
- }
+ pendingPromise->promise.reject(DOMCoreException::create(ExceptionCodeDescription(NETWORK_ERR)));
+ pendingPromise->hasReachedTerminalState = true;
}
}
m_pendingPromises.remove(iterator);
}
+void FontFaceSet::load(JSC::ExecState& state, const String& font, DeferredWrapper&& promise, ExceptionCode& ec)
+{
+ load(state, font, ASCIILiteral(" "), WTFMove(promise), ec);
}
+
+bool FontFaceSet::check(const String& font, ExceptionCode& ec)
+{
+ return check(font, ASCIILiteral(" "), ec);
+}
+
+}
Modified: trunk/Source/WebCore/css/FontFaceSet.h (199215 => 199216)
--- trunk/Source/WebCore/css/FontFaceSet.h 2016-04-08 04:21:32 UTC (rev 199215)
+++ trunk/Source/WebCore/css/FontFaceSet.h 2016-04-08 04:26:27 UTC (rev 199216)
@@ -28,41 +28,28 @@
#include "ActiveDOMObject.h"
#include "CSSFontFaceSet.h"
-#include "DOMCoreException.h"
#include "EventTarget.h"
#include "JSDOMPromise.h"
-#include <wtf/HashTraits.h>
-#include <wtf/Optional.h>
-#include <wtf/Ref.h>
-#include <wtf/RefCounted.h>
-#include <wtf/RefPtr.h>
-#include <wtf/Vector.h>
-#include <wtf/text/WTFString.h>
-namespace JSC {
-class ExecState;
-}
-
namespace WebCore {
-class Document;
-class FontFace;
+class DOMCoreException;
-class FontFaceSet final : public RefCounted<FontFaceSet>, public CSSFontFaceSetClient, public EventTargetWithInlineData, public ActiveDOMObject {
+class FontFaceSet final : public RefCounted<FontFaceSet>, private CSSFontFaceSetClient, public EventTargetWithInlineData, private ActiveDOMObject {
public:
static Ref<FontFaceSet> create(Document&, const Vector<RefPtr<FontFace>>& initialFaces);
static Ref<FontFaceSet> create(Document&, CSSFontFaceSet& backing);
virtual ~FontFaceSet();
- bool has(RefPtr<WebCore::FontFace>) const;
+ bool has(FontFace&) const;
size_t size() const;
- FontFaceSet& add(RefPtr<WebCore::FontFace>);
- bool remove(RefPtr<WebCore::FontFace>);
+ FontFaceSet& add(FontFace&);
+ bool remove(FontFace&);
void clear();
- void load(JSC::ExecState& execState, const String& font, DeferredWrapper&& promise, ExceptionCode& ec) { load(execState, font, String(" ", String::ConstructFromLiteral), WTFMove(promise), ec); }
+ void load(JSC::ExecState&, const String& font, DeferredWrapper&& promise, ExceptionCode&);
void load(JSC::ExecState&, const String& font, const String& text, DeferredWrapper&& promise, ExceptionCode&);
- bool check(const String& font, ExceptionCode& ec) { return check(font, String(" ", String::ConstructFromLiteral), ec); }
+ bool check(const String& font, ExceptionCode&);
bool check(const String& font, const String& text, ExceptionCode&);
String status() const;
@@ -83,11 +70,11 @@
};
Iterator createIterator() { return Iterator(*this); }
- using RefCounted<FontFaceSet>::ref;
- using RefCounted<FontFaceSet>::deref;
+ using RefCounted::ref;
+ using RefCounted::deref;
private:
- struct PendingPromise : public RefCounted<PendingPromise> {
+ struct PendingPromise : RefCounted<PendingPromise> {
typedef DOMPromise<Vector<RefPtr<FontFace>>&, DOMCoreException&> Promise;
static Ref<PendingPromise> create(Promise&& promise)
{
@@ -110,19 +97,19 @@
void fulfillPromise();
// CSSFontFaceSetClient
- void startedLoading() override;
- void completedLoading() override;
- void faceFinished(CSSFontFace&, CSSFontFace::Status) override;
+ void startedLoading() final;
+ void completedLoading() final;
+ void faceFinished(CSSFontFace&, CSSFontFace::Status) final;
// ActiveDOMObject
- const char* activeDOMObjectName() const override { return "FontFaceSet"; }
- bool canSuspendForDocumentSuspension() const override;
+ const char* activeDOMObjectName() const final { return "FontFaceSet"; }
+ bool canSuspendForDocumentSuspension() const final;
// EventTarget
- EventTargetInterface eventTargetInterface() const override { return FontFaceSetEventTargetInterfaceType; }
- ScriptExecutionContext* scriptExecutionContext() const override { return ActiveDOMObject::scriptExecutionContext(); }
- void refEventTarget() override { ref(); }
- void derefEventTarget() override { deref(); }
+ EventTargetInterface eventTargetInterface() const final { return FontFaceSetEventTargetInterfaceType; }
+ ScriptExecutionContext* scriptExecutionContext() const final { return ActiveDOMObject::scriptExecutionContext(); }
+ void refEventTarget() final { ref(); }
+ void derefEventTarget() final { deref(); }
Ref<CSSFontFaceSet> m_backing;
HashMap<RefPtr<CSSFontFace>, Vector<Ref<PendingPromise>>> m_pendingPromises;
Modified: trunk/Source/WebCore/css/FontFaceSet.idl (199215 => 199216)
--- trunk/Source/WebCore/css/FontFaceSet.idl 2016-04-08 04:21:32 UTC (rev 199215)
+++ trunk/Source/WebCore/css/FontFaceSet.idl 2016-04-08 04:26:27 UTC (rev 199216)
@@ -31,7 +31,6 @@
[
ConstructorCallWith=Document,
Constructor(sequence<FontFace> initialFaces),
- UsePointersEvenForNonNullableObjectArguments,
] interface FontFaceSet : EventTarget {
boolean has(FontFace font);
