Bug 88028 – [chromium] Provide access to the WebPlugin created by the helper plugin widget

• Component: Media
• Status: RESOLVED FIXED

[David Dorwin]

While bug 87399 enables instantiation of a plugin, the embedder cannot (directly) communicate with that plugin instance. Therefore, we need to extend the the functionality in that issue to provide access to the WebPlugin instance that is created when the helper plugin widget is created.

[David Dorwin]

Created attachment 145180 [details]
Patch for initial feedback

[David Dorwin]

This initial patch demonstrates that we can get the WebPlugin pointer, but I'd prefer a more elegant solution.

My initial idea was to decorate the WebFrameClient passed to WebHelperPluginImpl::initializeFrame() such that the createPlugin() call could be intercepted. The problem is that WebFrameClient is a large interface and, more importantly, has default implementations for all functions. As a result, the decorator could easily be missing a forwarding method without anyone noticing.

Suggestions welcome!

[David Dorwin]

One option would be to create an EmptyWebFrameClient or BaseWebFrameClient class that has all the default implementations and have RenderViewImpl and the test classes inherit from it. The decorator class would inherit from WebFrameClient. This would allow us to continue making Changes to WebKit before the corresponding Chromium changes while still forcing any changes to WebFrameClient to be accounted for in the decorator.

Reviewers, WDYT? I'm going to wait for feedback before I spend time implementing this large decorator.

[Adam Barth]

Comment on attachment 145180 [details]
Patch for initial feedback

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

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:122
> +    WebPlugin* m_plugin;

I feel a bit lost in the trees reading this patch.  What keeps m_plugin align?  I would have expected us to have something like a RefPtr to the plugin here.

[David Dorwin]

Comment on attachment 145180 [details]
Patch for initial feedback

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

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:122
>> +    WebPlugin* m_plugin;
> 
> I feel a bit lost in the trees reading this patch.  What keeps m_plugin align?  I would have expected us to have something like a RefPtr to the plugin here.

s/align/alive/ ?

WebPlugin is not RefCounted currently. The lifetime should be fine because the WebPlugin should exist as long as the element exists, and the element will exist as long as the WebHelperPluginImpl widget exists. That said, I would prefer a better guarantee.

FWIW, this pointer gets returned to the embedder code, and in the Chrome case, it is just used to obtain a PluginInstance, which is refcounted. See the code in the next comment on this bug.

In the current implementation, m_plugin is just a place to temporarily store data from a "callback" before it gets returned up the stack. All of this happens within WebMediaPlayerClientImpl::createHelperPlugin() and its callees. We could actually move it down into WebViewImpl::createHelperPlugin().

With this knowledge, one option is to:
 * document that the pointer createHelperPlugin() is only valid in the current call stack
 * have WebViewImpl::createHelperPlugin() return the WebPlugin (eliminating getPlugin(), which is currently called immediately after it)
 * clear m_plugin before init(), which is used by createHelperPlugin(), returns.

[David Dorwin]

Here is some example code that shows how this might be used in Chromium code:
      WebKit::WebPlugin* plugin = GetClient()->createHelperPlugin("application/x-ppapi-example", frame);
      webkit::ppapi::WebPluginImpl* ppapi_plugin = static_cast<webkit::ppapi::WebPluginImpl*>(plugin);
      scoped_refptr<webkit::ppapi::PluginInstance> instance = ppapi_plugin->instance();
      instance->DoFoo();

[David Dorwin]

Created attachment 145385 [details]
Shows possible decorator solution and alternate m_plugin lifetime. Does not compile.

[David Dorwin]

Created attachment 145391 [details]
Shows possible decorator solution and alternate m_plugin lifetime. Would compile with addition of WebFrameClientDecorator.

This is a better solution than the first patch. It uses the decorator pattern discussed in Comments #2 and #3 (the decorator is not yet implemented) and somewhat improves the WebPlugin lifetime management as discussed in comment #5

[Adam Barth]

Comment on attachment 145391 [details]
Shows possible decorator solution and alternate m_plugin lifetime. Would compile with addition of WebFrameClientDecorator.

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

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:105
> +        WebPlugin* plugin =
> +            WebFrameClientDecorator::createPlugin(frame, params);

There's no 80 col limit in WebKit, so feel free to use more horizontal space.

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:142
> +        delete webFrameClient;

We generally frown upon calling delete manually.  Is there really no object that's a good owner for this object?

[Adam Barth]

Comment on attachment 145391 [details]
Shows possible decorator solution and alternate m_plugin lifetime. Would compile with addition of WebFrameClientDecorator.

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

> Source/WebKit/chromium/ChangeLog:9
> +        A WebPlugin is created when the document created by createHelperPlugin() is layed out.
> +        Return it to the caller so that it can interact with the plugin instance.

It's unfortunate that we don't create the plugin until we do layout.  That's not really a problem with this patch.  It's an architectural problem with WebKit that we've been slowly chipping away at fixing for a while.

> Source/WebKit/chromium/public/WebMediaPlayerClient.h:81
> +    // The returned pointer is only guaranteed to be valid in this call stack.
> +    virtual WebPlugin* createHelperPlugin(const WebString& pluginType, WebFrame*) = 0;

That's sort of an odd thing to say.  There's some weird ownership stuff going on in this patch.

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:106
> +        m_helperPlugin->setPlugin(plugin);

So, we stash off a pointer to the plugin, but according to the comment in createPlugin, plugin will self-destruct as soon as this call stack unwinds.

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:118
> +    WebHelperPluginImpl* m_helperPlugin;

From my previous read though this patch, this is going to end up being a backpointer.  I guess that means that WebHelperPluginImpl owns this object.

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:142
>> +        delete webFrameClient;
> 
> We generally frown upon calling delete manually.  Is there really no object that's a good owner for this object?

Why don't we just hold an OwnPtr to the HelperPluginWebFrameClient ?  They we don't need any of this fancy business in the destructor.

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:160
> +    WebPlugin* plugin = m_plugin;
> +    m_plugin = 0; // Clear the pointer since it is not guaranteed to be valid.

So, this was all a hack to return this pointer to this program point (bypassing some intermediate stack frames) ?

Oh, because there's a big call stack related to layout before createPlugin actually gets call.  I'm starting to understand what's going on here.

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:181
> +    WebFrameClient* decoratedClient = new HelperPluginWebFrameClient(client, this);

Again, WebKit frowns on "naked new".  You should call adoptPtr immediately after calling new.  That will result in you having an OwnPtr member variable in this class which will handle all the memory management for you.

[Adam Barth]

Ok.  I understand what this patch is trying to do now.  I'd like to recommend a different approach.

Rather than trying to hook createPlugin to find the plugin in the helper frame, we should just look for the plugin in the DOM after it gets created.  Can't we just initialize everything, and then after things are initialized use getElementsByTagName("object") or something similar to go find the plugin?  That's way more direct than what's in the current patch.

[David Dorwin]

Comment on attachment 145391 [details]
Shows possible decorator solution and alternate m_plugin lifetime. Would compile with addition of WebFrameClientDecorator.

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

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:106
>> +        m_helperPlugin->setPlugin(plugin);
> 
> So, we stash off a pointer to the plugin, but according to the comment in createPlugin, plugin will self-destruct as soon as this call stack unwinds.

This is called within that stack, and it gets used before the stack unwinds (line 159).

The plugin doesn't self-destruct, it's just not guaranteed to be valid. The reason for the comment in WebMediaPlayerClient.h is that I don't want the embedder holding the pointer for an extended period. (In reality, there shouldn't be any problems as long as it's not used after closeHelperPlugin() is called.)

This could be solved by making WebPlugin RefCounted - any idea what the impact would be?

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:118
>> +    WebHelperPluginImpl* m_helperPlugin;
> 
> From my previous read though this patch, this is going to end up being a backpointer.  I guess that means that WebHelperPluginImpl owns this object.

Yes, is there something to do here?

>>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:142
>>> +        delete webFrameClient;
>> 
>> We generally frown upon calling delete manually.  Is there really no object that's a good owner for this object?
> 
> Why don't we just hold an OwnPtr to the HelperPluginWebFrameClient ?  They we don't need any of this fancy business in the destructor.

Yes, I think I can handle all this in the declaration.

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:160
>> +    m_plugin = 0; // Clear the pointer since it is not guaranteed to be valid.
> 
> So, this was all a hack to return this pointer to this program point (bypassing some intermediate stack frames) ?
> 
> Oh, because there's a big call stack related to layout before createPlugin actually gets call.  I'm starting to understand what's going on here.

Yes. WebPlugin is converted to a PluginContainer, which is returned as a Widget. All information about the plugin is lost in the call stack from this function to createPlugin().

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:181
>> +    WebFrameClient* decoratedClient = new HelperPluginWebFrameClient(client, this);
> 
> Again, WebKit frowns on "naked new".  You should call adoptPtr immediately after calling new.  That will result in you having an OwnPtr member variable in this class which will handle all the memory management for you.

Will fix.

[Adam Barth]

> This could be solved by making WebPlugin RefCounted - any idea what the impact would be?

That doesn't seem to be necessary.

> >> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:118
> >> +    WebHelperPluginImpl* m_helperPlugin;
> > 
> > From my previous read though this patch, this is going to end up being a backpointer.  I guess that means that WebHelperPluginImpl owns this object.
> 
> Yes, is there something to do here?

Nope.  I'm just talking through the patch out loud so you can tell me if I'm off my rocker.

> > Oh, because there's a big call stack related to layout before createPlugin actually gets call.  I'm starting to understand what's going on here.
> 
> Yes. WebPlugin is converted to a PluginContainer, which is returned as a Widget. All information about the plugin is lost in the call stack from this function to createPlugin().

What do you think about the approach of retrieving the WebPlugin from the DOM rather than hooking createPlugin ?

[David Dorwin]

(In reply to comment #11)
> Ok.  I understand what this patch is trying to do now.  I'd like to recommend a different approach.
> 
> Rather than trying to hook createPlugin to find the plugin in the helper frame, we should just look for the plugin in the DOM after it gets created.  Can't we just initialize everything, and then after things are initialized use getElementsByTagName("object") or something similar to go find the plugin?  That's way more direct than what's in the current patch.

Do you mean to do this in WebHelperPluginImpl or from outside it? How do you suggest we get the WebPlugin from a Node*?

I think it might look something like this, which is maybe not as horrible as I thought it would be.
  Node* node = NodeList[0];
  Widget* widget = static_cast<HTMLPlugInElement*>(node)->pluginWidget();
  WebPlugin* plugin = <WebPluginContainerImpl*>(widget)->plugin();


I think we still have the lifetime issue to address.
FWIW, ~WebPluginContainerImpl() calls m_webPlugin->destroy(), which results in a deferred deletion. Thus, the WebPlugin is valid until the <object> element is destroyed, which should only happen after closeHelperPlugin() is called to destroy the offscreen page. If we wanted to, we could define the lifetime as it was in the first patch:
// The returned pointer is valid until closeHelperPlugin() is called.

[Adam Barth]

> Do you mean to do this in WebHelperPluginImpl or from outside it? How do you suggest we get the WebPlugin from a Node*?
> 
> I think it might look something like this, which is maybe not as horrible as I thought it would be.
>   Node* node = NodeList[0];
>   Widget* widget = static_cast<HTMLPlugInElement*>(node)->pluginWidget();
>   WebPlugin* plugin = <WebPluginContainerImpl*>(widget)->plugin();

That looks reasonable.  We can add some ASSERTs regarding the tagName of the node.

> I think we still have the lifetime issue to address.
>
> FWIW, ~WebPluginContainerImpl() calls m_webPlugin->destroy(), which results in a deferred deletion. Thus, the WebPlugin is valid until the <object> element is destroyed, which should only happen after closeHelperPlugin() is called to destroy the offscreen page. If we wanted to, we could define the lifetime as it was in the first patch:
>
> // The returned pointer is valid until closeHelperPlugin() is called.

That seems like a reasonable statement about the lifetime.

[David Dorwin]

Quick update: I have the getElementsByTagName() solution working in general, but I need to sort out some corner cases before uploading.

[David Dorwin]

Created attachment 146173 [details]
Use getElementsByTagName() to get the WebPlugin

Implemented using the getElementsByTagName() per the suggestion in comment #11

[David Dorwin]

I tested the following scenarios (using Chromium changes that exercise this code):
 * Plugin present and enabled
 * Plugin not present (file not found)
 * Plugin present and not enabled
 * Enabling plugin between attempts to create helper plugin
   - This one is interesting because the disabled plugin is a placeholder, and enabling the plugin would destroy the placeholder WebPlugin. Thus, we do not return placeholders.
 * Disabling plugin after creating helper plugin

I also found and fixed a bug where Chromium's ExtensionHelper would hang on to the WebFrame even after it was destroyed by calling frameDetached() in ~WebFrameImpl().

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 146173 [details]
Use getElementsByTagName() to get the WebPlugin

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

> Source/WebKit/chromium/public/WebHelperPlugin.h:48
> +    // The returned pointer may be 0 even if initialization was successful.

We should have a blank line above this line.

> Source/WebKit/chromium/public/WebPlugin.h:143
> +    virtual bool isPpapi() { return false; }

I'm not 100% sure if this function is correct.  My understand was that WebKit should be ignorant of PPAPI.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2062
> +    // Tell the client that this object is no longer valid.
> +    if (client())
> +        client()->frameDetached(this);

I'm not sure I understand this part of your patch.  Is there a general bug in the WebFrame lifecycle that we haven't noticed until now?  Is there something strange about the helper plugin that's triggering the need for this extra lifecycle call?

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:155
> +    RefPtr<NodeList> objectElements = m_page->mainFrame()->document()->getElementsByTagName("object");

Rather than the string literal "object", you can use objectTag from HTMLNames.h

Do we want to return 0 if this returns an empty list?  That could happen, for example, if the embedder calls this function too early.

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:158
> +    WebCore::Widget* widget = reinterpret_cast<HTMLPlugInElement*>(node)->pluginWidget();

reinterpret_cast -> static_cast

Do you want to add an ASSERT(node->hasTagName(objectTag)) ?  I guess that's pretty redundant with getElementsByTagName.

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:161
> +    WebPlugin* plugin = reinterpret_cast<WebPluginContainerImpl*>(widget)->plugin();

reinterpret_cast -> static_cast

[James Robinson]

Comment on attachment 146173 [details]
Use getElementsByTagName() to get the WebPlugin

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

>> Source/WebKit/chromium/public/WebPlugin.h:143
>> +    virtual bool isPpapi() { return false; }
> 
> I'm not 100% sure if this function is correct.  My understand was that WebKit should be ignorant of PPAPI.

This is definitely wrong.  I see no callers of this function, why are you adding it?

[David Dorwin]

Comment on attachment 146173 [details]
Use getElementsByTagName() to get the WebPlugin

I made all the requested changes as noted below.

The remaining issues are:
 * Alternatives, if any, to isPpapi().
 * WebFrame lifetime issue.

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

>> Source/WebKit/chromium/public/WebHelperPlugin.h:48
>> +    // The returned pointer may be 0 even if initialization was successful.
> 
> We should have a blank line above this line.

Done.

>>> Source/WebKit/chromium/public/WebPlugin.h:143
>>> +    virtual bool isPpapi() { return false; }
>> 
>> I'm not 100% sure if this function is correct.  My understand was that WebKit should be ignorant of PPAPI.
> 
> This is definitely wrong.  I see no callers of this function, why are you adding it?

Okay. (FWIW, this is the Chromium-specific part of WebKit.)

This function would be used by Chromium code to verify the plugin type before casting a WebKit::WebPlugin* to a webkit::ppapi::WebPluginImpl*. See the example code in comment #6.
Since the Chromium code knows what plugin it asked for, this is really just an additional safety measure. If plugins are either placeholders, NPAPI, or PPAPI, then the WebPlugin should always be a ppapi::WebPluginImpl because it's not NPAPI and placeholders are not returned.

I was following the isMediaControlElement() pattern, though media controls are obviously common to all WebKit ports. Any other suggestions options for verifying the type?

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2062
>> +        client()->frameDetached(this);
> 
> I'm not sure I understand this part of your patch.  Is there a general bug in the WebFrame lifecycle that we haven't noticed until now?  Is there something strange about the helper plugin that's triggering the need for this extra lifecycle call?

This was briefly mentioned in comment #18, but I'll provide more details here. I believe this code exposes a scenario where the lifetime isn't properly managed. I'm open to other suggestions on how to resolve this.

The initialization of the new WebFrame causes clients to be informed via DidCreateDataSource(). One of those is ExtensionHelper, which creates a UserScriptScheduler that holds a raw pointer to the WebFrame. (This is the only code I could find where such a pointer was _stored_.) Ideally, this would be some type of refptr, but we don't use RefPtrs in the chromium/public APIs. While holding the raw pointer seems like a bad idea, at least the object holding the pointer is deleted if frameDetached() is called.

When the plugin cannot be instantiated (i.e. it is disabled), getPlugin() returns 0 and WebMediaPlayerClientImpl::createHelperPlugin() destroys the WebHelperPlugin in the same call stack (after DidCreateDataSource() is called). This causes the WebFrame to be destroyed. Since we don't inform the clients, the extensions code is still holding a raw pointer to the deleted WebFrame.

The extensions code also apparently schedules a task to run on the main thread and inject extension scripts. This task gets executed after the current task, which called createHelperPlugin(), completes. It tries to use the stored pointer and crashes.

There are two possibly unique things about this code:
1) A WebFrame is created and destroyed in the same call stack. In all other cases, script-injecting task would probably run between creation and destruction.
2) The WebFrame is destroyed without ever being detached. If it was detached, frameDetached() would be called before the frame is deleted. I don't know under what conditions a frame is detached before being destroyed.

Maybe the problem above could be resolved by posting the WebHelperPlugin destruction. However, it's not clear to me that the script-injecting task is the only one that will run and nothing guarantees that other clients won't hold such pointers in the future.

A workaround would be to disable extensions for this frame (something that might be useful anyway), but I didn't find a way to do that.

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:155
>> +    RefPtr<NodeList> objectElements = m_page->mainFrame()->document()->getElementsByTagName("object");
> 
> Rather than the string literal "object", you can use objectTag from HTMLNames.h
> 
> Do we want to return 0 if this returns an empty list?  That could happen, for example, if the embedder calls this function too early.

Done.

Done. I left an assert since this is a programming error but will remove it if preferred.

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:158
>> +    WebCore::Widget* widget = reinterpret_cast<HTMLPlugInElement*>(node)->pluginWidget();
> 
> reinterpret_cast -> static_cast
> 
> Do you want to add an ASSERT(node->hasTagName(objectTag)) ?  I guess that's pretty redundant with getElementsByTagName.

cast changed.

Yes, it's pretty redundant, but I added it anyway. isPluginElement() would be more helpful, but it didn't seem worth adding to Node.

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:161
>> +    WebPlugin* plugin = reinterpret_cast<WebPluginContainerImpl*>(widget)->plugin();
> 
> reinterpret_cast -> static_cast

Done.

[James Robinson]

(In reply to comment #22)
> >>> Source/WebKit/chromium/public/WebPlugin.h:143
> >>> +    virtual bool isPpapi() { return false; }
> >> 
> >> I'm not 100% sure if this function is correct.  My understand was that WebKit should be ignorant of PPAPI.
> > 
> > This is definitely wrong.  I see no callers of this function, why are you adding it?
> 
> Okay. (FWIW, this is the Chromium-specific part of WebKit.)
> 
> This function would be used by Chromium code to verify the plugin type before casting a WebKit::WebPlugin* to a webkit::ppapi::WebPluginImpl*. See the example code in comment #6.
> Since the Chromium code knows what plugin it asked for, this is really just an additional safety measure. If plugins are either placeholders, NPAPI, or PPAPI, then the WebPlugin should always be a ppapi::WebPluginImpl because it's not NPAPI and placeholders are not returned.
> 

This is a embedder-specific concern, it shouldn't infect the API.

[David Dorwin]

Created attachment 146417 [details]
Review feedback

[Adam Barth]

> which creates a UserScriptScheduler that holds a raw pointer to the WebFrame

I suspect that this is the bug.  We shouldn't be holding raw pointers to WebFrame.  There's nothing keeping them alive.  I suspect that this code can crash in other situations when things get scheduled on the event loop in a slightly different order.

[Adam Barth]

Are you calling webView->close() ?  It looks like that should call start the teardown process.

[Adam Barth]

(If you are calling close(), we might want to try to understand why the signal isn't getting routed all the way back out to the client.)

[David Dorwin]

(In reply to comment #23)
> (In reply to comment #22)
> > >>> Source/WebKit/chromium/public/WebPlugin.h:143
> > >>> +    virtual bool isPpapi() { return false; }
> > >> 
> > >> I'm not 100% sure if this function is correct.  My understand was that WebKit should be ignorant of PPAPI.
> > > 
> > > This is definitely wrong.  I see no callers of this function, why are you adding it?
> > 
> > Okay. (FWIW, this is the Chromium-specific part of WebKit.)
> > 
> > This function would be used by Chromium code to verify the plugin type before casting a WebKit::WebPlugin* to a webkit::ppapi::WebPluginImpl*. See the example code in comment #6.
> > Since the Chromium code knows what plugin it asked for, this is really just an additional safety measure. If plugins are either placeholders, NPAPI, or PPAPI, then the WebPlugin should always be a ppapi::WebPluginImpl because it's not NPAPI and placeholders are not returned.
> > 
> 
> This is a embedder-specific concern, it shouldn't infect the API.

Makes sense. The correct way to solve this would probably be to have a common WebPluginImpl base in Chromium that has such a function and have NPAPI, PPAPI, etc. inherit from it. Today, they all inherit directly from WebPlugin.

In any case, It has been removed from WebPlugin

[David Dorwin]

(In reply to comment #25)
> > which creates a UserScriptScheduler that holds a raw pointer to the WebFrame
> 
> I suspect that this is the bug.  We shouldn't be holding raw pointers to WebFrame.  There's nothing keeping them alive.  I suspect that this code can crash in other situations when things get scheduled on the event loop in a slightly different order.

Is this a bug regardless of the discussion below? If so, I'll file a Chromium bug.

(In reply to comment #26)
> Are you calling webView->close() ?  It looks like that should call start the teardown process.

No. WebHelperPluginImpl, like WebPagePopupImpl, uses the hosting page's WebView, so we can't close it.

They could perform the important step(s) from close() in their own teardown code. Specifically, calling m_page->mainFrame()->loader()->frameDetached() from their close() functions.

That seems like the right fix. Should a frame always be detached before ~WebFrameImpl() runs? If so, is there any thing we can assert on to prevent such mistakes in the future?

[David Dorwin]

Comment on attachment 146173 [details]
Use getElementsByTagName() to get the WebPlugin

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

>>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2062
>>> +        client()->frameDetached(this);
>> 
>> I'm not sure I understand this part of your patch.  Is there a general bug in the WebFrame lifecycle that we haven't noticed until now?  Is there something strange about the helper plugin that's triggering the need for this extra lifecycle call?
> 
> This was briefly mentioned in comment #18, but I'll provide more details here. I believe this code exposes a scenario where the lifetime isn't properly managed. I'm open to other suggestions on how to resolve this.
> 
> The initialization of the new WebFrame causes clients to be informed via DidCreateDataSource(). One of those is ExtensionHelper, which creates a UserScriptScheduler that holds a raw pointer to the WebFrame. (This is the only code I could find where such a pointer was _stored_.) Ideally, this would be some type of refptr, but we don't use RefPtrs in the chromium/public APIs. While holding the raw pointer seems like a bad idea, at least the object holding the pointer is deleted if frameDetached() is called.
> 
> When the plugin cannot be instantiated (i.e. it is disabled), getPlugin() returns 0 and WebMediaPlayerClientImpl::createHelperPlugin() destroys the WebHelperPlugin in the same call stack (after DidCreateDataSource() is called). This causes the WebFrame to be destroyed. Since we don't inform the clients, the extensions code is still holding a raw pointer to the deleted WebFrame.
> 
> The extensions code also apparently schedules a task to run on the main thread and inject extension scripts. This task gets executed after the current task, which called createHelperPlugin(), completes. It tries to use the stored pointer and crashes.
> 
> There are two possibly unique things about this code:
> 1) A WebFrame is created and destroyed in the same call stack. In all other cases, script-injecting task would probably run between creation and destruction.
> 2) The WebFrame is destroyed without ever being detached. If it was detached, frameDetached() would be called before the frame is deleted. I don't know under what conditions a frame is detached before being destroyed.
> 
> Maybe the problem above could be resolved by posting the WebHelperPlugin destruction. However, it's not clear to me that the script-injecting task is the only one that will run and nothing guarantees that other clients won't hold such pointers in the future.
> 
> A workaround would be to disable extensions for this frame (something that might be useful anyway), but I didn't find a way to do that.

Slight correction: The WebFrame is NOT created and destroyed in the same call stack because closeWidgetSoon() causes close() to be called asynchronously. It looks like the extension task runs as the result of an idle timeout, so it may just be a timing issue as to whether that specific task runs between creation of the HelperPlugin's frame and and close().

Nevertheless, we have a better solution as discussed in the comments.

[David Dorwin]

Created attachment 146442 [details]
Call frameDetached() from WebHelperPluginImpl::close()

[Adam Barth]

Comment on attachment 146442 [details]
Call frameDetached() from WebHelperPluginImpl::close()

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

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:231
> +            m_page->mainFrame()->loader()->frameDetached();

Ah, if this works, this is much better.  This will trigger all the WebCore internal state to be cleaned up as well.

[Adam Barth]

@ddorwin: If you'd like this landed via the commit-queue, you can set the commit-queue? flag and any committer can set commit-queue+.

[WebKit Review Bot]

Comment on attachment 146442 [details]
Call frameDetached() from WebHelperPluginImpl::close()

Clearing flags on attachment: 146442

Committed r119988: <http://trac.webkit.org/changeset/119988>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.