[webkit-reviews] review denied: [Bug 36721] Return correct load result to NPAPI plugins calling getUrlNotify on target frames : [Attachment 75628] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 36721: Return correct load result to NPAPI plugins calling getUrlNotify on
target frames
https://bugs.webkit.org/show_bug.cgi?id=36721

Attachment 75628: Patch
https://bugs.webkit.org/attachment.cgi?id=75628&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.

> 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?

> WebCore/plugins/PluginView.h:117
> +	   ResourceRequest m_resourceRequest;

If we actually don't need to compare requests, then this will go away, too.