- Revision
- 223690
- Author
- cdu...@apple.com
- Date
- 2017-10-19 09:10:47 -0700 (Thu, 19 Oct 2017)
Log Message
http/tests/workers/service/basic-register.html is a flaky failure.
https://bugs.webkit.org/show_bug.cgi?id=178494
<rdar://problem/35065315>
Reviewed by Youenn Fablet.
In WebSWServerConnection::resolveJobInClient(), when a service worker is
registered, we:
1. Add the origin to the WebSWOriginStore
2. Send the IPC to the WebProcess to notify it that the registration succeeded.
The assumption was that step 1 would be synchronous and would therefore send
the shared memory handle to the WebProcess (if the SharedMemory was invalidated)
*before* step 2.
The issue is that step 1 was scheduling a zero-timer to schedule the addition.
As a result, there was a race and the WebContent process could check the
the WebSWOriginTable *after* being notified that a service worker was registered
but *before* it received the SharedMemory handle for the WebSWOriginTable. This
could lead to false negatives and was causing the layout test to be flaky.
To address the issue, step 1 is now synchronous.
* Shared/SharedStringHashStore.cpp:
(WebKit::SharedStringHashStore::SharedStringHashStore):
(WebKit::SharedStringHashStore::scheduleAddition):
(WebKit::SharedStringHashStore::scheduleRemoval):
(WebKit::SharedStringHashStore::contains):
(WebKit::SharedStringHashStore::flushPendingChanges):
(WebKit::SharedStringHashStore::processPendingOperations):
* Shared/SharedStringHashStore.h:
* StorageProcess/ServiceWorker/WebSWOriginStore.cpp:
(WebKit::WebSWOriginStore::add):
(WebKit::WebSWOriginStore::addAll):
(WebKit::WebSWOriginStore::remove):
* StorageProcess/ServiceWorker/WebSWOriginStore.h:
* UIProcess/VisitedLinkStore.cpp:
(WebKit::VisitedLinkStore::addVisitedLinkHash):
(WebKit::VisitedLinkStore::removeVisitedLinkHash):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (223689 => 223690)
--- trunk/Source/WebKit/ChangeLog 2017-10-19 15:52:37 UTC (rev 223689)
+++ trunk/Source/WebKit/ChangeLog 2017-10-19 16:10:47 UTC (rev 223690)
@@ -1,3 +1,45 @@
+2017-10-19 Chris Dumez <cdu...@apple.com>
+
+ http/tests/workers/service/basic-register.html is a flaky failure.
+ https://bugs.webkit.org/show_bug.cgi?id=178494
+ <rdar://problem/35065315>
+
+ Reviewed by Youenn Fablet.
+
+ In WebSWServerConnection::resolveJobInClient(), when a service worker is
+ registered, we:
+ 1. Add the origin to the WebSWOriginStore
+ 2. Send the IPC to the WebProcess to notify it that the registration succeeded.
+
+ The assumption was that step 1 would be synchronous and would therefore send
+ the shared memory handle to the WebProcess (if the SharedMemory was invalidated)
+ *before* step 2.
+
+ The issue is that step 1 was scheduling a zero-timer to schedule the addition.
+ As a result, there was a race and the WebContent process could check the
+ the WebSWOriginTable *after* being notified that a service worker was registered
+ but *before* it received the SharedMemory handle for the WebSWOriginTable. This
+ could lead to false negatives and was causing the layout test to be flaky.
+
+ To address the issue, step 1 is now synchronous.
+
+ * Shared/SharedStringHashStore.cpp:
+ (WebKit::SharedStringHashStore::SharedStringHashStore):
+ (WebKit::SharedStringHashStore::scheduleAddition):
+ (WebKit::SharedStringHashStore::scheduleRemoval):
+ (WebKit::SharedStringHashStore::contains):
+ (WebKit::SharedStringHashStore::flushPendingChanges):
+ (WebKit::SharedStringHashStore::processPendingOperations):
+ * Shared/SharedStringHashStore.h:
+ * StorageProcess/ServiceWorker/WebSWOriginStore.cpp:
+ (WebKit::WebSWOriginStore::add):
+ (WebKit::WebSWOriginStore::addAll):
+ (WebKit::WebSWOriginStore::remove):
+ * StorageProcess/ServiceWorker/WebSWOriginStore.h:
+ * UIProcess/VisitedLinkStore.cpp:
+ (WebKit::VisitedLinkStore::addVisitedLinkHash):
+ (WebKit::VisitedLinkStore::removeVisitedLinkHash):
+
2017-10-18 Ryosuke Niwa <rn...@webkit.org>
Don't expose raw HTML in pasteboard to the web content
Modified: trunk/Source/WebKit/Shared/SharedStringHashStore.cpp (223689 => 223690)
--- trunk/Source/WebKit/Shared/SharedStringHashStore.cpp 2017-10-19 15:52:37 UTC (rev 223689)
+++ trunk/Source/WebKit/Shared/SharedStringHashStore.cpp 2017-10-19 16:10:47 UTC (rev 223690)
@@ -65,7 +65,7 @@
SharedStringHashStore::SharedStringHashStore(Client& client)
: m_client(client)
- , m_pendingOperationsTimer(RunLoop::main(), this, &SharedStringHashStore::pendingOperationsTimerFired)
+ , m_pendingOperationsTimer(RunLoop::main(), this, &SharedStringHashStore::processPendingOperations)
{
}
@@ -74,7 +74,7 @@
return m_table.sharedMemory()->createHandle(handle, SharedMemory::Protection::ReadOnly);
}
-void SharedStringHashStore::add(SharedStringHash sharedStringHash)
+void SharedStringHashStore::scheduleAddition(SharedStringHash sharedStringHash)
{
m_pendingOperations.append({ Operation::Add, sharedStringHash });
@@ -82,7 +82,7 @@
m_pendingOperationsTimer.startOneShot(0_s);
}
-void SharedStringHashStore::remove(WebCore::SharedStringHash sharedStringHash)
+void SharedStringHashStore::scheduleRemoval(WebCore::SharedStringHash sharedStringHash)
{
m_pendingOperations.append({ Operation::Remove, sharedStringHash });
@@ -92,10 +92,7 @@
bool SharedStringHashStore::contains(WebCore::SharedStringHash sharedStringHash)
{
- if (m_pendingOperationsTimer.isActive()) {
- m_pendingOperationsTimer.stop();
- pendingOperationsTimerFired();
- }
+ flushPendingChanges();
return m_table.contains(sharedStringHash);
}
@@ -108,6 +105,15 @@
m_table.clear();
}
+void SharedStringHashStore::flushPendingChanges()
+{
+ if (!m_pendingOperationsTimer.isActive())
+ return;
+
+ m_pendingOperationsTimer.stop();
+ processPendingOperations();
+}
+
void SharedStringHashStore::resizeTable(unsigned newTableSize)
{
auto newTableMemory = SharedMemory::allocate(newTableSize * sizeof(SharedStringHash));
@@ -158,7 +164,7 @@
m_client.didInvalidateSharedMemory();
}
-void SharedStringHashStore::pendingOperationsTimerFired()
+void SharedStringHashStore::processPendingOperations()
{
unsigned currentTableSize = m_tableSize;
unsigned approximateNewHashCount = std::count_if(m_pendingOperations.begin(), m_pendingOperations.end(), [](auto& operation) {
Modified: trunk/Source/WebKit/Shared/SharedStringHashStore.h (223689 => 223690)
--- trunk/Source/WebKit/Shared/SharedStringHashStore.h 2017-10-19 15:52:37 UTC (rev 223689)
+++ trunk/Source/WebKit/Shared/SharedStringHashStore.h 2017-10-19 16:10:47 UTC (rev 223690)
@@ -46,16 +46,20 @@
SharedStringHashStore(Client&);
bool createSharedMemoryHandle(SharedMemory::Handle&);
- void add(WebCore::SharedStringHash);
- void remove(WebCore::SharedStringHash);
+
+ void scheduleAddition(WebCore::SharedStringHash);
+ void scheduleRemoval(WebCore::SharedStringHash);
+
bool contains(WebCore::SharedStringHash);
void clear();
bool isEmpty() const { return !m_keyCount; }
+ void flushPendingChanges();
+
private:
void resizeTable(unsigned newTableSize);
- void pendingOperationsTimerFired();
+ void processPendingOperations();
struct Operation {
enum Type { Add, Remove };
Modified: trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp (223689 => 223690)
--- trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp 2017-10-19 15:52:37 UTC (rev 223689)
+++ trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp 2017-10-19 16:10:47 UTC (rev 223690)
@@ -43,12 +43,21 @@
void WebSWOriginStore::add(const SecurityOrigin& origin)
{
- m_store.add(computeSharedStringHash(origin.toString()));
+ m_store.scheduleAddition(computeSharedStringHash(origin.toString()));
+ m_store.flushPendingChanges();
}
+void WebSWOriginStore::addAll(const Vector<SecurityOrigin>& origins)
+{
+ for (auto& origin : origins)
+ m_store.scheduleAddition(computeSharedStringHash(origin.toString()));
+ m_store.flushPendingChanges();
+}
+
void WebSWOriginStore::remove(const SecurityOrigin& origin)
{
- m_store.remove(computeSharedStringHash(origin.toString()));
+ m_store.scheduleRemoval(computeSharedStringHash(origin.toString()));
+ m_store.flushPendingChanges();
}
void WebSWOriginStore::clear()
Modified: trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.h (223689 => 223690)
--- trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.h 2017-10-19 15:52:37 UTC (rev 223689)
+++ trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.h 2017-10-19 16:10:47 UTC (rev 223690)
@@ -43,6 +43,7 @@
WebSWOriginStore();
void add(const WebCore::SecurityOrigin&);
+ void addAll(const Vector<WebCore::SecurityOrigin>&);
void remove(const WebCore::SecurityOrigin&);
void clear();
Modified: trunk/Source/WebKit/UIProcess/VisitedLinkStore.cpp (223689 => 223690)
--- trunk/Source/WebKit/UIProcess/VisitedLinkStore.cpp 2017-10-19 15:52:37 UTC (rev 223689)
+++ trunk/Source/WebKit/UIProcess/VisitedLinkStore.cpp 2017-10-19 16:10:47 UTC (rev 223690)
@@ -79,7 +79,7 @@
void VisitedLinkStore::addVisitedLinkHash(SharedStringHash linkHash)
{
- m_linkHashStore.add(linkHash);
+ m_linkHashStore.scheduleAddition(linkHash);
}
bool VisitedLinkStore::containsVisitedLinkHash(WebCore::SharedStringHash linkHash)
@@ -89,7 +89,7 @@
void VisitedLinkStore::removeVisitedLinkHash(WebCore::SharedStringHash linkHash)
{
- m_linkHashStore.remove(linkHash);
+ m_linkHashStore.scheduleRemoval(linkHash);
}
void VisitedLinkStore::removeAll()
