Attachment 340571 Details for Bug 185330 – Sanity check v1

[patch] Sanity check v1

bug-185330-all-in-one-sanity-check.diff (text/plain), 15.75 KB, created by David Kilzer (:ddkilzer) on 2018-05-17 04:07:18 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: David Kilzer (:ddkilzer)

Created: 2018-05-17 04:07:18 PDT

Size: 15.75 KB

patch

obsolete

>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index fd142b5eb23..aa436ac5455 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,81 @@
>+2018-05-17  David Kilzer  <ddkilzer@apple.com>
>+
>+        [iOS] Add assert to catch improper use of WebCore::Timer in UI Process
>+        <https://webkit.org/b/185330>
>+        <rdar://problem/32816079>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * platform/RuntimeApplicationChecks.h:
>+        (WebCore::setIsUIProcess): Declare new method.
>+        (WebCore::isUIProcess): Ditto.
>+        * platform/Timer.cpp:
>+        (WebCore::TimerBase::TimerBase): Add assert.  This catches the
>+        unwanted behavior.
>+        (WebCore::TimerBase::isAllowedToUseWebCoreTimerInUIProcess): Add.
>+        * platform/Timer.h:
>+        (WebCore::TimerBase::isAllowedToUseWebCoreTimerInUIProcess): Add
>+        declaration.
>+        * platform/cocoa/RuntimeApplicationChecksCocoa.mm:
>+        (s_isUIProcess): Add. Global to track UI Process state.
>+        (WebCore::setIsUIProcess): Add. Sets global to true.
>+        (WebCore::isUIProcess): Retun value of global.
>+
>+2018-05-16  David Kilzer  <ddkilzer@apple.com>
>+
>+        Don't create the SubimageCache just to clear an image from it
>+        <https://webkit.org/b/000000>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * platform/graphics/cg/GraphicsContextCG.cpp:
>+        (WebCore::GraphicsContext::drawNativeImage): Use new
>+        SubimageCache::getSubimage() helper function.
>+        * platform/graphics/cg/NativeImageCG.cpp:
>+        (WebCore::clearNativeImageSubimages): Use new
>+        SubimageCache::clearImage() helper function which returns early
>+        if the subimage cache has not been created yet.  This fixes the
>+        bug.
>+        * platform/graphics/cg/SubimageCacheWithTimer.cpp:
>+        (WebCore::SubimageCacheWithTimer::getSubimage): Use `auto` after
>+        renaming SubimageCache typedef to SubimageCacheHashSet.
>+        (WebCore::SubimageCacheWithTimer::clearImage): Ditto.  Modernize
>+        loops.
>+        (WebCore::subimageCache): Delete.  Replaced by
>+        SubimageCacheWithTimer::getCache().
>+        (WebCore::SubimageCacheWithTimer::cacheExists): Add.  Returns
>+        false if the subimage cache has not been created yet.
>+        (WebCore::SubimageCacheWithTimer::getCache): Add.  Creates
>+        subimage cache if it doesn't exist yet using std::call_once(),
>+        and returns reference to subimage cache.  Replaces
>+        WebCore::subimageCache().
>+        * platform/graphics/cg/SubimageCacheWithTimer.h:
>+        (WebCore::SubimageCache::getSubimage): Add.  Inline helper
>+        function that simplifies subimage cache use.
>+        (WebCore::SubimageCache::clearImage): Add.  Ditto.
>+
>+2018-05-16  David Kilzer  <ddkilzer@apple.com>
>+
>+        Lazily create WebCore::Timer for WebCore::Image
>+        <https://webkit.org/b/000000>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Not every image is an animated image, so lazily creating
>+        m_animationStartTimer saves 56 bytes per instance of
>+        WebCore::Image.
>+
>+        * platform/graphics/Image.cpp:
>+        (WebCore::Image::Image): Remove default initializer for
>+        m_animationStartTimer.
>+        (WebCore::Image::startAnimationAsynchronously): Use
>+        std::call_once to initialize m_animationStartTimer.
>+        * platform/graphics/Image.h:
>+        (WebCore::Image::animationPending const): Update to check if
>+        m_animationStartTimer has been set before dereferencing it.
>+        (WebCore::Image::m_animationStartTimer): Change type to
>+        std::unique_ptr<Timer>.
>+
> 2018-05-07  Chris Dumez  <cdumez@apple.com>
> 
>         ASSERT(!childItemWithTarget(child->target())) is hit in HistoryItem::addChildItem()
>diff --git a/Source/WebCore/platform/RuntimeApplicationChecks.h b/Source/WebCore/platform/RuntimeApplicationChecks.h
>index 94e28f8592e..11817d3f99a 100644
>--- a/Source/WebCore/platform/RuntimeApplicationChecks.h
>+++ b/Source/WebCore/platform/RuntimeApplicationChecks.h
>@@ -40,6 +40,9 @@ inline bool isInWebProcess() { return true; }
> 
> #if PLATFORM(COCOA)
> 
>+WEBCORE_EXPORT void setIsUIProcess();
>+bool isUIProcess();
>+
> bool isInWebProcess();
> 
> WEBCORE_EXPORT void setApplicationBundleIdentifier(const String&);
>diff --git a/Source/WebCore/platform/Timer.cpp b/Source/WebCore/platform/Timer.cpp
>index 43a5920bfc1..4ddbd27ce82 100644
>--- a/Source/WebCore/platform/Timer.cpp
>+++ b/Source/WebCore/platform/Timer.cpp
>@@ -27,6 +27,8 @@
> #include "config.h"
> #include "Timer.h"
> 
>+#include "Logging.h"
>+#include "RuntimeApplicationChecks.h"
> #include "SharedTimer.h"
> #include "ThreadGlobalData.h"
> #include "ThreadTimers.h"
>@@ -36,6 +38,10 @@
> #include <wtf/MainThread.h>
> #include <wtf/Vector.h>
> 
>+#if USE(WEB_THREAD)
>+#include "WebCoreThread.h"
>+#endif
>+
> namespace WebCore {
> 
> class TimerHeapReference;
>@@ -186,6 +192,15 @@ inline bool TimerHeapLessThanFunction::operator()(const TimerBase* a, const Time
> 
> TimerBase::TimerBase()
> {
>+#if PLATFORM(IOS)
>+    // FIXME: Enable for PLATFORM(COCOA): <rdar://problem/40005654> IOSurfacePool should stop using WebCore::Timer.
>+    if (!isAllowedToUseWebCoreTimerInUIProcess()) {
>+#define WEBCORE_TIMERBASE_ASSERTION_MESSAGE "WebCore::Timer should not be used in UI Process."
>+        RELEASE_LOG_FAULT(Threading, WEBCORE_TIMERBASE_ASSERTION_MESSAGE);
>+        ASSERT_WITH_MESSAGE(false, WEBCORE_TIMERBASE_ASSERTION_MESSAGE);
>+#undef WEBCORE_TIMERBASE_ASSERTION_MESSAGE
>+    }
>+#endif
> }
> 
> TimerBase::~TimerBase()
>@@ -242,6 +257,23 @@ inline void TimerBase::checkConsistency() const
>         checkHeapIndex();
> }
> 
>+bool TimerBase::isAllowedToUseWebCoreTimerInUIProcess()
>+{
>+#if PLATFORM(COCOA)
>+    if (!isUIProcess())
>+        return true;
>+
>+#if USE(WEB_THREAD)
>+    if (WebThreadIsEnabled() && (WebThreadIsCurrent() || WebThreadIsLocked()))
>+        return true;
>+#endif
>+
>+    return false;
>+#else
>+    return true;
>+#endif
>+}
>+
> void TimerBase::heapDecreaseKey()
> {
>     ASSERT(static_cast<bool>(m_nextFireTime));
>diff --git a/Source/WebCore/platform/Timer.h b/Source/WebCore/platform/Timer.h
>index 5e757d40555..cc87e95a060 100644
>--- a/Source/WebCore/platform/Timer.h
>+++ b/Source/WebCore/platform/Timer.h
>@@ -78,6 +78,8 @@ private:
>     void checkConsistency() const;
>     void checkHeapIndex() const;
> 
>+    static bool isAllowedToUseWebCoreTimerInUIProcess();
>+
>     void setNextFireTime(MonotonicTime);
> 
>     bool inHeap() const { return m_heapIndex != -1; }
>diff --git a/Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm b/Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm
>index 377dcaa1e0d..02b1fcc9dfa 100644
>--- a/Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm
>+++ b/Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm
>@@ -38,6 +38,7 @@ namespace WebCore {
> #if !ASSERT_MSG_DISABLED
> static bool applicationBundleIdentifierOverrideWasQueried;
> #endif
>+static bool s_isUIProcess;
> 
> // The application bundle identifier gets set to the UIProcess bundle identifier by the WebProcess and
> // the Networking upon initialization. It is unset otherwise.
>@@ -65,6 +66,16 @@ void setApplicationBundleIdentifier(const String& bundleIdentifier)
>     applicationBundleIdentifierOverride() = bundleIdentifier;
> }
> 
>+void setIsUIProcess()
>+{
>+    s_isUIProcess = true;
>+}
>+
>+bool isUIProcess()
>+{
>+    return s_isUIProcess;
>+}
>+
> bool isInWebProcess()
> {
>     static bool mainBundleIsWebProcess = [[[NSBundle mainBundle] bundleIdentifier] isEqualToString:@"com.apple.WebKit.WebContent.Development"]
>diff --git a/Source/WebCore/platform/graphics/Image.cpp b/Source/WebCore/platform/graphics/Image.cpp
>index 90e816f27f6..bf4d98828a6 100644
>--- a/Source/WebCore/platform/graphics/Image.cpp
>+++ b/Source/WebCore/platform/graphics/Image.cpp
>@@ -50,7 +50,6 @@ namespace WebCore {
> 
> Image::Image(ImageObserver* observer)
>     : m_imageObserver(observer)
>-    , m_animationStartTimer(*this, &Image::startAnimation)
> {
> }
> 
>@@ -349,9 +348,12 @@ void Image::computeIntrinsicDimensions(Length& intrinsicWidth, Length& intrinsic
> 
> void Image::startAnimationAsynchronously()
> {
>-    if (m_animationStartTimer.isActive())
>+    std::call_once(onceFlag, [this] {
>+        m_animationStartTimer = std::make_unique<Timer>(*this, &Image::startAnimation);
>+    });
>+    if (m_animationStartTimer->isActive())
>         return;
>-    m_animationStartTimer.startOneShot(0_s);
>+    m_animationStartTimer->startOneShot(0_s);
> }
> 
> void Image::dump(TextStream& ts) const
>diff --git a/Source/WebCore/platform/graphics/Image.h b/Source/WebCore/platform/graphics/Image.h
>index 65988f79bf2..2450c7d7603 100644
>--- a/Source/WebCore/platform/graphics/Image.h
>+++ b/Source/WebCore/platform/graphics/Image.h
>@@ -138,7 +138,7 @@ public:
>     virtual void stopAnimation() {}
>     virtual void resetAnimation() {}
>     virtual bool isAnimating() const { return false; }
>-    bool animationPending() const { return m_animationStartTimer.isActive(); }
>+    bool animationPending() const { return m_animationStartTimer && m_animationStartTimer->isActive(); }
> 
>     // Typically the CachedImage that owns us.
>     ImageObserver* imageObserver() const { return m_imageObserver; }
>@@ -200,7 +200,8 @@ protected:
> private:
>     RefPtr<SharedBuffer> m_encodedImageData;
>     ImageObserver* m_imageObserver;
>-    Timer m_animationStartTimer;
>+    std::once_flag onceFlag;
>+    std::unique_ptr<Timer> m_animationStartTimer;
> };
> 
> WTF::TextStream& operator<<(WTF::TextStream&, const Image&);
>diff --git a/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp b/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp
>index 895c90ed10c..4a780dd1d50 100644
>--- a/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp
>+++ b/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp
>@@ -330,7 +330,7 @@ void GraphicsContext::drawNativeImage(const RetainPtr<CGImageRef>& image, const
>             adjustedDestRect.setHeight(subimageRect.height() / yScale);
> 
> #if CACHE_SUBIMAGES
>-            subImage = subimageCache().getSubimage(subImage.get(), subimageRect);
>+            subImage = SubimageCache::getSubimage(subImage.get(), subimageRect);
> #else
>             subImage = adoptCF(CGImageCreateWithImageInRect(subImage.get(), subimageRect));
> #endif
>diff --git a/Source/WebCore/platform/graphics/cg/NativeImageCG.cpp b/Source/WebCore/platform/graphics/cg/NativeImageCG.cpp
>index b08026bd1d7..adbecf848ac 100644
>--- a/Source/WebCore/platform/graphics/cg/NativeImageCG.cpp
>+++ b/Source/WebCore/platform/graphics/cg/NativeImageCG.cpp
>@@ -86,7 +86,7 @@ void clearNativeImageSubimages(const NativeImagePtr& image)
> {
> #if CACHE_SUBIMAGES
>     if (image)
>-        subimageCache().clearImage(image.get());
>+        SubimageCache::clearImage(image.get());
> #endif
> }
> 
>diff --git a/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp b/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp
>index 33923ec4901..85ca3b79e2c 100644
>--- a/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp
>+++ b/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp
>@@ -34,6 +34,9 @@
> 
> namespace WebCore {
> 
>+SubimageCacheWithTimer* SubimageCacheWithTimer::s_cache;
>+std::once_flag SubimageCacheWithTimer::s_onceFlag;
>+
> static const Seconds subimageCacheClearDelay { 1_s };
> static const int maxSubimageCacheSize = 300;
> 
>@@ -83,7 +86,7 @@ RetainPtr<CGImageRef> SubimageCacheWithTimer::getSubimage(CGImageRef image, cons
>     }
> 
>     ASSERT(m_cache.size() < maxSubimageCacheSize);
>-    SubimageCache::AddResult result = m_cache.add<SubimageCacheAdder>(SubimageRequest(image, rect));
>+    auto result = m_cache.add<SubimageCacheAdder>(SubimageRequest(image, rect));
>     if (result.isNewEntry)
>         m_images.add(image);
> 
>@@ -94,23 +97,29 @@ void SubimageCacheWithTimer::clearImage(CGImageRef image)
> {
>     if (m_images.contains(image)) {
>         Vector<SubimageCacheEntry> toBeRemoved;
>-        SubimageCache::const_iterator end = m_cache.end();
>-        for (SubimageCache::const_iterator it = m_cache.begin(); it != end; ++it) {
>-            if (it->image.get() == image)
>-                toBeRemoved.append(*it);
>+        for (const auto& entry : m_cache) {
>+            if (entry.image.get() == image)
>+                toBeRemoved.append(entry);
>         }
> 
>-        for (Vector<SubimageCacheEntry>::iterator removeIt = toBeRemoved.begin(); removeIt != toBeRemoved.end(); ++removeIt)
>-            m_cache.remove(*removeIt);
>+        for (auto& entry : toBeRemoved)
>+            m_cache.remove(entry);
> 
>         m_images.removeAll(image);
>     }
> }
> 
>-SubimageCacheWithTimer& subimageCache()
>+bool SubimageCacheWithTimer::cacheExists()
>+{
>+    return !!SubimageCacheWithTimer::s_cache;
>+}
>+
>+SubimageCacheWithTimer& SubimageCacheWithTimer::getCache()
> {
>-    static SubimageCacheWithTimer& cache = *new SubimageCacheWithTimer;
>-    return cache;
>+    std::call_once(SubimageCacheWithTimer::s_onceFlag, [] {
>+        SubimageCacheWithTimer::s_cache = new SubimageCacheWithTimer;
>+    });
>+    return *SubimageCacheWithTimer::s_cache;
> }
> 
> }
>diff --git a/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h b/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h
>index 9033e797372..fc972e03d5a 100644
>--- a/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h
>+++ b/Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h
>@@ -80,22 +80,44 @@ public:
>         static const bool safeToCompareToEmptyOrDeleted = true;
>     };
> 
>-    typedef HashSet<SubimageCacheEntry, SubimageCacheHash, SubimageCacheEntryTraits> SubimageCache;
>+    typedef HashSet<SubimageCacheEntry, SubimageCacheHash, SubimageCacheEntryTraits> SubimageCacheHashSet;
> 
>-public:
>-    SubimageCacheWithTimer();
>     RetainPtr<CGImageRef> getSubimage(CGImageRef, const FloatRect&);
>     void clearImage(CGImageRef);
> 
>+    static bool cacheExists();
>+    static SubimageCacheWithTimer& getCache();
>+
> private:
>+    SubimageCacheWithTimer();
>     void invalidateCacheTimerFired();
> 
>     HashCountedSet<CGImageRef> m_images;
>-    SubimageCache m_cache;
>+    SubimageCacheHashSet m_cache;
>     DeferrableOneShotTimer m_timer;
>+
>+    static SubimageCacheWithTimer* s_cache;
>+    static std::once_flag s_onceFlag;
> };
> 
>-SubimageCacheWithTimer& subimageCache();
>+namespace SubimageCache {
>+
>+RetainPtr<CGImageRef> getSubimage(CGImageRef, const FloatRect&);
>+void clearImage(CGImageRef);
>+
>+}
>+
>+inline RetainPtr<CGImageRef> SubimageCache::getSubimage(CGImageRef image, const FloatRect& rect)
>+{
>+    return SubimageCacheWithTimer::getCache().getSubimage(image, rect);
>+}
>+
>+inline void SubimageCache::clearImage(CGImageRef image)
>+{
>+    if (!SubimageCacheWithTimer::cacheExists())
>+        return;
>+    SubimageCacheWithTimer::getCache().clearImage(image);
>+}
> 
> #endif // CACHE_SUBIMAGES
> 
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 3fe1e160c5c..4e9db90677d 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-05-17  David Kilzer  <ddkilzer@apple.com>
>+
>+        [iOS] Add assert to catch improper use of WebCore::Timer in UI Process
>+        <https://webkit.org/b/185330>
>+        <rdar://problem/32816079>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * UIProcess/API/Cocoa/WKWebViewConfiguration.mm:
>+        (-[WKWebViewConfiguration init]): Call WebCore::setIsUIProcess()
>+        to set the global flag when a WKWebViewConfiguration object is
>+        created in the current process.
>+
> 2018-05-07  Megan Gardner  <megan_gardner@apple.com>
> 
>         Allow Web Touch events to timeout
>diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm b/Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm
>index 94068975fcb..e744b83c050 100644
>--- a/Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm
>+++ b/Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm
>@@ -172,6 +172,7 @@ - (instancetype)init
>         return nil;
> 
>     WebKit::InitializeWebKit2();
>+    WebCore::setIsUIProcess();
> 
> #if PLATFORM(IOS)
> #if !ENABLE(EXTRA_ZOOM_MODE)

Actions: View | Formatted Diff | Diff

Attachments on bug 185330: 340571 | 340637 | 341175 | 341181 | 341183 | 341541 | 344076 | 344083 | 344217 | 344224