Incremental bytecode cache should not append function updates when loaded from memory
authortzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Apr 2019 21:55:33 +0000 (21:55 +0000)
committertzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Apr 2019 21:55:33 +0000 (21:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196865

Reviewed by Filip Pizlo.

JSTests:

* stress/bytecode-cache-shared-code-block.js: Added.
(b):
(program):

Source/JavaScriptCore:

Function updates hold the assumption that a function can only be executed/cached
after its containing code block has already been cached. This assumptions does
not hold if the UnlinkedCodeBlock is loaded from memory by the CodeCache, since
we might have two independent SourceProviders executing different paths of the
code and causing the same UnlinkedCodeBlock to be modified in memory.
Use a RefPtr instead of Ref for m_cachedBytecode in ShellSourceProvider to distinguish
between a new, empty cache and a cache that was not loaded and therefore cannot be updated.

* jsc.cpp:
(ShellSourceProvider::ShellSourceProvider):

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

JSTests/ChangeLog
JSTests/stress/bytecode-cache-shared-code-block.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/jsc.cpp

index 3d14bdf..ea428c7 100644 (file)
@@ -1,3 +1,14 @@
+2019-04-15  Tadeu Zagallo  <tzagallo@apple.com>
+
+        Incremental bytecode cache should not append function updates when loaded from memory
+        https://bugs.webkit.org/show_bug.cgi?id=196865
+
+        Reviewed by Filip Pizlo.
+
+        * stress/bytecode-cache-shared-code-block.js: Added.
+        (b):
+        (program):
+
 2019-04-13  Tadeu Zagallo  <tzagallo@apple.com>
 
         CodeCache should check that the UnlinkedCodeBlock was successfully created before caching it
diff --git a/JSTests/stress/bytecode-cache-shared-code-block.js b/JSTests/stress/bytecode-cache-shared-code-block.js
new file mode 100644 (file)
index 0000000..a9e8fc6
--- /dev/null
@@ -0,0 +1,10 @@
+//@ runBytecodeCache
+
+var program = `(function () {
+    function a() { }
+    function b() { }
+    return { a, b };
+})`;
+
+loadString(program)().a();
+loadString(program)().b();
index efa2b5b..f4e5357 100644 (file)
@@ -1,3 +1,21 @@
+2019-04-15  Tadeu Zagallo  <tzagallo@apple.com>
+
+        Incremental bytecode cache should not append function updates when loaded from memory
+        https://bugs.webkit.org/show_bug.cgi?id=196865
+
+        Reviewed by Filip Pizlo.
+
+        Function updates hold the assumption that a function can only be executed/cached
+        after its containing code block has already been cached. This assumptions does
+        not hold if the UnlinkedCodeBlock is loaded from memory by the CodeCache, since
+        we might have two independent SourceProviders executing different paths of the
+        code and causing the same UnlinkedCodeBlock to be modified in memory.
+        Use a RefPtr instead of Ref for m_cachedBytecode in ShellSourceProvider to distinguish
+        between a new, empty cache and a cache that was not loaded and therefore cannot be updated.
+
+        * jsc.cpp:
+        (ShellSourceProvider::ShellSourceProvider):
+
 2019-04-15  Saam barati  <sbarati@apple.com>
 
         mergeOSREntryValue is wrong when the incoming value does not match up with the flush format
index 78d1142..c110dc4 100644 (file)
@@ -972,14 +972,14 @@ public:
 
     RefPtr<CachedBytecode> cachedBytecode() const override
     {
-        if (!m_cachedBytecode->size())
+        if (!m_cachedBytecode)
             loadBytecode();
         return m_cachedBytecode.copyRef();
     }
 
     void updateCache(const UnlinkedFunctionExecutable* executable, const SourceCode&, CodeSpecializationKind kind, const UnlinkedFunctionCodeBlock* codeBlock) const override
     {
-        if (!cacheEnabled())
+        if (!cacheEnabled() || !m_cachedBytecode)
             return;
         Ref<CachedBytecode> cachedBytecode = encodeFunctionCodeBlock(*executable->vm(), codeBlock);
         m_cachedBytecode->addFunctionUpdate(executable, kind, WTFMove(cachedBytecode));
@@ -989,17 +989,19 @@ public:
     {
         if (!cacheEnabled())
             return;
+        if (!m_cachedBytecode)
+            m_cachedBytecode = CachedBytecode::create();
         m_cachedBytecode->addGlobalUpdate(generator());
     }
 
     void commitCachedBytecode() const override
     {
 #if OS(DARWIN)
-        if (!cacheEnabled() || !m_cachedBytecode->hasUpdates())
+        if (!cacheEnabled() || !m_cachedBytecode || !m_cachedBytecode->hasUpdates())
             return;
 
         auto clearBytecode = makeScopeExit([&] {
-            m_cachedBytecode = CachedBytecode::create();
+            m_cachedBytecode = nullptr;
         });
 
         String filename = cachePath();
@@ -1075,9 +1077,7 @@ private:
 
     ShellSourceProvider(const String& source, const SourceOrigin& sourceOrigin, URL&& url, const TextPosition& startPosition, SourceProviderSourceType sourceType)
         : StringSourceProvider(source, sourceOrigin, WTFMove(url), startPosition, sourceType)
-        , m_cachedBytecode(CachedBytecode::create())
     {
-        loadBytecode();
     }
 
     static bool cacheEnabled()
@@ -1086,7 +1086,7 @@ private:
         return enabled;
     }
 
-    mutable Ref<CachedBytecode> m_cachedBytecode;
+    mutable RefPtr<CachedBytecode> m_cachedBytecode;
 };
 
 static inline SourceCode jscSource(const String& source, const SourceOrigin& sourceOrigin, URL&& url = URL(), const TextPosition& startPosition = TextPosition(), SourceProviderSourceType sourceType = SourceProviderSourceType::Program)