[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 #30 from Robert Hogan <robert at webkit.org>  2010-12-07 14:05:49 PST ---
(In reply to comment #28)
> (From update of attachment 75628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75628&action=review
> 
> > LayoutTests/ChangeLog:14
> > +        * platform/qt/Skipped: unskip plugins/get-url-with-blank-target.html
> > +        * platform/gtk/Skipped: ditto
> 
> What about platform/win?
> 
> > WebCore/loader/FrameLoader.cpp:207
> >      , m_sandboxFlags(SandboxAll)
> >      , m_forcedSandboxFlags(SandboxNone)
> >  {
> > +    m_PluginToNotify = 0;
> 
> This should be initialized, not assigned.
> 
> Also, wrong style, should be m_pluginToNotify.
> 
> > WebCore/loader/FrameLoader.cpp:1403
> >      Frame* frame = findFrameForNavigation(frameName);
> >      if (frame) {
> > -        frame->loader()->load(request, lockHistory);
> > +        frame->loader()->load(request, lockHistory, m_PluginToNotify);
> > +        m_PluginToNotify = 0;
> >          return;
> 
> What guarantees that frame->loader() != this?
> 
> > WebCore/loader/FrameLoader.h:494
> > +    PluginView* m_PluginToNotify;
> 
> Can the view be destroyed before FrameLoader? Will we end up with a stale pointer then?
> 
> I imagine that this may not be too bad, as the request will be stopped when destroying plug-in, too. But to program defensively, we should not keep a stale pointer, even if we're not going to use it.
> 

All fixed!

> > WebCore/plugins/PluginView.cpp:426
> > +        if (m_requestsAwaitingNotification[i]->request() == request && m_requestsAwaitingNotification[i]->sendNotification()) {
> 
> I still don't believe that comparing requests can ever be good. Am I missing something?

No, I was just reluctant to pass the PluginRequest as well as the PluginView but it now seems much cleaner that way.

> 
> > WebCore/plugins/PluginView.h:117
> > +        ResourceRequest m_resourceRequest;
> 
> If we actually don't need to compare requests, then this will go away, too.

It can't go away entirely, but I now just keep the url instead. PluginRequest::frameLoadRequest() seems to get destroyed before the request completes in some situations, causing crashes in the layout tests, so I can't reference it when sending the notification from PluginView.

Thanks for the detailed reviews!

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