[Webkit-unassigned] [Bug 36721] Return correct load result to NPAPI plugins calling getUrlNotify on target frames

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36721





--- Comment #34 from Robert Hogan <robert at webkit.org>  2010-12-08 13:09:47 PST ---
(In reply to comment #33)
> (From update of attachment 75840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75840&action=review
> 
> I haven't had the time to review the patch in detail yet. I'm feeling uneasy about the PluginRequest pointer that might also become stale, and about adding so many data members to various classes, meaning that information gets duplicated. Duplicated information is the source of all evil, as the copies tend to get out of sync.
> 
> > WebCore/loader/FrameLoader.cpp:1406
> > +        frame->loader()->load(request, lockHistory, m_pluginToNotify, m_pluginRequest);
> > +        if (frame->loader() != this)
> > +            clearPluginReferences();
> 
> What guarantees that the frame pointer isn't stale after calling load()? 

I see what you mean.

> I think that a cleaner approach would be to not store the references to member variables at all in this code path (meaning that load() wouldn't get another override).

Do you mean:

if (frame->loader() != this) {
  frame->loader()->load(request, lockHistory, m_pluginToNotify, m_pluginRequest);
  clearPluginReferences();
} else
  frame->loader()->load(request, lockHistory);

Or do you mean if a target frame is specified, don't wait for the request result to notify the plugin?


> 
> > WebCore/loader/FrameLoader.h:340
> > +    void clearPluginReferences();
> 
> Commonly, we'd call this pluginViewDestroyed(), and pass a plugin view pointer. This also makes me wonder about what exactly happens if two plug-in instances try to load in a frame (surely one request will be stopped, but does it work right?)

Would something like this remove the uncertainty?

void FrameLoader::load(const ResourceRequest& request, bool lockHistory, PluginView* plugin, PluginRequest* pluginRequest)
{
    if (m_pluginToNotify){
        notifyPlugin(ResourceError(errorDomainWebKitInternal, 0, request.url().string(), ""));
        clearPluginReferences();
    }
    m_pluginToNotify = plugin;
    m_pluginRequest = pluginRequest;
    load(request, lockHistory);
}

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.