Changeset 260330 in webkit


Ignore:
Timestamp:
Apr 18, 2020 7:08:52 PM (4 years ago)
Author:
beidson@apple.com
Message:

Fix WebUserContentControllerProxy vs ContentWorld lifetime
​https://bugs.webkit.org/show_bug.cgi?id=210700

Reviewed by Alex Christensen.

Source/WebKit:

Covered by API test.

WebUserContentControllerProxy currently keeps all of its associated API::ContentWorlds alive via RefPtrs.

This is despite the fact that all of the associated WebScriptMessageHandlers, UserScripts, and
UserStyleSheets already keep their associated API::ContentWorlds alive.

It then decideds to tell WebProcesses to forget a content world after all of its clients are removed.

Unfortunately, content worlds are used for more than just content controller stuff. They're used for direct
JavaScript evaluation as well.

So a client could:

  • Add a script message handler in a content world.
  • Evaluate JavaScript in that content world, setting up some persistent state.
  • Remove the script message handler.
  • Find that their persistent state from the JavaScript evaluation is gone from that world, even though they still retain a usable handle to that world.

The only party who has any business managing the lifetime of an API::ContentWorld is the API::ContentWorld itself.

Making this change is:

1 - Nice cleanup
2 - Fixes the above mentioned bug

  • UIProcess/API/APIContentWorld.cpp:

(API::ContentWorld::worldForIdentifier):
(API::ContentWorld::ContentWorld):
(API::ContentWorld::sharedWorldWithName):
(API::ContentWorld::~ContentWorld):
(API::ContentWorld::addAssociatedUserContentControllerProxy):
(API::ContentWorld::userContentControllerProxyDestroyed):

  • UIProcess/API/APIContentWorld.h:
  • UIProcess/UserContent/WebUserContentControllerProxy.cpp:

(WebKit::WebUserContentControllerProxy::parameters const):
(WebKit::WebUserContentControllerProxy::addContentWorld):
(WebKit::WebUserContentControllerProxy::contentWorldDestroyed):
(WebKit::WebUserContentControllerProxy::addUserScript):
(WebKit::WebUserContentControllerProxy::removeUserScript):
(WebKit::WebUserContentControllerProxy::removeAllUserScripts):
(WebKit::WebUserContentControllerProxy::addUserStyleSheet):
(WebKit::WebUserContentControllerProxy::removeUserStyleSheet):
(WebKit::WebUserContentControllerProxy::removeAllUserStyleSheets):
(WebKit::WebUserContentControllerProxy::addUserScriptMessageHandler):
(WebKit::WebUserContentControllerProxy::removeUserMessageHandlerForName):
(WebKit::WebUserContentControllerProxy::removeAllUserMessageHandlers):
(WebKit::WebUserContentControllerProxy::addContentWorldUse): Deleted.
(WebKit::WebUserContentControllerProxy::shouldSendRemoveContentWorldsMessage): Deleted.
(WebKit::WebUserContentControllerProxy::removeContentWorldUses): Deleted.

  • UIProcess/UserContent/WebUserContentControllerProxy.h:

Tools:

  • TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEvaluateJavaScript.mm:

(-[DummyMessageHandler userContentController:didReceiveScriptMessage:]):
(TEST): Make sure removing a script message handler from a particular world

doesn't also destroy the other JavaScript contents of that world.

Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r260328 r260330  
     12020-04-18  Brady Eidson  <beidson@apple.com>
     2
     3        Fix WebUserContentControllerProxy vs ContentWorld lifetime
     4        https://bugs.webkit.org/show_bug.cgi?id=210700
     5
     6        Reviewed by Alex Christensen.
     7
     8        Covered by API test.
     9       
     10        WebUserContentControllerProxy currently keeps all of its associated API::ContentWorlds alive via RefPtrs.
     11       
     12        This is despite the fact that all of the associated WebScriptMessageHandlers, UserScripts, and
     13        UserStyleSheets already keep their associated API::ContentWorlds alive.
     14
     15        It then decideds to tell WebProcesses to forget a content world after all of its clients are removed.
     16       
     17        Unfortunately, content worlds are used for more than just content controller stuff. They're used for direct
     18        JavaScript evaluation as well.
     19       
     20        So a client could:
     21          - Add a script message handler in a content world.
     22          - Evaluate JavaScript in that content world, setting up some persistent state.
     23          - Remove the script message handler.
     24          - Find that their persistent state from the JavaScript evaluation is gone from that world, even though they
     25            still retain a usable handle to that world.
     26       
     27        The only party who has any business managing the lifetime of an API::ContentWorld is the API::ContentWorld itself.
     28       
     29        Making this change is:
     30          1 - Nice cleanup
     31          2 - Fixes the above mentioned bug
     32       
     33        * UIProcess/API/APIContentWorld.cpp:
     34        (API::ContentWorld::worldForIdentifier):
     35        (API::ContentWorld::ContentWorld):
     36        (API::ContentWorld::sharedWorldWithName):
     37        (API::ContentWorld::~ContentWorld):
     38        (API::ContentWorld::addAssociatedUserContentControllerProxy):
     39        (API::ContentWorld::userContentControllerProxyDestroyed):
     40        * UIProcess/API/APIContentWorld.h:
     41
     42        * UIProcess/UserContent/WebUserContentControllerProxy.cpp:
     43        (WebKit::WebUserContentControllerProxy::parameters const):
     44        (WebKit::WebUserContentControllerProxy::addContentWorld):
     45        (WebKit::WebUserContentControllerProxy::contentWorldDestroyed):
     46        (WebKit::WebUserContentControllerProxy::addUserScript):
     47        (WebKit::WebUserContentControllerProxy::removeUserScript):
     48        (WebKit::WebUserContentControllerProxy::removeAllUserScripts):
     49        (WebKit::WebUserContentControllerProxy::addUserStyleSheet):
     50        (WebKit::WebUserContentControllerProxy::removeUserStyleSheet):
     51        (WebKit::WebUserContentControllerProxy::removeAllUserStyleSheets):
     52        (WebKit::WebUserContentControllerProxy::addUserScriptMessageHandler):
     53        (WebKit::WebUserContentControllerProxy::removeUserMessageHandlerForName):
     54        (WebKit::WebUserContentControllerProxy::removeAllUserMessageHandlers):
     55        (WebKit::WebUserContentControllerProxy::addContentWorldUse): Deleted.
     56        (WebKit::WebUserContentControllerProxy::shouldSendRemoveContentWorldsMessage): Deleted.
     57        (WebKit::WebUserContentControllerProxy::removeContentWorldUses): Deleted.
     58        * UIProcess/UserContent/WebUserContentControllerProxy.h:
     59
    1602020-04-18  David Kilzer  <ddkilzer@apple.com>
    261

  • trunk/Source/WebKit/UIProcess/API/APIContentWorld.cpp

    r257552 r260330  
    2828
    2929#include "ContentWorldShared.h"
     30#include "WebUserContentControllerProxy.h"
    3031#include <wtf/HashMap.h>
    3132#include <wtf/text/StringHash.h>
    3233
    3334namespace API {
     35
     36static HashMap<WTF::String, ContentWorld*>& sharedWorldNameMap()
     37{
     38    static NeverDestroyed<HashMap<WTF::String, ContentWorld*>> sharedMap;
     39    return sharedMap;
     40}
     41
     42static HashMap<WebKit::ContentWorldIdentifier, ContentWorld*>& sharedWorldIdentifierMap()
     43{
     44    static NeverDestroyed<HashMap<WebKit::ContentWorldIdentifier, ContentWorld*>> sharedMap;
     45    return sharedMap;
     46}
     47
     48ContentWorld* ContentWorld::worldForIdentifier(WebKit::ContentWorldIdentifier identifer)
     49{
     50    return sharedWorldIdentifierMap().get(identifer);
     51}
    3452
    3553ContentWorld::ContentWorld(const WTF::String& name)
    … …  
    4563
    4664    m_identifier = WebKit::ContentWorldIdentifier::generate();
     65    auto addResult = sharedWorldIdentifierMap().add(m_identifier, this);
     66    ASSERT_UNUSED(addResult, addResult.isNewEntry);
    4767}
    4868
    49 static HashMap<WTF::String, ContentWorld*>& sharedWorldMap()
     69ContentWorld::ContentWorld(WebKit::ContentWorldIdentifier identifier)
     70    : m_identifier(identifier)
    5071{
    51     static HashMap<WTF::String, ContentWorld*>* sharedMap = new HashMap<WTF::String, ContentWorld*>;
    52     return *sharedMap;
     72    ASSERT(m_identifier == WebKit::pageContentWorldIdentifier());
    5373}
    5474
    5575Ref<ContentWorld> ContentWorld::sharedWorldWithName(const WTF::String& name)
    5676{
    57     auto result = sharedWorldMap().add(name, nullptr);
     77    auto result = sharedWorldNameMap().add(name, nullptr);
    5878    if (result.isNewEntry) {
    5979        result.iterator->value = new ContentWorld(name);
    … …  
    7898ContentWorld::~ContentWorld()
    7999{
    80     if (name().isNull())
    81         return;
     100    ASSERT(m_identifier != WebKit::pageContentWorldIdentifier());
    82101
    83     auto taken = sharedWorldMap().take(name());
    84     ASSERT_UNUSED(taken, taken == this);
     102    auto result = sharedWorldIdentifierMap().take(m_identifier);
     103    ASSERT_UNUSED(result, result == this || m_identifier == WebKit::pageContentWorldIdentifier());
     104
     105    if (!name().isNull()) {
     106        auto taken = sharedWorldNameMap().take(name());
     107        ASSERT_UNUSED(taken, taken == this);
     108    }
     109
     110    for (auto proxy : m_associatedContentControllerProxies)
     111        proxy->contentWorldDestroyed(*this);
     112}
     113
     114void ContentWorld::addAssociatedUserContentControllerProxy(WebKit::WebUserContentControllerProxy& proxy)
     115{
     116    auto addResult = m_associatedContentControllerProxies.add(&proxy);
     117    ASSERT_UNUSED(addResult, addResult.isNewEntry);
     118}
     119
     120void ContentWorld::userContentControllerProxyDestroyed(WebKit::WebUserContentControllerProxy& proxy)
     121{
     122    bool removed = m_associatedContentControllerProxies.remove(&proxy);
     123    ASSERT_UNUSED(removed, removed);
    85124}
    86125

  • trunk/Source/WebKit/UIProcess/API/APIContentWorld.h

    r257552 r260330  
    2828#include "APIObject.h"
    2929#include "ContentWorldShared.h"
     30#include <wtf/HashSet.h>
    3031#include <wtf/text/WTFString.h>
     32
     33namespace WebKit {
     34class WebUserContentControllerProxy;
     35}
    3136
    3237namespace API {
    … …  
    3439class ContentWorld final : public API::ObjectImpl<API::Object::Type::ContentWorld> {
    3540public:
     41    static ContentWorld* worldForIdentifier(WebKit::ContentWorldIdentifier);
    3642    static Ref<ContentWorld> sharedWorldWithName(const WTF::String&);
    3743    static ContentWorld& pageContentWorld();
    … …  
    4450    std::pair<WebKit::ContentWorldIdentifier, WTF::String> worldData() const { return { m_identifier, m_name }; }
    4551
     52    void addAssociatedUserContentControllerProxy(WebKit::WebUserContentControllerProxy&);
     53    void userContentControllerProxyDestroyed(WebKit::WebUserContentControllerProxy&);
     54
    4655private:
    4756    explicit ContentWorld(const WTF::String&);
    48     explicit ContentWorld(WebKit::ContentWorldIdentifier identifier)
    49         : m_identifier(identifier) { }
     57    explicit ContentWorld(WebKit::ContentWorldIdentifier);
    5058
    5159    WebKit::ContentWorldIdentifier m_identifier;
    5260    WTF::String m_name;
     61    HashSet<WebKit::WebUserContentControllerProxy*> m_associatedContentControllerProxies;
    5362};
    5463

  • trunk/Source/WebKit/UIProcess/UserContent/WebUserContentControllerProxy.cpp

    r259324 r260330  
    7474WebUserContentControllerProxy::~WebUserContentControllerProxy()
    7575{
     76    for (const auto& identifier : m_associatedContentWorlds) {
     77        auto* world = API::ContentWorld::worldForIdentifier(identifier);
     78        RELEASE_ASSERT(world);
     79        world->userContentControllerProxyDestroyed(*this);
     80    }
     81   
    7682    webUserContentControllerProxies().remove(m_identifier);
    7783    for (auto& process : m_processes) {
    … …  
    111117    parameters.identifier = identifier();
    112118   
    113     for (const auto& world : m_contentWorlds)
    114         parameters.userContentWorlds.append(world.key->worldData());
     119    ASSERT(parameters.userContentWorlds.isEmpty());
     120    for (const auto& identifier : m_associatedContentWorlds) {
     121        auto* world = API::ContentWorld::worldForIdentifier(identifier);
     122        RELEASE_ASSERT(world);
     123        parameters.userContentWorlds.append(world->worldData());
     124    }
    115125
    116126    for (auto userScript : m_userScripts->elementsOfType<API::UserScript>())
    … …  
    150160}
    151161
    152 void WebUserContentControllerProxy::addContentWorldUse(API::ContentWorld& world)
    153 {
    154     if (&world == &API::ContentWorld::pageContentWorld())
    155         return;
    156 
    157     auto addResult = m_contentWorlds.add(&world);
    158     if (addResult.isNewEntry) {
    159         for (auto& process : m_processes)
    160             process.send(Messages::WebUserContentController::AddContentWorlds({ world.worldData() }), identifier());
    161     }
    162 }
    163 
    164 bool WebUserContentControllerProxy::shouldSendRemoveContentWorldsMessage(API::ContentWorld& world, unsigned numberOfUsesToRemove)
    165 {
    166     if (&world == &API::ContentWorld::pageContentWorld())
    167         return false;
    168 
    169     auto it = m_contentWorlds.find(&world);
    170     for (unsigned i = 0; i < numberOfUsesToRemove; ++i) {
    171         if (m_contentWorlds.remove(it)) {
    172             ASSERT(i == (numberOfUsesToRemove - 1));
    173             return true;
    174         }
    175     }
    176    
    177     return false;
    178 }
    179 
    180 void WebUserContentControllerProxy::removeContentWorldUses(API::ContentWorld& world, unsigned numberOfUsesToRemove)
    181 {
    182     if (shouldSendRemoveContentWorldsMessage(world, numberOfUsesToRemove)) {
    183         for (auto& process : m_processes)
    184             process.send(Messages::WebUserContentController::RemoveContentWorlds({ world.identifier() }), identifier());
    185     }
    186 }
    187 
    188 void WebUserContentControllerProxy::removeContentWorldUses(HashCountedSet<RefPtr<API::ContentWorld>>& worlds)
    189 {
    190     Vector<ContentWorldIdentifier> worldsToRemove;
    191     for (auto& worldUsePair : worlds) {
    192         if (shouldSendRemoveContentWorldsMessage(*worldUsePair.key.get(), worldUsePair.value))
    193             worldsToRemove.append(worldUsePair.key->identifier());
    194     }
    195 
    196     for (auto& process : m_processes)
    197         process.send(Messages::WebUserContentController::RemoveContentWorlds(worldsToRemove), identifier());
     162void WebUserContentControllerProxy::addContentWorld(API::ContentWorld& world)
     163{
     164    if (world.identifier() == pageContentWorldIdentifier())
     165        return;
     166
     167    auto addResult = m_associatedContentWorlds.add(world.identifier());
     168    if (!addResult.isNewEntry)
     169        return;
     170
     171    world.addAssociatedUserContentControllerProxy(*this);
     172    for (auto& process : m_processes)
     173        process.send(Messages::WebUserContentController::AddContentWorlds({ world.worldData() }), identifier());
     174}
     175
     176void WebUserContentControllerProxy::contentWorldDestroyed(API::ContentWorld& world)
     177{
     178    bool result = m_associatedContentWorlds.remove(world.identifier());
     179    ASSERT_UNUSED(result, result);
     180
     181    for (auto& process : m_processes)
     182        process.send(Messages::WebUserContentController::RemoveContentWorlds({ world.identifier() }), identifier());
    198183}
    199184
    … …  
    202187    Ref<API::ContentWorld> world = userScript.contentWorld();
    203188
    204     addContentWorldUse(world.get());
     189    addContentWorld(world.get());
    205190
    206191    m_userScripts->elements().append(&userScript);
    … …  
    218203
    219204    m_userScripts->elements().removeAll(&userScript);
    220 
    221     removeContentWorldUses(world.get(), 1);
    222205}
    223206
    … …  
    227210        process.send(Messages::WebUserContentController::RemoveAllUserScripts({ world.identifier() }), identifier());
    228211
    229     unsigned userScriptsRemoved = m_userScripts->removeAllOfTypeMatching<API::UserScript>([&](const auto& userScript) {
     212    m_userScripts->removeAllOfTypeMatching<API::UserScript>([&](const auto& userScript) {
    230213        return &userScript->contentWorld() == &world;
    231214    });
    232 
    233     removeContentWorldUses(world, userScriptsRemoved);
    234215}
    235216
    … …  
    249230
    250231    m_userScripts->elements().clear();
    251 
    252     removeContentWorldUses(worlds);
    253232}
    254233
    … …  
    257236    Ref<API::ContentWorld> world = userStyleSheet.contentWorld();
    258237
    259     addContentWorldUse(world.get());
     238    addContentWorld(world.get());
    260239
    261240    m_userStyleSheets->elements().append(&userStyleSheet);
    … …  
    273252
    274253    m_userStyleSheets->elements().removeAll(&userStyleSheet);
    275 
    276     removeContentWorldUses(world.get(), 1);
    277254}
    278255
    … …  
    282259        process.send(Messages::WebUserContentController::RemoveAllUserStyleSheets({ world.identifier() }), identifier());
    283260
    284     unsigned userStyleSheetsRemoved = m_userStyleSheets->removeAllOfTypeMatching<API::UserStyleSheet>([&](const auto& userStyleSheet) {
     261    m_userStyleSheets->removeAllOfTypeMatching<API::UserStyleSheet>([&](const auto& userStyleSheet) {
    285262        return &userStyleSheet->contentWorld() == &world;
    286263    });
    287 
    288     removeContentWorldUses(world, userStyleSheetsRemoved);
    289264}
    290265
    … …  
    304279
    305280    m_userStyleSheets->elements().clear();
    306 
    307     removeContentWorldUses(worlds);
    308281}
    309282
    … …  
    317290    }
    318291
    319     addContentWorldUse(world);
     292    addContentWorld(world);
    320293
    321294    m_scriptMessageHandlers.add(handler.identifier(), &handler);
    … …  
    336309            m_scriptMessageHandlers.remove(it);
    337310
    338             removeContentWorldUses(world, 1);
    339311            return;
    340312        }
    … …  
    355327        return false;
    356328    });
    357 
    358     removeContentWorldUses(world, numberRemoved);
    359329}
    360330

  • trunk/Source/WebKit/UIProcess/UserContent/WebUserContentControllerProxy.h

    r259307 r260330  
    2727
    2828#include "APIObject.h"
     29#include "ContentWorldShared.h"
    2930#include "MessageReceiver.h"
    3031#include "UserContentControllerIdentifier.h"
    … …  
    9495    void removeAllUserStyleSheets();
    9596
    96     void removeAllUserContent(API::ContentWorld&);
    97 
    9897    // Returns false if there was a name conflict.
    9998    bool addUserScriptMessageHandler(WebScriptMessageHandler&);
    … …  
    114113    UserContentControllerIdentifier identifier() const { return m_identifier; }
    115114
     115    void contentWorldDestroyed(API::ContentWorld&);
     116
    116117private:
    117118    // IPC::MessageReceiver.
    … …  
    120121    void didPostMessage(IPC::Connection&, WebPageProxyIdentifier, FrameInfoData&&, uint64_t messageHandlerID, const IPC::DataReference&);
    121122
    122     void addContentWorldUse(API::ContentWorld&);
    123     void removeContentWorldUses(API::ContentWorld&, unsigned numberOfUsesToRemove);
    124     void removeContentWorldUses(HashCountedSet<RefPtr<API::ContentWorld>>&);
    125     bool shouldSendRemoveContentWorldsMessage(API::ContentWorld&, unsigned numberOfUsesToRemove);
     123    void addContentWorld(API::ContentWorld&);
    126124
    127125    UserContentControllerIdentifier m_identifier;
    … …  
    130128    Ref<API::Array> m_userStyleSheets;
    131129    HashMap<uint64_t, RefPtr<WebScriptMessageHandler>> m_scriptMessageHandlers;
    132     HashCountedSet<RefPtr<API::ContentWorld>> m_contentWorlds;
     130    HashSet<ContentWorldIdentifier> m_associatedContentWorlds;
    133131
    134132#if ENABLE(CONTENT_EXTENSIONS)

  • trunk/Tools/ChangeLog

    r260327 r260330  
     12020-04-18  Brady Eidson  <beidson@apple.com>
     2
     3        Fix WebUserContentControllerProxy vs ContentWorld lifetime
     4        https://bugs.webkit.org/show_bug.cgi?id=210700
     5
     6        Reviewed by Alex Christensen.
     7
     8        * TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEvaluateJavaScript.mm:
     9        (-[DummyMessageHandler userContentController:didReceiveScriptMessage:]):
     10        (TEST): Make sure removing a script message handler from a particular world
     11          doesn't also destroy the other JavaScript contents of that world.
     12
    1132020-04-18  Daniel Bates  <dabates@apple.com>
    214

  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEvaluateJavaScript.mm

    r257892 r260330  
    3535#import "TestWKWebView.h"
    3636#import "WKWebViewConfigurationExtras.h"
    37 #import <WebKit/WKWebViewConfigurationPrivate.h>
    38 #import <WebKit/WKContentWorld.h>
     37#import <WebKit/WKContentWorldPrivate.h>
    3938#import <WebKit/WKErrorPrivate.h>
    4039#import <WebKit/WKPreferencesPrivate.h>
    4140#import <WebKit/WKPreferencesRef.h>
     41#import <WebKit/WKUserContentControllerPrivate.h>
     42#import <WebKit/WKWebViewConfigurationPrivate.h>
    4243#import <WebKit/WKWebViewPrivate.h>
    4344#import <WebKit/_WKFrameTreeNode.h>
    … …  
    134135}
    135136
     137@interface DummyMessageHandler : NSObject <WKScriptMessageHandler>
     138@end
     139
     140@implementation DummyMessageHandler
     141- (void)userContentController:(WKUserContentController *)userContentController didReceiveScriptMessage:(WKScriptMessage *)message
     142{
     143}
     144@end
     145
    136146TEST(WKWebView, EvaluateJavaScriptInWorlds)
    137147{
    … …  
    186196    isDone = false;
    187197
    188     // Set a varibale value in a named world.
     198    // Add a scriptMessageHandler in a named world.
    189199    RetainPtr<WKContentWorld> namedWorld = [WKContentWorld worldWithName:@"NamedWorld"];
     200    id handler = [[[DummyMessageHandler alloc] init] autorelease];
     201    [webView.get().configuration.userContentController _addScriptMessageHandler:handler name:@"testHandlerName" userContentWorld:namedWorld.get()._userContentWorld];
     202
     203    // Set a variable value in that named world.
    190204    [webView evaluateJavaScript:@"var bar = 'baz'" inContentWorld:namedWorld.get() completionHandler:^(id result, NSError *error) {
    191205        EXPECT_NULL(result);
    … …  
    196210    isDone = false;
    197211
    198     // Set a global variable value in a named world via a function call.
     212    // Set a global variable value in that named world via a function call.
    199213    [webView callAsyncJavaScript:@"window.baz = 'bat'" arguments:nil inContentWorld:namedWorld.get() completionHandler:^(id result, NSError *error) {
    200214        EXPECT_NULL(result);
    … …  
    206220    isDone = false;
    207221
    208     // Verify they are there in that named world.
     222    // Remove the dummy message handler
     223    [webView.get().configuration.userContentController _removeScriptMessageHandlerForName:@"testHandlerName" userContentWorld:namedWorld.get()._userContentWorld];
     224
     225    // Verify the variables we set are there in that named world.
    209226    [webView evaluateJavaScript:@"bar" inContentWorld:namedWorld.get() completionHandler:^(id result, NSError *error) {
    210227        EXPECT_TRUE([result isKindOfClass:[NSString class]]);
Note: See TracChangeset for help on using the changeset viewer.