Bug 131203

Summary: Crash in plugin process
Product: WebKit Reporter: Stephanie Lewis <slewis>
Component: Plug-insAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, commit-queue, ddkilzer, slewis
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch ggaren: review+

Description Stephanie Lewis 2014-04-03 18:36:48 PDT
Created attachment 228573 [details]
patch

<rdar://problem/16479432>
Comment 1 WebKit Commit Bot 2014-04-03 18:37:55 PDT
Attachment 228573 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 Geoffrey Garen 2014-04-03 19:04:43 PDT
Comment on attachment 228573 [details]
patch

r=me

You should explain in your ChangeLog that the old code crashed because it the C++ scope would delete its pluginController before the call to pluginDidBecomeHidden.

Would it be better for this function to take a pluginInstanceID argument, instead of a raw (dangerous) PluginControllerProxy pointer?
Comment 3 Stephanie Lewis 2014-04-07 20:51:44 PDT
committed http://trac.webkit.org/projects/webkit/changeset/166907
Comment 4 Darin Adler 2014-04-07 23:15:37 PDT
Comment on attachment 228573 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228573&action=review

> Source/WebKit2/PluginProcess/WebProcessConnection.cpp:96
> +    unsigned pluginInstanceID = pluginController->pluginInstanceID();
>      {
> -        ASSERT(m_pluginControllers.contains(pluginController->pluginInstanceID()));
> +        ASSERT(m_pluginControllers.contains(pluginInstanceID));
>  
> -        std::unique_ptr<PluginControllerProxy> pluginControllerUniquePtr = m_pluginControllers.take(pluginController->pluginInstanceID());
> +        std::unique_ptr<PluginControllerProxy> pluginControllerUniquePtr = m_pluginControllers.take(pluginInstanceID);
>          ASSERT(pluginControllerUniquePtr.get() == pluginController);
>      }
>  
> -    pluginDidBecomeHidden(pluginController->pluginInstanceID());
> +    pluginDidBecomeHidden(pluginInstanceID);

Not really sure why the original code uses take. I would write this like this:

    unsigned instanceID = pluginController->pluginInstanceID();

    ASSERT(m_pluginControllers.get(pluginInstanceID) == pluginController);
    m_pluginControllers.remove(instanceID);

    pluginDidBecomeHidden(pluginInstanceID);