Closed Bug 539755 Opened 15 years ago Closed 15 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: 15 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.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 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.

Attachment

General

Created:
Updated:
Size: