539755 - [OOPP] implement _getvalueforurl,_setvalueforurl,_getauthenticationinfo

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

implement _getvalueforurl,_setvalueforurl,_getauthenticationinfo. Java wants these somewhat badly.

Comment 1

[Benjamin Smedberg]

Attachment: NPN_GetValueForURL etc, rev. 1

I don't have tests yet, I'm hoping jgriffin can help out with that.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 421686 [details] [diff] [review]
NPN_GetValueForURL etc, rev. 1

>+  rpc NPN_GetValueForURL(int32_t variable, nsCString url)

I think using NPNURLVariable directly is better. We did the same with NPNVariable.

>+        username->Adopt(u, ulen);
>+        password->Adopt(p, plen);

Does adopt do ok with 0 length passed in?

>+    NS_OVERRIDE virtual bool

Why the NS_OVERRIDE? Should the other methods there get similar annotations?

> _getvalueforurl(NPP npp, NPNURLVariable variable, const char *url,
> ...
>+    if (!url || !value || !len)
>+        return NPERR_INVALID_PARAM;

Plugin host uses a different return value, NPERR_INVALID_URL, seems dangerous to change that.

Plugin host checks npp as well. Not doing that could cause plugins to crash.

Same thing for _setvalueforurl.

Comment 3

[Benjamin Smedberg]

Attachment: NPN_GetValueForURL etc, rev. 1.1

Yeah, .Adopt is perfectly happy with a 0-length string as long as the char* is actually an allocated buffer.

Comment 4

[Jonathan Griffin (:jgriffin)]

Attachment: Mochitest for NPN_GetAuthenticationInfo

Comment 5

[Jonathan Griffin (:jgriffin)]

Josh beat me to the punch for tests for NPN_Get/SetValueForURL, so I'm just adding a test for NPN_GetAuthenticationInfo.

Comment 6

[Jonathan Griffin (:jgriffin)]

Btw:  there's no documentation for NPN_GetAuthenticationInfo.  I noticed it does not restrict getting auth info for the url which loaded the plugin; it can retrieve any data in the auth manager.  Is this by design?

Comment 7

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/9537fbb2b5e8

jgriffin, I suspect it is, but I don't know the use-case. jst would!

Comment 8

[Benjamin Smedberg]

Comment on attachment 422461 [details] [diff] [review]
Mochitest for NPN_GetAuthenticationInfo

>diff --git a/modules/plugin/test/mochitest/authenticate.sjs b/modules/plugin/test/mochitest/authenticate.sjs
>new file mode 100644
>--- /dev/null
>+++ b/modules/plugin/test/mochitest/authenticate.sjs
>@@ -0,0 +1,186 @@
>+// Note: borrowed from toolkit/components/passwordmgr tests

Is there a reason we can't just use that file, instead of having two copies?


>diff --git a/modules/plugin/test/mochitest/test_getauthenticationinfo.html b/modules/plugin/test/mochitest/test_getauthenticationinfo.html

>+netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

I wonder if it makes more sense to write this as a chrome mochitest rather than using .enablePrivilege. Is the HTTP server still available in chrome mochitests?

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

>+static bool
>+getAuthInfo(NPObject* npobj, const NPVariant* args, uint32_t argCount, NPVariant* result)
>+{
>+  if (argCount != 5)
>+    return false;
>+
>+  NPP npp = static_cast<TestNPObject*>(npobj)->npp;
>+  const NPString* protocol = &NPVARIANT_TO_STRING(args[0]);
>+  const NPString* host = &NPVARIANT_TO_STRING(args[1]);
>+  uint32_t port = NPVARIANT_TO_INT32(args[2]);
>+  const NPString* scheme = &NPVARIANT_TO_STRING(args[3]);
>+  const NPString* realm = &NPVARIANT_TO_STRING(args[4]);

I don't think you should assume that these are the types you expect, but should check NPVARIANT_IS_STRING or NPVARIANT_IS_INT32 first.

>+  char* username = NULL;
>+  char* password = NULL;
>+  uint32_t ulen = 0, plen = 0;
>+  
>+  NPError err = NPN_GetAuthenticationInfo(npp, 
>+      protocol->UTF8Characters, 
>+      host->UTF8Characters, 
>+      port, 
>+      scheme->UTF8Characters, 
>+      realm->UTF8Characters,
>+      &username, 
>+      &ulen, 
>+      &password, 
>+      &plen);
>+  
>+  if (err != NPERR_NO_ERROR) {
>+    return false;
>+  }
>+
>+  char* outstring = (char*)malloc(ulen + plen + 2);
>+  memset(outstring, 0, ulen + plen + 2);
>+  strncpy(outstring, username, ulen);
>+  strcat(outstring, "|");
>+  strncat(outstring, password, plen);
>+
>+  STRINGZ_TO_NPVARIANT(outstring, *result);

Memory management is suspect here. Although there aren't existing docs, I believe the following rules apply:
* The `username` and `password` outparams in NPN_GetAuthenticationInfo are allocated, hopefully/presumably with NPN_MemAlloc. Currently you don't seem to be freeing these at all.
* The result variant string must be allocated with NPN_MemAlloc, not malloc.

Comment 9

[Jonathan Griffin (:jgriffin)]

Attachment: Mochitest for NPN_GetAuthenticationInfo v0.2

I addressed all the issues in comment #8, except moving the test to mochitest-chrome.  It turns out that's pretty convoluted, because the plugin has to reside in an http:// page (due to the authentication requirement for the test), and you can't invoke a plugin method on an http:// page from a chrome:// url due to various scripting restrictions.  There are ways around this, but I'm already working on a more general way to solve this for bug 533306, so I'd like to wait until that's done, then refactor this test along with a bunch of others.

Comment 10

[Benjamin Smedberg]

Comment on attachment 422618 [details] [diff] [review]
Mochitest for NPN_GetAuthenticationInfo v0.2

free(username);
free(password);

should be NPN_MemFree

r=me with that change

Comment 11

[Jonathan Griffin (:jgriffin)]

So, it looks like the implementation has landed in m-c:

http://hg.mozilla.org/mozilla-central/rev/9537fbb2b5e8

But the test fails in m-c when OOPP is enabled; passes when it's disabled.  When it fails, no username or password are returned.

I can step through this in a debugger if it's not immediately obvious what's going on.

Comment 12

[Jonathan Griffin (:jgriffin)]

I took a quick look in the debugger.  The code in the parent process, PluginInstanceParent::AnswerNPN_GetAuthenticationInfo(), is correctly retrieving the username/password, but when it gets to the child process, CallNPN_GetAuthenticationInfo() in _getauthenticationinfo(), the strings are empty.

Comment 13

[Benjamin Smedberg]

Stupid, dumb, yay tests.

Followup code fix http://hg.mozilla.org/mozilla-central/rev/55168ac1bf38
Tests http://hg.mozilla.org/mozilla-central/rev/b68b33478e77

Comment 14

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/a8d36e4b50e8

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 16

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430