Closed
Bug 539755
Opened 15 years ago
Closed 15 years ago
[OOPP] implement _getvalueforurl,_setvalueforurl,_getauthenticationinfo
Categories
(Core Graveyard :: Plug-ins, defect)
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)
10.10 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
11.14 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
implement _getvalueforurl,_setvalueforurl,_getauthenticationinfo. Java wants these somewhat badly.
Assignee | ||
Comment 1•15 years ago
|
||
I don't have tests yet, I'm hoping jgriffin can help out with that.
Attachment #421686 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•15 years ago
|
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.
Assignee | ||
Comment 3•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #421853 -
Flags: review?(bent.mozilla) → review+
Comment 4•15 years ago
|
||
Attachment #422461 -
Flags: review?(benjamin)
Comment 5•15 years ago
|
||
Josh beat me to the punch for tests for NPN_Get/SetValueForURL, so I'm just adding a test for NPN_GetAuthenticationInfo.
Comment 6•15 years ago
|
||
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?
Assignee | ||
Comment 7•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/9537fbb2b5e8
jgriffin, I suspect it is, but I don't know the use-case. jst would!
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 8•15 years ago
|
||
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-
Comment 9•15 years ago
|
||
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)
Assignee | ||
Comment 10•15 years ago
|
||
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+
Comment 11•15 years ago
|
||
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 → ---
Comment 12•15 years ago
|
||
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.
Assignee | ||
Comment 13•15 years ago
|
||
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: 15 years ago → 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•15 years ago
|
||
Whiteboard: [fixed-lorentz]
Comment 15•15 years ago
|
||
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
Assignee | ||
Comment 16•15 years ago
|
||
Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430
status1.9.2:
--- → .4-fixed
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•