⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 280676 in webkit


Ignore:
Timestamp:
Aug 4, 2021, 7:46:31 PM (5 years ago)
Author:
Devin Rousso
Message:

REGRESSION (r280374): ASSERTION FAILED: Completion handler should not be called more than once under WebCore::MediaControlsContextMenuProvider::contextMenuItemSelected
​https://bugs.webkit.org/show_bug.cgi?id=228725
<rdar://problem/81437221>

Reviewed by Eric Carlson.

The contextmenu system used by (modern) media controls are a bit wonky in that it has to
support both macOS and iOS, which use wildly different mechanisms. The former has distinct
methods for handling when a contextmenu item is selected vs when the menu is dismissed (at
least as of r280374). The latter has a single method that handles both. Additionally, the
(modern) media controls JS expects the following from showMediaControlsContextMenu:

  1. showMediaControlsContextMenu will only return true if the contextmenu will be shown
  2. the callback provided to showMediaControlsContextMenu will always/only be invoked when the contextmenu is dismissed (regardless of whether an item is selected)
  3. if an item is selected, the logic for that will be handled by the MediaControlsHost

This patch primarily addresses #2, but also slightly adjusts the code to fix #1. It does #1
by moving the call that saves the callback further down. On iOS, #2 already works. On macOS,
it does #2 by changing from CompletionHandler to Function, allowing it to be called more
than once, with the understanding that the JS callback will not be invoked more than once.
This way, macOS can match the behavior of iOS by eagerly invoking the JS callback when a
contextmenu item is selected without waiting for the menu to actually dismiss, while still
handling the contextmenu being dismissed without an item being selected (and also not having
to worry about whether the CompletionHandler has already been invoked).

  • Modules/mediacontrols/MediaControlsHost.h:
  • Modules/mediacontrols/MediaControlsHost.cpp:

(WebCore::MediaControlsContextMenuProvider::create):
(WebCore::MediaControlsContextMenuProvider::MediaControlsContextMenuProvider):
(WebCore::MediaControlsContextMenuProvider::didDismissContextMenu):
(WebCore::MediaControlsContextMenuProvider::contextMenuCleared):
(WebCore::MediaControlsHost::showMediaControlsContextMenu):

Location:
trunk/Source/WebCore
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r280671 r280676  
     12021-08-04  Devin Rousso  <drousso@apple.com>
     2
     3        REGRESSION (r280374): ASSERTION FAILED: Completion handler should not be called more than once under WebCore::MediaControlsContextMenuProvider::contextMenuItemSelected
     4        https://bugs.webkit.org/show_bug.cgi?id=228725
     5        <rdar://problem/81437221>
     6
     7        Reviewed by Eric Carlson.
     8
     9        The contextmenu system used by (modern) media controls are a bit wonky in that it has to
     10        support both macOS and iOS, which use wildly different mechanisms. The former has distinct
     11        methods for handling when a contextmenu item is selected vs when the menu is dismissed (at
     12        least as of r280374). The latter has a single method that handles both. Additionally, the
     13        (modern) media controls JS expects the following from `showMediaControlsContextMenu`:
     14         1. `showMediaControlsContextMenu` will only `return true` if the contextmenu will be shown
     15         2. the callback provided to `showMediaControlsContextMenu` will always/only be invoked when
     16            the contextmenu is dismissed (regardless of whether an item is selected)
     17         3. if an item is selected, the logic for that will be handled by the `MediaControlsHost`
     18        This patch primarily addresses #2, but also slightly adjusts the code to fix #1. It does #1
     19        by moving the call that saves the callback further down. On iOS, #2 already works. On macOS,
     20        it does #2 by changing from `CompletionHandler` to `Function`, allowing it to be called more
     21        than once, with the understanding that the JS callback will not be invoked more than once.
     22        This way, macOS can match the behavior of iOS by eagerly invoking the JS callback when a
     23        contextmenu item is selected without waiting for the menu to actually dismiss, while still
     24        handling the contextmenu being dismissed without an item being selected (and also not having
     25        to worry about whether the `CompletionHandler` has already been invoked).
     26
     27        * Modules/mediacontrols/MediaControlsHost.h:
     28        * Modules/mediacontrols/MediaControlsHost.cpp:
     29        (WebCore::MediaControlsContextMenuProvider::create):
     30        (WebCore::MediaControlsContextMenuProvider::MediaControlsContextMenuProvider):
     31        (WebCore::MediaControlsContextMenuProvider::didDismissContextMenu):
     32        (WebCore::MediaControlsContextMenuProvider::contextMenuCleared):
     33        (WebCore::MediaControlsHost::showMediaControlsContextMenu):
     34
    1352021-08-04  Dana Estra  <destra@apple.com>
    236

  • trunk/Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp

    r280374 r280676  
    6262#include "VoidCallback.h"
    6363#include <JavaScriptCore/JSCJSValueInlines.h>
    64 #include <wtf/CompletionHandler.h>
     64#include <wtf/Function.h>
    6565#include <wtf/JSONValues.h>
    6666#include <wtf/Scope.h>
    … …  
    366366class MediaControlsContextMenuProvider final : public ContextMenuProvider {
    367367public:
    368     static Ref<MediaControlsContextMenuProvider> create(Vector<ContextMenuItem>&& items, CompletionHandler<void(uint64_t)>&& callback)
     368    static Ref<MediaControlsContextMenuProvider> create(Vector<ContextMenuItem>&& items, Function<void(uint64_t)>&& callback)
    369369    {
    370370        return adoptRef(*new MediaControlsContextMenuProvider(WTFMove(items), WTFMove(callback)));
    … …  
    372372
    373373private:
    374     MediaControlsContextMenuProvider(Vector<ContextMenuItem>&& items, CompletionHandler<void(uint64_t)>&& callback)
     374    MediaControlsContextMenuProvider(Vector<ContextMenuItem>&& items, Function<void(uint64_t)>&& callback)
    375375        : m_items(WTFMove(items))
    376376        , m_callback(WTFMove(callback))
    … …  
    391391    void didDismissContextMenu() override
    392392    {
    393         if (m_callback)
     393        if (!m_didDismiss) {
     394            m_didDismiss = true;
    394395            m_callback(ContextMenuItemTagNoAction);
     396        }
    395397    }
    396398
    … …  
    402404    void contextMenuCleared() override
    403405    {
    404         if (m_callback)
    405             m_callback(ContextMenuItemTagNoAction);
     406        didDismissContextMenu();
    406407        m_items.clear();
    407408    }
    … …  
    413414
    414415    Vector<ContextMenuItem> m_items;
    415     CompletionHandler<void(uint64_t)> m_callback;
     416    Function<void(uint64_t)> m_callback;
     417    bool m_didDismiss { false };
    416418};
    417419
    … …  
    464466    if (m_showMediaControlsContextMenuCallback)
    465467        return false;
    466 
    467     m_showMediaControlsContextMenuCallback = WTFMove(callback);
    468 
    469     auto invokeCallbackAtScopeExit = makeScopeExit([&, protectedThis = makeRef(*this)] {
    470         if (m_showMediaControlsContextMenuCallback)
    471             std::exchange(m_showMediaControlsContextMenuCallback, nullptr)->handleEvent();
    472     });
    473468
    474469    if (!m_mediaElement)
    … …  
    656651    ASSERT(!idMap.isEmpty());
    657652
    658     auto handleItemSelected = [weakMediaElement = makeWeakPtr(mediaElement), idMap = WTFMove(idMap), invokeCallbackAtScopeExit = WTFMove(invokeCallbackAtScopeExit)] (MenuItemIdentifier selectedItemID) {
     653    m_showMediaControlsContextMenuCallback = WTFMove(callback);
     654
     655    auto handleItemSelected = [weakThis = makeWeakPtr(this), idMap = WTFMove(idMap)] (MenuItemIdentifier selectedItemID) {
     656        if (!weakThis)
     657            return;
     658        Ref strongThis = *weakThis;
     659
     660        auto invokeCallbackAtScopeExit = makeScopeExit([strongThis] {
     661            if (auto showMediaControlsContextMenuCallback = std::exchange(strongThis->m_showMediaControlsContextMenuCallback, nullptr))
     662                showMediaControlsContextMenuCallback->handleEvent();
     663        });
     664
    659665        if (selectedItemID == invalidMenuItemIdentifier)
    660666            return;
    661667
    662         if (!weakMediaElement)
     668        if (!strongThis->m_mediaElement)
    663669            return;
    664 
    665         auto& mediaElement = *weakMediaElement;
     670        auto& mediaElement = *strongThis->m_mediaElement;
    666671
    667672        UserGestureIndicator gestureIndicator(ProcessingUserGesture, &mediaElement.document());

  • trunk/Source/WebCore/Modules/mediacontrols/MediaControlsHost.h

    r279309 r280676  
    4646class VoidCallback;
    4747
    48 class MediaControlsHost : public RefCounted<MediaControlsHost> {
     48class MediaControlsHost final : public RefCounted<MediaControlsHost>, public CanMakeWeakPtr<MediaControlsHost> {
    4949    WTF_MAKE_FAST_ALLOCATED(MediaControlsHost);
    5050public:
Note: See TracChangeset for help on using the changeset viewer.