Bug 138675 – Implement NPAPI redirect handling

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Vicki Pfau]

We should support redirect handling for NPAPI plugins as specified in NPAPI version 26: https://wiki.mozilla.org/NPAPI:HTTPRedirectHandling

[Vicki Pfau]

Created attachment 241447 [details]
Patch

[Vicki Pfau]

Created attachment 241454 [details]
Patch

[WebKit Commit Bot]

Attachment 241454 [details] did not pass style-queue:


ERROR: Source/WebCore/plugins/npapi.h:871:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872:  The parameter name "direction" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873:  Extra space between void and NP_LOADDS  [whitespace/declaration] [3]
Total errors found: 4 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vicki Pfau]

Created attachment 241456 [details]
Patch

[WebKit Commit Bot]

Attachment 241456 [details] did not pass style-queue:


ERROR: Source/WebCore/plugins/npapi.h:871:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872:  The parameter name "direction" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873:  Extra space between void and NP_LOADDS  [whitespace/declaration] [3]
Total errors found: 4 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vicki Pfau]

Created attachment 241460 [details]
Patch

[WebKit Commit Bot]

Attachment 241460 [details] did not pass style-queue:


ERROR: Source/WebCore/plugins/npapi.h:871:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872:  The parameter name "direction" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873:  Extra space between void and NP_LOADDS  [whitespace/declaration] [3]
Total errors found: 4 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 241460 [details]
Patch

Couple of things:

* I think you should split up the patch and land the loader parts before the plug-in support.
* Why are no changes to ResourceHandle needed? It seems like asynchronous willSendRequest would require that.
* I think you should just change willSendRequest and have it take an std::function instead of adding "maybeSendRequest". 
* Having ResourceRequest and bool seems strange, maybe you can use a WTF::Optional<ResourceRequest> instead.

[Build Bot]

Comment on attachment 241460 [details]
Patch

Attachment 241460 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6530079978946560

New failing tests:
http/tests/plugins/get-url-redirect.html

[Build Bot]

Created attachment 241543 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Vicki Pfau]

Created attachment 241911 [details]
Patch

[Vicki Pfau]

Comment on attachment 241543 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

This should fix all of the build and test issues, but I should probably still split this into two patches.

[WebKit Commit Bot]

Attachment 241911 [details] did not pass style-queue:


ERROR: Source/WebCore/plugins/npapi.h:871:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872:  The parameter name "direction" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873:  Extra space between void and NP_LOADDS  [whitespace/declaration] [3]
Total errors found: 4 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vicki Pfau]

Created attachment 242469 [details]
Patch

[WebKit Commit Bot]

Attachment 242469 [details] did not pass style-queue:


ERROR: Source/WebCore/plugins/npapi.h:871:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872:  The parameter name "direction" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873:  Extra space between void and NP_LOADDS  [whitespace/declaration] [3]
Total errors found: 4 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 242469 [details]
Patch

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

> Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:70
> +void NetscapePlugInStreamLoader::willSendRequest(ResourceRequest&& request, const ResourceResponse& redirectResponse, std::function<void(ResourceRequest&)> callback)

function parameter should be ResourceRequest&&.

> Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:76
> +        if (request.isNull() || !protect->m_client) {
> +            request = ResourceRequest();

This is weird. Just pass an empty ResourceRequest to callback() instead.

> Source/WebCore/loader/NetscapePlugInStreamLoader.h:41
> +    virtual void willSendRequest(NetscapePlugInStreamLoader*, ResourceRequest&&, const ResourceResponse& redirectResponse, std::function<void(ResourceRequest&)> callback) = 0;

I think "completionHandler" is a better name than callback.

> Source/WebKit/mac/Plugins/Hosted/HostedNetscapePluginStream.mm:201
> +    // FIXME: Call NPP_URLRedirectNotify

Is there a bug number for this FIXME?

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapeBrowserFuncs.cpp:1052
> +    netscapeFuncs.urlredirectresponse = NPN_URLRedirectResponse;

This should only be set if we're on an OS where this is supported.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:407
> +void NetscapePlugin::registerRedirect(NetscapePluginStream* stream, const URL& requestURL, int redirectResponseStatus, void* notificationData)

"registerRedirect" is a weird name.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:420
> +    auto pair = m_redirects.take(notifyData);

This needs to handle invalid hash keys. Also, pair is a bad variable name, especially since you're using auto here.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePluginStream.cpp:352
> +void NetscapePluginStream::redirect(const String& newURLString)
> +{
> +    m_requestURLString = newURLString;
> +}

This is just a setter. It should have a setter name.

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:141
> +    std::function<void(ResourceRequest&)> m_callback;

This should be named better.

[Vicki Pfau]

Created attachment 242550 [details]
Patch

[WebKit Commit Bot]

Attachment 242550 [details] did not pass style-queue:


ERROR: Source/WebCore/plugins/PluginStream.cpp:406:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/plugins/npapi.h:871:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872:  The parameter name "direction" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873:  Extra space between void and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebKit/mac/Plugins/Hosted/HostedNetscapePluginStream.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/Hosted/HostedNetscapePluginStream.mm:199:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:141:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:227:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/WebNetscapePluginStream.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/WebNetscapePluginStream.mm:300:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/plugins/PluginStream.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/loader/ResourceLoader.cpp:289:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/loader/ResourceLoader.h:96:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/loader/NetscapePlugInStreamLoader.h:41:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/loader/NetscapePlugInStreamLoader.h:60:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 18 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

<rdar://problem/15779101>

[Alexey Proskuryakov]

*** Bug 49480 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Created attachment 258133 [details]
proposed patch

Worked through how this patch works by re-writing many parts, and ended up with the same result. A few differences:
- used rvalue references more;
- removed a "cancel loaded" clause in WebResourceLoader that was added in r176626 without explanation, and shouldn't be needed;
- performed a small refactoring in ResourceHandle to clean up "willSendRequest" variants.

[WebKit Commit Bot]

Attachment 258133 [details] did not pass style-queue:


ERROR: Source/WebKit/win/Plugins/PluginStream.cpp:406:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/plugins/npapi.h:871:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872:  The parameter name "direction" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872:  Extra space between NPBool and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873:  Extra space between void and NP_LOADDS  [whitespace/declaration] [3]
ERROR: Source/WebKit/mac/Plugins/Hosted/HostedNetscapePluginStream.h:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/Hosted/HostedNetscapePluginStream.mm:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/win/Plugins/PluginStream.h:85:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:132:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:141:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:224:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/WebNetscapePluginStream.h:100:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/WebNetscapePluginStream.mm:300:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/loader/NetscapePlugInStreamLoader.h:41:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 14 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 258133 [details]
proposed patch

Clearing flags on attachment: 258133

Committed r187886: <http://trac.webkit.org/changeset/187886>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This patch broke the Windows build:

..\..\win\Plugins\PluginView.cpp(800): error C2248: 'WebCore::PluginStream::didReceiveResponse' : cannot access private member declared in class 'WebCore::PluginStream' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(86) : see declaration of 'WebCore::PluginStream::didReceiveResponse'
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(57) : see declaration of 'WebCore::PluginStream'
..\..\win\Plugins\PluginView.cpp(811): error C2248: 'WebCore::PluginStream::didReceiveData' : cannot access private member declared in class 'WebCore::PluginStream' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(87) : see declaration of 'WebCore::PluginStream::didReceiveData'
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(57) : see declaration of 'WebCore::PluginStream'
..\..\win\Plugins\PluginView.cpp(822): error C2248: 'WebCore::PluginStream::didFinishLoading' : cannot access private member declared in class 'WebCore::PluginStream' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(89) : see declaration of 'WebCore::PluginStream::didFinishLoading'
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(57) : see declaration of 'WebCore::PluginStream'
..\..\win\Plugins\PluginView.cpp(833): error C2248: 'WebCore::PluginStream::didFail' : cannot access private member declared in class 'WebCore::PluginStream' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(88) : see declaration of 'WebCore::PluginStream::didFail'
          c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(57) : see declaration of 'WebCore::PluginStream'

[Alex Christensen]

Fixed in http://trac.webkit.org/changeset/187894