Expose the remainder of the Java plugins XPCOM dependencies cleanly through NPAPI.

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
Plug-ins
P2
normal
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 354356 [details] [diff] [review]
Add NPN_Get/SetValueForURL() and NPN_GetAuthenticationInfo().

Right now the Java plugin still uses XPCOM APIs for 3 things, all of which can be cleanly exposed through the NPAPI. Other plugins are interested in some of this as well. The functionality is specifically:

- cookie info
- proxy info
- authentication info

This has already been discussed on the plugin-futures mailing list, but does not have final agreement yet. But this bug is a placeholder for the work to add this to the Mozilla implementation. I've already got what I think is all we need to do here, so see attachment for details.
Attachment #354356 - Flags: review?(joshmoz)
Duplicate of this bug: 315683
Blocks: 408516
(Assignee)

Updated

10 years ago
Flags: blocking1.9.1+

Comment 2

10 years ago
This stuff should have tests, and for tests we have to get bug 386676 pushed successfully :)

Comment 3

10 years ago
Comment on attachment 354356 [details] [diff] [review]
Add NPN_Get/SetValueForURL() and NPN_GetAuthenticationInfo().

>+_findproxyforurl(const char* url, char* proxy, uint32_t* len)
...
>+    char ** result;
>+    uint32 resultlen;
>+    if (NS_SUCCEEDED(pm->FindProxyForURL(url, result))) {
>+      resultlen = PL_strlen(*result);
>+      if (resultlen < *len) {
>+        PL_strcpy(proxy, *result);
>+        rv = NS_OK;
>+      }
>+      *len = resultlen;

You're assigning "NS_OK" to a variable of type "NPError" - probably want "NPERR_NO_ERROR".

Why modify the value of "len" even if you didn't perform the copy and will return an error? Is that some way of indicating to the caller that the provided buffer was too small?

>+_setvalueforurl(NPP instance, NPNURLVariable variable, const char *url,
>+                const char *value)
...
>+  case NPNURLVCookie:
>+    {
>+      nsCOMPtr<nsICookieStorage> cs = do_GetService(kPluginManagerCID);
>+
>+      if (cs) {
>+        char buf[256];
>+        cs->SetCookie(url, buf, strlen(value));
>+
>+        // Deal with errors.

Why is that buffer empty? Why not deal with errors now?

>+  case NPNURLVProxy:

Is this supposed to just return an error?

>+_getauthenticationinfo(NPP instance, const char *protocol, const char *host,
...
>+  if (!PL_strcasecmp(protocol, "HTTP") && !PL_strcasecmp(protocol, "HTTPS"))
>+    return NPERR_GENERIC_ERROR;

This logic seems off to me. I think you're trying to say "if this is not HTTP or HTTPS then bail early". If the protocol value is "HTTP" the first test will return 0, which you negate to true, which means you then test to see if protocol equals "HTTPS", which it obviously does not because you already know it equals "HTTP". Second test will be non-zero, which you'd negate to false, and you'd end up returning early for a protocol value of "HTTP".
Attachment #354356 - Flags: review?(joshmoz) → review-

Comment 4

10 years ago
My last comment about HTTP/HTTPS logic is not totally true, you'd end up not returning early for HTTP because your test resolves to (true && false). You'd also not return early for a protocol value of FTP.
(Assignee)

Comment 5

10 years ago
Created attachment 355700 [details] [diff] [review]
Add NPN_Get/SetValueForURL() and NPN_GetAuthenticationInfo().

I need to start paying more attention to what I cut n' paste, obviously.

Josh, this should take care of all of what you found above, and some other minor problems as well.
Attachment #354356 - Attachment is obsolete: true
Attachment #355700 - Flags: superreview?(bzbarsky)
Attachment #355700 - Flags: review?(joshmoz)

Comment 6

10 years ago
Comment on attachment 355700 [details] [diff] [review]
Add NPN_Get/SetValueForURL() and NPN_GetAuthenticationInfo().

Looks good, perhaps we should document memory mgmt for outparams of getters for auth and URL proxy/cookie.
Attachment #355700 - Flags: review?(joshmoz) → review+
What's _findproxyforurl used for?

Is there a reason we restrict the length of the cookie string?

Documenting the memory management and charset stuff for those char*s would be good.

Are we sure cookie values never contain null?  Or do we need to hand out the cookie length too?
Comment on attachment 355700 [details] [diff] [review]
Add NPN_Get/SetValueForURL() and NPN_GetAuthenticationInfo().

sr- pending response
Attachment #355700 - Flags: superreview?(bzbarsky) → superreview-
P2, per discussion w/ JST.
Priority: -- → P2
(Assignee)

Comment 10

10 years ago
Created attachment 356283 [details] [diff] [review]
Updated fix.

_findproxyforurl() was leftovers from an earlier version of this :(

I ended up adding length arguments to the get/setvalueforurl functions and that way dealing with whatever values that might have nulls in them. The code in the previous patches was basically a copy of what the current JVM auth tools in our code do, which obviously didn't work in those cases.
Attachment #355700 - Attachment is obsolete: true
Attachment #356283 - Flags: superreview?(bzbarsky)
Attachment #356283 - Flags: review+
Comment on attachment 356283 [details] [diff] [review]
Updated fix.

sr=bzbarsky
Attachment #356283 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 13

10 years ago
Fix pushed to trunk and 1.9.1
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
getvalueforurl + NPNURLVCookie returns always NPERR_GENERIC_ERROR. Maybe !cookieStr.get(), instead of !cookieStr ?

Updated

8 years ago
Depends on: 564070
You need to log in before you can comment on or make changes to this bug.