Changeset 223690 in webkit


Ignore:
Timestamp:
Oct 19, 2017 9:10:47 AM (6 years ago)
Author:
Chris Dumez
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):

Location:
trunk/Source/WebKit
Files:
6 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebKit/ChangeLog

    r223678 r223690  
     12017-10-19  Chris Dumez  <cdumez@apple.com>
     2
     3        http/tests/workers/service/basic-register.html is a flaky failure.
     4        https://bugs.webkit.org/show_bug.cgi?id=178494
     5        <rdar://problem/35065315>
     6
     7        Reviewed by Youenn Fablet.
     8
     9        In WebSWServerConnection::resolveJobInClient(), when a service worker is
     10        registered, we:
     11        1. Add the origin to the WebSWOriginStore
     12        2. Send the IPC to the WebProcess to notify it that the registration succeeded.
     13
     14        The assumption was that step 1 would be synchronous and would therefore send
     15        the shared memory handle to the WebProcess (if the SharedMemory was invalidated)
     16        *before* step 2.
     17
     18        The issue is that step 1 was scheduling a zero-timer to schedule the addition.
     19        As a result, there was a race and the WebContent process could check the
     20        the WebSWOriginTable *after* being notified that a service worker was registered
     21        but *before* it received the SharedMemory handle for the WebSWOriginTable. This
     22        could lead to false negatives and was causing the layout test to be flaky.
     23
     24        To address the issue, step 1 is now synchronous.
     25
     26        * Shared/SharedStringHashStore.cpp:
     27        (WebKit::SharedStringHashStore::SharedStringHashStore):
     28        (WebKit::SharedStringHashStore::scheduleAddition):
     29        (WebKit::SharedStringHashStore::scheduleRemoval):
     30        (WebKit::SharedStringHashStore::contains):
     31        (WebKit::SharedStringHashStore::flushPendingChanges):
     32        (WebKit::SharedStringHashStore::processPendingOperations):
     33        * Shared/SharedStringHashStore.h:
     34        * StorageProcess/ServiceWorker/WebSWOriginStore.cpp:
     35        (WebKit::WebSWOriginStore::add):
     36        (WebKit::WebSWOriginStore::addAll):
     37        (WebKit::WebSWOriginStore::remove):
     38        * StorageProcess/ServiceWorker/WebSWOriginStore.h:
     39        * UIProcess/VisitedLinkStore.cpp:
     40        (WebKit::VisitedLinkStore::addVisitedLinkHash):
     41        (WebKit::VisitedLinkStore::removeVisitedLinkHash):
     42
    1432017-10-18  Ryosuke Niwa  <rniwa@webkit.org>
    244
  • trunk/Source/WebKit/Shared/SharedStringHashStore.cpp

    r222820 r223690  
    6666SharedStringHashStore::SharedStringHashStore(Client& client)
    6767    : m_client(client)
    68     , m_pendingOperationsTimer(RunLoop::main(), this, &SharedStringHashStore::pendingOperationsTimerFired)
     68    , m_pendingOperationsTimer(RunLoop::main(), this, &SharedStringHashStore::processPendingOperations)
    6969{
    7070}
     
    7575}
    7676
    77 void SharedStringHashStore::add(SharedStringHash sharedStringHash)
     77void SharedStringHashStore::scheduleAddition(SharedStringHash sharedStringHash)
    7878{
    7979    m_pendingOperations.append({ Operation::Add, sharedStringHash });
     
    8383}
    8484
    85 void SharedStringHashStore::remove(WebCore::SharedStringHash sharedStringHash)
     85void SharedStringHashStore::scheduleRemoval(WebCore::SharedStringHash sharedStringHash)
    8686{
    8787    m_pendingOperations.append({ Operation::Remove, sharedStringHash });
     
    9393bool SharedStringHashStore::contains(WebCore::SharedStringHash sharedStringHash)
    9494{
    95     if (m_pendingOperationsTimer.isActive()) {
    96         m_pendingOperationsTimer.stop();
    97         pendingOperationsTimerFired();
    98     }
     95    flushPendingChanges();
    9996    return m_table.contains(sharedStringHash);
    10097}
     
    107104    m_tableSize = 0;
    108105    m_table.clear();
     106}
     107
     108void SharedStringHashStore::flushPendingChanges()
     109{
     110    if (!m_pendingOperationsTimer.isActive())
     111        return;
     112
     113    m_pendingOperationsTimer.stop();
     114    processPendingOperations();
    109115}
    110116
     
    159165}
    160166
    161 void SharedStringHashStore::pendingOperationsTimerFired()
     167void SharedStringHashStore::processPendingOperations()
    162168{
    163169    unsigned currentTableSize = m_tableSize;
  • trunk/Source/WebKit/Shared/SharedStringHashStore.h

    r223608 r223690  
    4747
    4848    bool createSharedMemoryHandle(SharedMemory::Handle&);
    49     void add(WebCore::SharedStringHash);
    50     void remove(WebCore::SharedStringHash);
     49
     50    void scheduleAddition(WebCore::SharedStringHash);
     51    void scheduleRemoval(WebCore::SharedStringHash);
     52
    5153    bool contains(WebCore::SharedStringHash);
    5254    void clear();
     
    5456    bool isEmpty() const { return !m_keyCount; }
    5557
     58    void flushPendingChanges();
     59
    5660private:
    5761    void resizeTable(unsigned newTableSize);
    58     void pendingOperationsTimerFired();
     62    void processPendingOperations();
    5963
    6064    struct Operation {
  • trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp

    r223608 r223690  
    4444void WebSWOriginStore::add(const SecurityOrigin& origin)
    4545{
    46     m_store.add(computeSharedStringHash(origin.toString()));
     46    m_store.scheduleAddition(computeSharedStringHash(origin.toString()));
     47    m_store.flushPendingChanges();
     48}
     49
     50void WebSWOriginStore::addAll(const Vector<SecurityOrigin>& origins)
     51{
     52    for (auto& origin : origins)
     53        m_store.scheduleAddition(computeSharedStringHash(origin.toString()));
     54    m_store.flushPendingChanges();
    4755}
    4856
    4957void WebSWOriginStore::remove(const SecurityOrigin& origin)
    5058{
    51     m_store.remove(computeSharedStringHash(origin.toString()));
     59    m_store.scheduleRemoval(computeSharedStringHash(origin.toString()));
     60    m_store.flushPendingChanges();
    5261}
    5362
  • trunk/Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.h

    r223608 r223690  
    4444
    4545    void add(const WebCore::SecurityOrigin&);
     46    void addAll(const Vector<WebCore::SecurityOrigin>&);
    4647    void remove(const WebCore::SecurityOrigin&);
    4748    void clear();
  • trunk/Source/WebKit/UIProcess/VisitedLinkStore.cpp

    r222820 r223690  
    8080void VisitedLinkStore::addVisitedLinkHash(SharedStringHash linkHash)
    8181{
    82     m_linkHashStore.add(linkHash);
     82    m_linkHashStore.scheduleAddition(linkHash);
    8383}
    8484
     
    9090void VisitedLinkStore::removeVisitedLinkHash(WebCore::SharedStringHash linkHash)
    9191{
    92     m_linkHashStore.remove(linkHash);
     92    m_linkHashStore.scheduleRemoval(linkHash);
    9393}
    9494
Note: See TracChangeset for help on using the changeset viewer.