git://git.webkit.org / WebKit-https.git / commitdiff
? search:
summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: 808edba)
FontFaceSet binding does not handle null correctly
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Apr 2016 04:26:27 +0000 (04:26 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Apr 2016 04:26:27 +0000 (04:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=156141

Reviewed by Youenn Fablet.

Source/WebCore:

* 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.

LayoutTests:

* 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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@199216 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog patch | blob | history
LayoutTests/fast/text/font-face-set-javascript-expected.txt patch | blob | history
LayoutTests/fast/text/font-face-set-javascript.html patch | blob | history
Source/WebCore/ChangeLog patch | blob | history
Source/WebCore/css/FontFaceSet.cpp patch | blob | history
Source/WebCore/css/FontFaceSet.h patch | blob | history
Source/WebCore/css/FontFaceSet.idl patch | blob | history

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index aa1169c..c1edefc 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,14 @@
+2016-04-07  Darin Adler  <darin@apple.com>
+
+        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  <fpizlo@apple.com>
 
         Implementing caching transition puts that need to reallocate with indexing storage
diff --git a/LayoutTests/fast/text/font-face-set-javascript-expected.txt b/LayoutTests/fast/text/font-face-set-javascript-expected.txt
index ec88ef3..38a41ac 100644 (file)
--- a/LayoutTests/fast/text/font-face-set-javascript-expected.txt
+++ b/LayoutTests/fast/text/font-face-set-javascript-expected.txt
@@ -13,6 +13,7 @@ PASS item.value is fontFace1
 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.value is fontFace2
 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.
diff --git a/LayoutTests/fast/text/font-face-set-javascript.html b/LayoutTests/fast/text/font-face-set-javascript.html
index 91fa3b9..5489a2d 100644 (file)
--- a/LayoutTests/fast/text/font-face-set-javascript.html
+++ b/LayoutTests/fast/text/font-face-set-javascript.html
@@ -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 @@ shouldBeTrue("item.done");
 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 @@ shouldBeTrue("item.done");
 
 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')");
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 29decf6..e21ff36 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,34 @@
+2016-04-07  Darin Adler  <darin@apple.com>
+
+        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  <pecoraro@apple.com>
 
         Remove ENABLE(ENABLE_ES6_CLASS_SYNTAX) guards
diff --git a/Source/WebCore/css/FontFaceSet.cpp b/Source/WebCore/css/FontFaceSet.cpp
index 3f9f58b..d80cc9a 100644 (file)
--- a/Source/WebCore/css/FontFaceSet.cpp
+++ b/Source/WebCore/css/FontFaceSet.cpp
@@ -62,7 +62,7 @@ FontFaceSet::FontFaceSet(Document& document, const Vector<RefPtr<FontFace>>& ini
 {
     m_backing->addClient(*this);
     for (auto& face : initialFaces)
-        add(face.get());
+        add(*face);
 }
 
 FontFaceSet::FontFaceSet(Document& document, CSSFontFaceSet& backing)
@@ -99,11 +99,9 @@ FontFaceSet::PendingPromise::~PendingPromise()
 {
 }
 
-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 @@ size_t FontFaceSet::size() const
     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::clear()
 
 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 @@ void FontFaceSet::load(JSC::ExecState& execState, const String& font, const Stri
     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 @@ String FontFaceSet::status() const
 {
     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 @@ void FontFaceSet::faceFinished(CSSFontFace& face, CSSFontFace::Status newStatus)
         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);
+}
+
 }
diff --git a/Source/WebCore/css/FontFaceSet.h b/Source/WebCore/css/FontFaceSet.h
index 870d133..dcea766 100644 (file)
--- a/Source/WebCore/css/FontFaceSet.h
+++ b/Source/WebCore/css/FontFaceSet.h
@@ -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 @@ public:
     };
     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 @@ private:
     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;
diff --git a/Source/WebCore/css/FontFaceSet.idl b/Source/WebCore/css/FontFaceSet.idl
index bf7143a..b19229a 100644 (file)
--- a/Source/WebCore/css/FontFaceSet.idl
+++ b/Source/WebCore/css/FontFaceSet.idl
@@ -31,7 +31,6 @@ enum FontFaceSetLoadStatus {
 [
     ConstructorCallWith=Document,
     Constructor(sequence<FontFace> initialFaces),
-    UsePointersEvenForNonNullableObjectArguments,
 ] interface FontFaceSet : EventTarget {
     boolean has(FontFace font);
 
Mirror of the WebKit open source project from svn.webkit.org.