Changeset 253213 in webkit


Ignore:
Timestamp:
Dec 6, 2019 1:05:12 PM (4 years ago)
Author:
Chris Dumez
Message:

Prevent synchronous XHR in beforeunload / unload event handlers
​https://bugs.webkit.org/show_bug.cgi?id=204912
<rdar://problem/57676394>

Reviewed by Darin Adler.

Source/WebCore:

Prevent synchronous XHR in beforeunload / unload event handlers. They are terrible for performance
and the Beacon API (or Fetch keepalive) are more efficient & supported alternatives.

In particular, this would cause hangs when trying to navigate away from a site or when closing
attempt, which would result in terrible user experience.

Chrome and Edge have expressed public support for this. Chrome has actually been testing this behavior
for a while now:
​https://www.chromestatus.com/feature/4664843055398912

I added this new behavior behind an experimental feature flag, enabled by default.

Tests: http/tests/xmlhttprequest/sync-xhr-in-beforeunload.html

http/tests/xmlhttprequest/sync-xhr-in-unload.html

  • loader/DocumentThreadableLoader.cpp:

(WebCore::DocumentThreadableLoader::DocumentThreadableLoader):

  • loader/FrameLoader.cpp:

(WebCore::PageLevelForbidScope::PageLevelForbidScope):
(WebCore::ForbidPromptsScope::ForbidPromptsScope):
(WebCore::ForbidPromptsScope::~ForbidPromptsScope):
(WebCore::ForbidSynchronousLoadsScope::ForbidSynchronousLoadsScope):
(WebCore::ForbidSynchronousLoadsScope::~ForbidSynchronousLoadsScope):
(WebCore::FrameLoader::dispatchUnloadEvents):
(WebCore::FrameLoader::dispatchBeforeUnloadEvent):

  • page/Page.cpp:

(WebCore::Page::forbidSynchronousLoads):
(WebCore::Page::allowSynchronousLoads):
(WebCore::Page::areSynchronousLoadsAllowed):

  • page/Page.h:

LayoutTests:

Add layout test coverage.

  • http/tests/xmlhttprequest/resources/sync-xhr-in-beforeunload-window.html: Added.
  • http/tests/xmlhttprequest/resources/sync-xhr-in-unload-window.html: Added.
  • http/tests/xmlhttprequest/sync-xhr-in-beforeunload-expected.txt: Added.
  • http/tests/xmlhttprequest/sync-xhr-in-beforeunload.html: Added.
  • http/tests/xmlhttprequest/sync-xhr-in-unload-expected.txt: Added.
  • http/tests/xmlhttprequest/sync-xhr-in-unload.html: Added.
Location:
trunk
Files:
7 added
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r253210 r253213  
     12019-12-06  Chris Dumez  <cdumez@apple.com>
     2
     3        Prevent synchronous XHR in beforeunload / unload event handlers
     4        https://bugs.webkit.org/show_bug.cgi?id=204912
     5        <rdar://problem/57676394>
     6
     7        Reviewed by Darin Adler.
     8
     9        Add layout test coverage.
     10
     11        * http/tests/xmlhttprequest/resources/sync-xhr-in-beforeunload-window.html: Added.
     12        * http/tests/xmlhttprequest/resources/sync-xhr-in-unload-window.html: Added.
     13        * http/tests/xmlhttprequest/sync-xhr-in-beforeunload-expected.txt: Added.
     14        * http/tests/xmlhttprequest/sync-xhr-in-beforeunload.html: Added.
     15        * http/tests/xmlhttprequest/sync-xhr-in-unload-expected.txt: Added.
     16        * http/tests/xmlhttprequest/sync-xhr-in-unload.html: Added.
     17
    1182019-12-06  Antti Koivisto  <antti@apple.com>
    219

  • trunk/Source/WebCore/ChangeLog

    r253210 r253213  
     12019-12-06  Chris Dumez  <cdumez@apple.com>
     2
     3        Prevent synchronous XHR in beforeunload / unload event handlers
     4        https://bugs.webkit.org/show_bug.cgi?id=204912
     5        <rdar://problem/57676394>
     6
     7        Reviewed by Darin Adler.
     8
     9        Prevent synchronous XHR in beforeunload / unload event handlers. They are terrible for performance
     10        and the Beacon API (or Fetch keepalive) are more efficient & supported alternatives.
     11
     12        In particular, this would cause hangs when trying to navigate away from a site or when closing
     13        attempt, which would result in terrible user experience.
     14
     15        Chrome and Edge have expressed public support for this. Chrome has actually been testing this behavior
     16        for a while now:
     17        https://www.chromestatus.com/feature/4664843055398912
     18
     19        I added this new behavior behind an experimental feature flag, enabled by default.
     20
     21        Tests: http/tests/xmlhttprequest/sync-xhr-in-beforeunload.html
     22               http/tests/xmlhttprequest/sync-xhr-in-unload.html
     23
     24        * loader/DocumentThreadableLoader.cpp:
     25        (WebCore::DocumentThreadableLoader::DocumentThreadableLoader):
     26        * loader/FrameLoader.cpp:
     27        (WebCore::PageLevelForbidScope::PageLevelForbidScope):
     28        (WebCore::ForbidPromptsScope::ForbidPromptsScope):
     29        (WebCore::ForbidPromptsScope::~ForbidPromptsScope):
     30        (WebCore::ForbidSynchronousLoadsScope::ForbidSynchronousLoadsScope):
     31        (WebCore::ForbidSynchronousLoadsScope::~ForbidSynchronousLoadsScope):
     32        (WebCore::FrameLoader::dispatchUnloadEvents):
     33        (WebCore::FrameLoader::dispatchBeforeUnloadEvent):
     34        * page/Page.cpp:
     35        (WebCore::Page::forbidSynchronousLoads):
     36        (WebCore::Page::allowSynchronousLoads):
     37        (WebCore::Page::areSynchronousLoadsAllowed):
     38        * page/Page.h:
     39
    1402019-12-06  Antti Koivisto  <antti@apple.com>
    241

  • trunk/Source/WebCore/loader/DocumentThreadableLoader.cpp

    r251582 r253213  
    5656#include "RuntimeEnabledFeatures.h"
    5757#include "SecurityOrigin.h"
     58#include "Settings.h"
    5859#include "SharedBuffer.h"
    5960#include "SubresourceIntegrity.h"
    … …  
    131132    // Setting a referrer header is only supported in the async code path.
    132133    ASSERT(m_async || m_referrer.isEmpty());
     134
     135    if (document.settings().disallowSyncXHRDuringPageDismissalEnabled() && !m_async && (!document.page() || !document.page()->areSynchronousLoadsAllowed())) {
     136        logErrorAndFail(ResourceError(errorDomainWebKitInternal, 0, request.url(), "Synchronous loads are not allowed at this time"));
     137        return;
     138    }
    133139
    134140    // Referrer and Origin headers should be set after the preflight if any.

  • trunk/Source/WebCore/loader/FrameLoader.cpp

    r253090 r253213  
    211211}
    212212
    213 struct ForbidPromptsScope {
    214     ForbidPromptsScope(Page* page) : m_page(page)
     213class PageLevelForbidScope {
     214protected:
     215    explicit PageLevelForbidScope(Page* page)
     216        : m_page(makeWeakPtr(page))
    215217    {
    216         if (!m_page)
    217             return;
    218         m_page->forbidPrompts();
     218    }
     219
     220    ~PageLevelForbidScope() = default;
     221
     222    WeakPtr<Page> m_page;
     223};
     224
     225struct ForbidPromptsScope : public PageLevelForbidScope {
     226    explicit ForbidPromptsScope(Page* page)
     227        : PageLevelForbidScope(page)
     228    {
     229        if (m_page)
     230            m_page->forbidPrompts();
    219231    }
    220232
    221233    ~ForbidPromptsScope()
    222234    {
    223         if (!m_page)
    224             return;
    225         m_page->allowPrompts();
    226     }
    227 
    228     Page* m_page;
     235        if (m_page)
     236            m_page->allowPrompts();
     237    }
     238};
     239
     240struct ForbidSynchronousLoadsScope : public PageLevelForbidScope {
     241    explicit ForbidSynchronousLoadsScope(Page* page)
     242        : PageLevelForbidScope(page)
     243    {
     244        if (m_page)
     245            m_page->forbidSynchronousLoads();
     246    }
     247
     248    ~ForbidSynchronousLoadsScope()
     249    {
     250        if (m_page)
     251            m_page->allowSynchronousLoads();
     252    }
    229253};
    230254
    … …  
    32993323    // We store the frame's page in a local variable because the frame might get detached inside dispatchEvent.
    33003324    ForbidPromptsScope forbidPrompts(m_frame.page());
     3325    ForbidSynchronousLoadsScope forbidSynchronousLoads(m_frame.page());
    33013326    IgnoreOpensDuringUnloadCountIncrementer ignoreOpensDuringUnloadCountIncrementer(m_frame.document());
    33023327
    … …  
    33793404        SetForScope<PageDismissalType> change(m_pageDismissalEventBeingDispatched, PageDismissalType::BeforeUnload);
    33803405        ForbidPromptsScope forbidPrompts(m_frame.page());
     3406        ForbidSynchronousLoadsScope forbidSynchronousLoads(m_frame.page());
    33813407        domWindow->dispatchEvent(beforeUnloadEvent, domWindow->document());
    33823408    }

  • trunk/Source/WebCore/page/Page.cpp

    r253182 r253213  
    24192419}
    24202420
     2421void Page::forbidSynchronousLoads()
     2422{
     2423    ++m_forbidSynchronousLoadsDepth;
     2424}
     2425
     2426void Page::allowSynchronousLoads()
     2427{
     2428    ASSERT(m_forbidSynchronousLoadsDepth);
     2429    --m_forbidSynchronousLoadsDepth;
     2430}
     2431
     2432bool Page::areSynchronousLoadsAllowed()
     2433{
     2434    return !m_forbidSynchronousLoadsDepth;
     2435}
     2436
    24212437void Page::logNavigation(const Navigation& navigation)
    24222438{

  • trunk/Source/WebCore/page/Page.h

    r253051 r253213  
    585585    bool arePromptsAllowed();
    586586
     587    void forbidSynchronousLoads();
     588    void allowSynchronousLoads();
     589    bool areSynchronousLoadsAllowed();
     590
    587591    void mainFrameLoadStarted(const URL&, FrameLoadType);
    588592
    … …  
    919923    unsigned m_lastSpatialNavigationCandidatesCount { 0 };
    920924    unsigned m_forbidPromptsDepth { 0 };
     925    unsigned m_forbidSynchronousLoadsDepth { 0 };
    921926
    922927    Ref<SocketProvider> m_socketProvider;

  • trunk/Source/WebCore/page/Settings.yaml

    r253185 r253213  
    5555  initial: 0
    5656
     57disallowSyncXHRDuringPageDismissalEnabled:
     58  type: bool
     59  initial: true
     60
    5761# Allow clients concerned with memory consumption to set a quota on session storage
    5862# since the memory used won't be released until the Page is destroyed.

  • trunk/Source/WebKit/Shared/WebPreferences.yaml

    r253185 r253213  
    1818  condition: ENABLE(DEVICE_ORIENTATION)
    1919  webcoreName: deviceOrientationPermissionAPIEnabled
     20
     21DisallowSyncXHRDuringPageDismissalEnabled:
     22  type: bool
     23  defaultValue: true
     24  humanReadableName: "Disallow sync XHR during page dismissal"
     25  humanReadableDescription: "Disallow synchronous XMLHttpRequest during page dismissal"
     26  category: experimental
    2027
    2128JavaScriptEnabled:
Note: See TracChangeset for help on using the changeset viewer.