Bug 138675 - Implement NPAPI redirect handling
Summary: Implement NPAPI redirect handling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar
: 49480 (view as bug list)
Depends on: 138987
Blocks:
  Show dependency treegraph
 
Reported: 2014-11-12 15:24 PST by Vicki Pfau
Modified: 2015-08-04 15:17 PDT (History)
9 users (show)

See Also:


Attachments
Patch (60.31 KB, patch)
2014-11-12 15:41 PST, Vicki Pfau
no flags Details | Formatted Diff | Diff
Patch (59.68 KB, patch)
2014-11-12 17:11 PST, Vicki Pfau
no flags Details | Formatted Diff | Diff
Patch (66.77 KB, patch)
2014-11-12 17:24 PST, Vicki Pfau
no flags Details | Formatted Diff | Diff
Patch (66.80 KB, patch)
2014-11-12 18:09 PST, Vicki Pfau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (643.78 KB, application/zip)
2014-11-13 20:26 PST, Build Bot
no flags Details
Patch (68.70 KB, patch)
2014-11-19 17:25 PST, Vicki Pfau
no flags Details | Formatted Diff | Diff
Patch (64.98 KB, patch)
2014-12-02 17:43 PST, Vicki Pfau
no flags Details | Formatted Diff | Diff
Patch (68.85 KB, patch)
2014-12-03 19:35 PST, Vicki Pfau
no flags Details | Formatted Diff | Diff
proposed patch (75.74 KB, patch)
2015-08-03 16:32 PDT, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vicki Pfau 2014-11-12 15:24:09 PST
We should support redirect handling for NPAPI plugins as specified in NPAPI version 26: https://wiki.mozilla.org/NPAPI:HTTPRedirectHandling
Comment 1 Vicki Pfau 2014-11-12 15:41:51 PST
Created attachment 241447 [details]
Patch
Comment 2 Vicki Pfau 2014-11-12 17:11:25 PST
Created attachment 241454 [details]
Patch
Comment 3 WebKit Commit Bot 2014-11-12 17:13:21 PST
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.
Comment 4 Vicki Pfau 2014-11-12 17:24:25 PST
Created attachment 241456 [details]
Patch
Comment 5 WebKit Commit Bot 2014-11-12 17:25:40 PST
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.
Comment 6 Vicki Pfau 2014-11-12 18:09:09 PST
Created attachment 241460 [details]
Patch
Comment 7 WebKit Commit Bot 2014-11-12 18:10:37 PST
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.
Comment 8 Anders Carlsson 2014-11-13 20:12:31 PST
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.
Comment 9 Build Bot 2014-11-13 20:26:32 PST
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
Comment 10 Build Bot 2014-11-13 20:26:34 PST
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
Comment 11 Vicki Pfau 2014-11-19 17:25:34 PST
Created attachment 241911 [details]
Patch
Comment 12 Vicki Pfau 2014-11-19 17:26:20 PST
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.
Comment 13 WebKit Commit Bot 2014-11-19 17:27:46 PST
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.
Comment 14 Vicki Pfau 2014-12-02 17:43:41 PST
Created attachment 242469 [details]
Patch
Comment 15 WebKit Commit Bot 2014-12-02 17:46:11 PST
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.
Comment 16 Anders Carlsson 2014-12-03 09:31:06 PST
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.
Comment 17 Vicki Pfau 2014-12-03 19:35:13 PST
Created attachment 242550 [details]
Patch
Comment 18 WebKit Commit Bot 2014-12-03 19:36:44 PST
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.
Comment 19 David Kilzer (:ddkilzer) 2014-12-07 15:10:19 PST
<rdar://problem/15779101>
Comment 20 Alexey Proskuryakov 2015-07-28 11:15:54 PDT
*** Bug 49480 has been marked as a duplicate of this bug. ***
Comment 21 Alexey Proskuryakov 2015-08-03 16:32:16 PDT
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.
Comment 22 WebKit Commit Bot 2015-08-03 16:33:25 PDT
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.
Comment 23 WebKit Commit Bot 2015-08-04 13:49:00 PDT
Comment on attachment 258133 [details]
proposed patch

Clearing flags on attachment: 258133

Committed r187886: <http://trac.webkit.org/changeset/187886>
Comment 24 WebKit Commit Bot 2015-08-04 13:49:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Brent Fulgham 2015-08-04 14:53:38 PDT
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'
Comment 26 Alex Christensen 2015-08-04 15:17:40 PDT
Fixed in http://trac.webkit.org/changeset/187894