475991 - Extend the NPAPI to allow plugins to participate in redirects

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[Ian Melven]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729)

We would like to extend the NPAPI to allow plugins to participate in redirects.

Our proposal which has been posted to plugin-futures and discussed there is :

We propose adding a new callback in the style of URLNotify that would allow a plugin to prevent the browser from following any HTTP redirects for the two calls NPN_GetURLNotify and NPN_PostURLNotify. The other two non-notification calls NPN_GetURL and NPN_PostURL will not change.

Non-HTTP redirects, for example, if a file:// URL redirects to another file via a link/shortcut etc. will not be processed via this mechanism and their behaviour will not change.

A new NPVERS value will be added to inform the plug-in that the browser's NPPluginFuncs structure includes the the field for the callback listed below. This does not mean that the browser will actually call it.

NPVERS_HAS_REDIRECT_BEHAVIOR = 22 

If the browser version is at least the above value, the plug-in can provide a callback in a new field added to the NPPPluginFuncs structure, at the end:

int32 NPP_URLNotifyRedirect(NPP instance, const char* url, int32 status, void* notifyData)

  instance:  the plug-in instance that made the request
  url: the new redirected url, using UTF-8 encoding
  status: the status code returned from the server (should be 3xx)
  notifyData: the data passed into NPN_PostURLNotify() or NPN_GetURLNotify()

the return value is:

  NPERR_NO_ERR:  the browser will go ahead and make the redirect call; there may be more calls to NPP_URLNotifyRedirect if there are further redirections.

any other value: the browser will not redirect, and then will call NPP_URLNotify() with reason NPRES_USER_BREAK, and the last redirected url (the same one passed to the NPP_URLNotifyRedirect the last time).

If the plug-in has the above version or later, it can put NULL in the NPPluginFuncs field, and the browser will not use the behavior.

The variable that dictates whether or not the browser supports redirect notification is NPVariantType_Bool:

NPNVsupportsRedirectNotification = 3000

The plug-in should call NPN_GetValue() on this.

If an error or false is returned, the browser does not support this behavior and the existing behavior will apply: redirects are followed, and NPP_URLNotify is called with the final redirected URL, and NPP_URLNotifyRedirect is never called. As now, it is up to the plug-in to keep the original URL (if it needs to) in its opaque notifyData structure, but the plug-in will have no indication of the possible chain of redirects that led it to the final URL, nor control over whether or not the browser re-sends headers or POST data to the redirected URLs (ie browsers sometimes change POSTs to GETs for security reasons, but this is entirely up to the browser and should not be affected by this change).

If true is returned, it is supported, and if the plug-in version is of the above version or later, and has filled in the appropriate field of the NPPluginFuncs structure in during the call to NP_GetEntryPoints, then it will be called on all redirects for requests made by the plug-in through NPN_GetURLNotify() and NPN_PostURLNotify()

A plugin can use its private notifyData argument to NPN_GetURLNotify() and NPN_PostURLNotify() to associate those requests with the NPN_URLNotifyRedirect callback they may cause internally, there is no explicit means to do this via the NPAPI or the browser, it is left to the plugin. 


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Ian Melven]

I am working on the patch to implement this.

Comment 2

[Johnny Stenback (:jst)]

We need to fix this for 1.9.3 final, marking as such to make sure it doesn't get lost.

Comment 3

[Benjamin Smedberg]

jst, has the NPAPI spec for this been agreed-on? I would take this after FF4 ships, especially considering that we don't have shipping plugins which use it.

Comment 4

[Josh Aas]

We have not agreed on a spec.

Comment 5

[Josh Aas]

The following NPAPI extension specification has been approved. We'll be implementing for Firefox 4.

https://wiki.mozilla.org/NPAPI:HTTPRedirectHandling

Comment 6

[Josh Aas]

I think this should go in early, marking blocking2.0beta8+. I am working on a patch now, maybe 60% done.

Comment 7

[Josh Aas]

Attachment: fix v0.6

Backing up my work so far. This is not ready to go but it does basically work for in-process and out-of-process plugins.

Comment 8

[Josh Aas]

Attachment: fix v0.7

Comment 9

[Honza Bambas (:mayhemer)]

Maybe rather use NS_BINDING_ABORTED instead of NS_ERROR_FAILURE.

Comment 10

[Josh Aas]

(In reply to comment #9)
> Maybe rather use NS_BINDING_ABORTED instead of NS_ERROR_FAILURE.

The documentation for NS_BINDING_ABORTED says "The async request failed because it was aborted by some user action." The request is not being aborted by the user.

Comment 11

[Josh Aas]

Attachment: fix v1.0

Still needs tests but this should be a full implementation.

Comment 12

[Benjamin Smedberg]

I just posted to plugin-futures a questions about the spec and what NPStream.url ought to be when redirects occur.

Comment 13

[Benjamin Smedberg]

Comment on attachment 494295 [details] [diff] [review]
fix v1.0

How does the behavior of the plugin host differ if the plugin doesn't implement NPP_URLRedirectNotify? If there is not a significant difference in behavior, URLRedirectNotifySupported is probably overkill: we should just always send the notification, and if the plugin doesn't implement it we just send back the "redirect allowed" message.

In StreamNotifyChild::RecvRedirectNotify the "if" clause seems wrong. If there is no closure, we'll never notify the plugin and the stream will sit in suspended state indefinitely.

The API design of NPN_URLRedirectResponse is a bit annoying. I would prefer that we passed in a closure pointer that they handed back, rather than reusing their notifyData. Looping through the existing streams probably isn't *that* expensive, but it doesn't feel good to me. And if there were two streams with the same notifyData, things might get even more weird.

In nsNPAPIPluginInstance::SendRedirectNotification:
 if (pluginFunctions->version >= NPVERS_HAS_URL_REDIRECT_HANDLING) doesn't make much sense to me: don't we allocate the pluginFunctions structure? Simply doing the null-check sounds sufficient.

We don't null-check notifyData in this function. Why not? Note that the response we get will currently have null notifydata, which would be useless for looking up the stream in question.

What does the NS_ERROR_FAILURE code mean in practice? It seems odd to be returning failure from this method, but I see that it falls through to the default logic in nsPluginStreamListenerPeer::AsyncOnChannelRedirect. This should probably be noted in a comment. Perhaps making that method return bool/PRBool instead of nsresult would be more clear.

Comment 14

[Josh Aas]

(In reply to comment #13)

> How does the behavior of the plugin host differ if the plugin doesn't implement
> NPP_URLRedirectNotify? If there is not a significant difference in behavior,
> URLRedirectNotifySupported is probably overkill: we should just always send the
> notification, and if the plugin doesn't implement it we just send back the
> "redirect allowed" message.

There is a significant difference - if the plugin implements 'NPP_URLRedirectNotify' it takes full responsibility for redirects. If it does not we have our own backup policy that disallows at least one kind of potentially dangerous redirect.

> In StreamNotifyChild::RecvRedirectNotify the "if" clause seems wrong. If there
> is no closure, we'll never notify the plugin and the stream will sit in
> suspended state indefinitely.

I will look into this.

> The API design of NPN_URLRedirectResponse is a bit annoying. I would prefer
> that we passed in a closure pointer that they handed back, rather than reusing
> their notifyData. Looping through the existing streams probably isn't *that*
> expensive, but it doesn't feel good to me. And if there were two streams with
> the same notifyData, things might get even more weird.

Two streams can't have the same notifyData. That would be a bug. Too late to change the spec on this point, multiple people have started implementing. We could improve the way we keep track of streams internally in the future.

> In nsNPAPIPluginInstance::SendRedirectNotification:
>  if (pluginFunctions->version >= NPVERS_HAS_URL_REDIRECT_HANDLING) doesn't make
> much sense to me: don't we allocate the pluginFunctions structure? Simply doing
> the null-check sounds sufficient.

I agree. I was actually going to get rid of that check in an updated patch already. I posted this patch before I investigated my suspicions that resulted in bug 615881.

> We don't null-check notifyData in this function. Why not? Note that the
> response we get will currently have null notifydata, which would be useless for
> looking up the stream in question.

I will look into this.

> What does the NS_ERROR_FAILURE code mean in practice? It seems odd to be
> returning failure from this method, but I see that it falls through to the
> default logic in nsPluginStreamListenerPeer::AsyncOnChannelRedirect. This
> should probably be noted in a comment. Perhaps making that method return
> bool/PRBool instead of nsresult would be more clear.

I will look into this.

Comment 15

[Benjamin Smedberg]

> Two streams can't have the same notifyData. That would be a bug.

Why? That's certainly not *documented* anywhere, and although I agree it sounds really funny and probably dumb, it hasn't been illegal until now...

Comment 16

[Josh Aas]

(In reply to comment #15)
> > Two streams can't have the same notifyData. That would be a bug.
> 
> Why? That's certainly not *documented* anywhere, and although I agree it sounds
> really funny and probably dumb, it hasn't been illegal until now...

It probably isn't documented anywhere, that API comes from an era in which not much was documented. I think it is pretty well understood though and I don't think any plugins of significance do that. Almost certainly not any plugins that will be using this API.

Comment 17

[Josh Aas]

(In reply to comment #15)

> Why? That's certainly not *documented* anywhere, and although I agree it sounds
> really funny and probably dumb, it hasn't been illegal until now...

Also, we could document it and send a clarifying email to plugin-futures. I don't think we necessarily have to allow anything that wasn't explicitly documented as off-limits.

Comment 18

[Josh Aas]

Attachment: fix v1.1

Comment 19

[Benjamin Smedberg]

Comment on attachment 494475 [details] [diff] [review]
fix v1.1

>diff --git a/dom/plugins/PluginInstanceChild.cpp b/dom/plugins/PluginInstanceChild.cpp

>+bool
>+StreamNotifyChild::RecvRedirectNotify(const nsCString& url, const int32_t& status)
>+{
>+    PluginInstanceChild* instance = static_cast<PluginInstanceChild*>(Manager());
>+
>+    if (mClosure && instance->mPluginIface->urlredirectnotify)
>+      instance->mPluginIface->urlredirectnotify(instance->GetNPP(), mURL.get(), status, mClosure);

This is still incorrect unless mClosure is non-null. We could perhaps *require* that it be non-null in the call to NPN_GetURLNotify, but we should enforce that rule if we're going to make it (and we'll need to enforce it separately for in-process and OOPP).

>+void PluginInstanceChild::NPN_URLRedirectResponse(void* notifyData, NPBool allow)

nit, void on its own line

>+{
>+    if (!notifyData) {
>+        return;
>+    }
>+
>+    InfallibleTArray<PStreamNotifyChild*> notifyStreams;
>+    ManagedPStreamNotifyChild(notifyStreams);
>+    PRUint32 notifyStreamCount = notifyStreams.Length();
>+    for (PRUint32 i = 0; i < notifyStreamCount; i++) {
>+        StreamNotifyChild* sn = static_cast<StreamNotifyChild*>(notifyStreams[i]);
>+        if (sn->mClosure == notifyData) {
>+            sn->SendRedirectNotifyResponse(static_cast<bool>(allow));
>+            return;
>+        }
>+    }

Assert that we don't hit the bottom of this function? At least in debug builds we should figure out if plugins are making weird calls.

>diff --git a/modules/plugin/base/src/nsNPAPIPluginInstance.cpp b/modules/plugin/base/src/nsNPAPIPluginInstance.cpp

>+void nsNPAPIPluginInstance::URLRedirectResponse(void* notifyData, NPBool allow)

nit, void on its own line

Comment 20

[Josh Aas]

Attachment: fix v1.2

Updated patch, not ready yet because it does not address Benjamin's concerns about hanging the redirect when notifyData is NULL.

Comment 21

[Josh Aas]

Attachment: fix v1.3

As using a null closure (notifyData) with this API is not allowed, this version of the patch will disallow any redirect for a stream with null closure data when the plugin claims to support redirect handling.

I also refactored some of the code to make the logic more clear.

Comment 22

[Josh Aas]

Attachment: fix v1.4

Fix Windows compile error.

Comment 23

[Josh Aas]

Attachment: tests v1.0

Comment 24

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 498864 [details] [diff] [review]
tests v1.0

Sorry, Josh, but I'm about to go on Christmas vacation and won't be able to finish my review before I go.

I should be back in about a week.

Comment 25

[Rob Campbell [:rc] (:robcee)]

Attachment: [checked-in] line-ending-fix

fix for line-endings in two files

Comment 26

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 499522 [details] [diff] [review]
[checked-in] line-ending-fix

http://hg.mozilla.org/mozilla-central/rev/de553c56e953

Comment 27

[Josh Aas]

Attachment: tests v1.1

Comment 28

[Josh Aas]

Attachment: tests v1.2

Comment 29

[[e-mail]]

Comment on attachment 499563 [details] [diff] [review]
tests v1.2

>diff --git a/modules/plugin/test/testplugin/nptest.cpp b/modules/plugin/test/testplugin/nptest.cpp

>+void
>+NPP_URLRedirectNotify(NPP instance, const char* url, int32_t status, void* notifyData)
>+{
>+  if (notifyData) {
>+    NPN_URLRedirectResponse(instance, notifyData,
>+                            static_cast<URLNotifyData*>(notifyData)->allowRedirects);
>+  }
>+  NPN_URLRedirectResponse(instance, notifyData, true);

Do you actually want this twice? And if not, can you tweak the test so it would catch it? :)

>@@ -2303,25 +2337,41 @@ streamTest(NPObject* npobj, const NPVari

>-  if (!NPVARIANT_IS_OBJECT(args[3]))
>+  NPObject* writeCallback = NULL;
>+  if (NPVARIANT_IS_NULL(args[3])) {
>+  }

This looks a little odd. Maybe move the NULL assignment inside the block? Compiler shouldn't warn, since you're initializing on both branches...

r=dwitte

Comment 30

[Josh Aas]

Attachment: tests v1.3

Comment 31

[Josh Aas]

The existing implementation has a bug which essentially disables the new feature for Windows OOPPs because we look for plugin-side redirect support in the plugin's function table before we have asked the plugin to initialize the table. This is what causes the tests to fail on Windows.

Comment 32

[Josh Aas]

Attachment: fix v1.5

On Windows NP_GetEntryPoints has to be called before NP_Initialize. In our OOPP code we setup up a function table in the parent process when NP_GetEntryPoints is called but we don't actually force the child process to call NP_GetEntryPoints until the subsequent call to NP_Initialize. That used to work because the parent's function table was never dependent on the child's. Since NPAPI redirect handling support is determined by the pointer value (NULL or not) in the plugin's function table that is no longer true. We need to actually call NP_GetEntryPoints in the child when asked to so that the parent can query for NULL entries. This updated patch does that and all tests pass on Windows.

This also fixes bug 622667, the negative consequences of which are exposed by this patch.

Comment 33

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/b9ee503b2c90
http://hg.mozilla.org/mozilla-central/rev/5efd80cc9937