Closed Bug 470993 Opened 16 years ago Closed 15 years ago

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

Categories

(Core Graveyard :: Plug-ins, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: jst, Assigned: jst)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 2 obsolete files)

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)
Blocks: 408516
Flags: blocking1.9.1+
This stuff should have tests, and for tests we have to get bug 386676 pushed successfully :)
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-
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.
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 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
Attached patch Updated fix.Splinter Review
_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+
Fix pushed to trunk and 1.9.1
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
getvalueforurl + NPNURLVCookie returns always NPERR_GENERIC_ERROR. Maybe !cookieStr.get(), instead of !cookieStr ?
Depends on: 564070
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: