Closed Bug 539755 Opened 13 years ago Closed 13 years ago

[OOPP] implement _getvalueforurl,_setvalueforurl,_getauthenticationinfo

Categories

(Core Graveyard :: Plug-ins, defect)

x86
Linux
defect
Not set
normal

Tracking

(status1.9.2 .4-fixed)

RESOLVED FIXED
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

(Whiteboard: [fixed-lorentz])

Attachments

(2 files, 2 obsolete files)

implement _getvalueforurl,_setvalueforurl,_getauthenticationinfo. Java wants these somewhat badly.
Attached patch NPN_GetValueForURL etc, rev. 1 (obsolete) — Splinter Review
I don't have tests yet, I'm hoping jgriffin can help out with that.
Attachment #421686 - Flags: review?(bent.mozilla)
Flags: in-testsuite?
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.
Yeah, .Adopt is perfectly happy with a 0-length string as long as the char* is actually an allocated buffer.
Attachment #421686 - Attachment is obsolete: true
Attachment #421853 - Flags: review?(bent.mozilla)
Attachment #421686 - Flags: review?(bent.mozilla)
Attachment #421853 - Flags: review?(bent.mozilla) → review+
Depends on: 530980
Attachment #422461 - Flags: review?(benjamin)
Josh beat me to the punch for tests for NPN_Get/SetValueForURL, so I'm just adding a test for NPN_GetAuthenticationInfo.
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?
http://hg.mozilla.org/mozilla-central/rev/9537fbb2b5e8

jgriffin, I suspect it is, but I don't know the use-case. jst would!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
Attachment #422461 - Flags: review?(benjamin) → review-
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.
Attachment #422461 - Attachment is obsolete: true
Attachment #422618 - Flags: review?(benjamin)
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
Attachment #422618 - Flags: review?(benjamin) → review+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Stupid, dumb, yay tests.

Followup code fix http://hg.mozilla.org/mozilla-central/rev/55168ac1bf38
Tests http://hg.mozilla.org/mozilla-central/rev/b68b33478e77
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.