Bug 34539 – Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a stub for NPN_GetAuthenticationInfo

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Gustavo Noronha (kov)]

I'm not really sure for what ports the version is correct. This was pointed out to me by Christian Perch, while discussing why the java plugin doesn't work for us:

 <chpe> PluginPackage.cpp:    m_browserFuncs.version = NP_VERSION_MINOR;
 this line looks wrong, btw.
 that define is the latest version, not the one that you actually implement, which is 19 I think (at most!)
 if any plugin actually depends on something later, it might crash :)

<kov> I sense platform differences weirdness
 chpe, I believe that is defined inside webkit
 chpe, http://trac.webkit.org/browser/trunk/WebCore/bridge/npapi.h

<chpe> yes, in npapi.h, value "24"
<chpe> but the actually exported functions are only up to pluginthreadasynccall, aka v19

<kov> right

I'm attaching a patch that conditionalizes the version definition, in the hopes that we can figure out what should be done here.

[Gustavo Noronha (kov)]

Created attachment 48060 [details]
Set the record straight regarding NPAPI compatibility, hopefully

[Eric Seidel (no email)]

Comment on attachment 48060 [details]
Set the record straight regarding NPAPI compatibility, hopefully

We need a test case, or at least a COMPILE_ASSERT in DumpRenderTree or something.

[Eric Seidel (no email)]

Comment on attachment 48060 [details]
Set the record straight regarding NPAPI compatibility, hopefully

This was just changed in another way by another patch approved today.  At least I think it was.  NPVersion().  I remember approving such a patch.

[Garth Dahlstrom]

The incorrect reporting of NPAPI compatibility basically has been blocking #33044 (applet support patch for Qt port) moving forward from Qt 4.5.3's WebKit (branched from master at sha1: ed4a2c954ba6dd6f0ec4cc0c80f2ac14c8938ba7) for the better part of a couple of months of part-time hacking.

The Java plugin on Windows invokes the unimplemented methods and mysteriously seg faults.   I reverted the changes one by one till I hit changing NP_VERSION_MINOR from 23 to 20 in npapi.h, and then things stopped seg faulting.

[Garth Dahlstrom]

Created attachment 51264 [details]
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo

I'm attaching a work in progress patch to help implement the functions for NPAPI to bring it up to v1.9 support...   It is a patch against the QtWebKit Week11 source (https://trac.webkit.org/wiki/QtWebKitJournal#WeeklyBuildWeek11) 

It builds on Windows and OSX, runs on Windows using the Java Applet patch without crashing.

I can see NPN_GetValueForURL being exercised for both Proxy and Cookies when hitting http://www.java.com/en/download/help/testvm.xml on Windows.

It could use some work in implementing a Qt agnostic means of retrieving Proxy values from the Network/Application layer.   Please feel free to take a stab at improve it.

[Kenneth Rohde Christiansen]

Should this be up for review? I think e don't want to commit this with debugging using qDebug, nor with out-commented code.

[Garth Dahlstrom]

Created attachment 51714 [details]
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo -- Fixes proxy string logic problem

I've updated the patch to fix the proxy support which had a logic error.

It looks like some corruption is happening in the get value method for cookies.    This is not seen in the debug statements of either implementation from the Qt app (this patch provides to functionally identical implementations in Qt and pure WebKit) but is observed as occasional extra junk being appended to the end of the Java cookies in the debug console, sometimes it indicates a cookie value is returned when there is no value supposed to be returned.   

Hosting test browser app:
[2960] npapi.cpp - NPN_GetValueForURL::  Proxy url: http://www.javatester.org:80/ 
[2960] npapi.cpp - NPN_GetValueForURL::Proxy fetching Proxy from Qt 
[2960] npapi.cpp - NPN_GetValueForURL::Proxy  DIRECT 
[2960] npapi.cpp - NPN_GetValueForURL::  Cookie url: http://www.javatester.org/JavaVersionDisplayApplet.class 
[2960] npapi.cpp - NPN_GetValueForURL::Cookies [  ] Length: 0 
[2960] len is set before:  0x28d20c 
[2960] len is set after:  0x28d20c 
[2960] len is:  0 

Java Console:
Java Plug-in 1.6.0_17
Using JRE version 1.6.0_17-b04 Java HotSpot(TM) Client VM
...
security: property package.definition new value com.sun.javaws,com.sun.deploy,com.sun.jnlp,org.mozilla.jss
basic: Added progress listener: sun.plugin.util.GrayBoxPainter$GrayBoxProgressListener@b166b5
network: Cache entry found [url: http://www.javatester.org/JavaVersionDisplayApplet.class, version: null]
network: Connecting http://www.javatester.org/JavaVersionDisplayApplet.class with proxy=DIRECT
network: Connecting http://www.javatester.org:80/ with proxy=DIRECT
network: Connecting http://www.javatester.org/JavaVersionDisplayApplet.class with cookie "°REC"   <--- This is junk since there is no value returned from the host Qt app as far as I can tell.
...

I suspect the problem is something to do with the statements in the if block at line 36 of the attached patch, however I'm unable to figure out what's happening (I've tried to implement it 2 ways, results are the same)...   

I'm not very good at low level C/C++, which is hence I use Qt for native compiled apps...  Maybe I'm doing something obviously wrong (i.e. does the length pointer thing look okay)?

[Garth Dahlstrom]

Created attachment 52222 [details]
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo -- Fixes junk being added to end of NPN_GetValueForURL strings

This fix basically replaces memcpy w/ strcpy.  memcpy was causing junk to be appended to the end of the strings.

Next step is to clean this up to meet WebKit code submission guidelines. 

Someone could implement some kind of callback implementation for the NPN_GetAuthenticationInfo stub, but at the moment this function is not of particular interest to me.

[Dawit A.]

Created attachment 55604 [details]
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo [Updated]

Since there is no movement on this patch and Java applet support in QtWebKit depends on it, here is a cleaned up version of the original patch that removes all the swaps all the improper use of qDebug for LOG and makes minor code adjustment.

[WebKit Review Bot]

Attachment 55604 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/plugins/npapi.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/plugins/npapi.cpp:190:  NPN_GetValueForURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/npapi.cpp:228:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/plugins/npapi.cpp:243:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/plugins/npapi.cpp:268:  NPN_SetValueForURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/npapi.cpp:272:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/plugins/npapi.cpp:297:  NPN_GetAuthenticationInfo is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/npapi.cpp:302:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 8 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 55604 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2203113

[Dawit A.]

Created attachment 55617 [details]
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo [Update II]

Though it compiles fine here for me, corrected #include statements and order to attempt and fix problems reported by the Qt compile bot...

[WebKit Review Bot]

Attachment 55617 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/plugins/npapi.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/plugins/npapi.cpp:188:  NPN_GetValueForURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/npapi.cpp:226:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/plugins/npapi.cpp:241:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/plugins/npapi.cpp:266:  NPN_SetValueForURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/npapi.cpp:270:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/plugins/npapi.cpp:295:  NPN_GetAuthenticationInfo is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/npapi.cpp:300:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 8 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 55617 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2172117

[WebKit Review Bot]

Attachment 55617 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2227119

[Dawit A.]

No clue why the qt or gtk build bots fail! Those things the error claims are undefined are defined in npapi.h. At least in my checked out version ot qtwebkit. Is the version on the build machine different ? old ? Perhaps I am missing something...

[WebKit Review Bot]

Attachment 55617 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2230156

[Dawit A.]

Created attachment 55694 [details]
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo [Update III]

Maybe third time is the charm.... Copied the conditional defines that typedef unit32 and int32 from npapi.h.

[WebKit Review Bot]

Attachment 55694 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/plugins/npapi.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/plugins/npapi.cpp:202:  NPN_GetValueForURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/npapi.cpp:240:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/plugins/npapi.cpp:255:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/plugins/npapi.cpp:280:  NPN_SetValueForURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/npapi.cpp:284:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/plugins/npapi.cpp:309:  NPN_GetAuthenticationInfo is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 7 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 55694 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2178120

[Anders Carlsson]

Comment on attachment 55694 [details]
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo [Update III]

> +
> +// FIXME: This is added to work around build failure by the qt/gtk build boats.
> +// See comments # 11,14 & 15 of https://bugs.webkit.org/show_bug.cgi?id=34539.
> +#ifndef _INT32
> +#define _INT32
> +#ifdef __LP64__
> +typedef int int32;
> +typedef unsigned int uint32;
> +#else /* __LP64__ */
> +typedef long int32;
> +typedef unsigned long uint32;
> +#endif /* __LP64__ */
> +#endif
> +
> +

This is wrong. You should use the new C99 types instead, int32_t and uint32_t.

Marking r- for this reason.

[Dawit A.]

(In reply to comment #21)
> (From update of attachment 55694 [details])
> > +
> > +// FIXME: This is added to work around build failure by the qt/gtk build boats.
> > +// See comments # 11,14 & 15 of https://bugs.webkit.org/show_bug.cgi?id=34539.
> > +#ifndef _INT32
> > +#define _INT32
> > +#ifdef __LP64__
> > +typedef int int32;
> > +typedef unsigned int uint32;
> > +#else /* __LP64__ */
> > +typedef long int32;
> > +typedef unsigned long uint32;
> > +#endif /* __LP64__ */
> > +#endif
> > +
> > +
> 
> This is wrong. You should use the new C99 types instead, int32_t and uint32_t.
> 
> Marking r- for this reason.

It is not by choice... See the function declaration for NPN{Get/Set}ValueForURL in WebCore/bridge/npapi.h. Actually the redefinition here is not necessary if the build bots (both gtk and qt) did not fail. However, if this definition is wrong here, then it is also wrong the header file I mentioned above...

[Anders Carlsson]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 55694 [details] [details])
> > > +
> > > +// FIXME: This is added to work around build failure by the qt/gtk build boats.
> > > +// See comments # 11,14 & 15 of https://bugs.webkit.org/show_bug.cgi?id=34539.
> > > +#ifndef _INT32
> > > +#define _INT32
> > > +#ifdef __LP64__
> > > +typedef int int32;
> > > +typedef unsigned int uint32;
> > > +#else /* __LP64__ */
> > > +typedef long int32;
> > > +typedef unsigned long uint32;
> > > +#endif /* __LP64__ */
> > > +#endif
> > > +
> > > +
> > 
> > This is wrong. You should use the new C99 types instead, int32_t and uint32_t.
> > 
> > Marking r- for this reason.
> 
> It is not by choice... See the function declaration for NPN{Get/Set}ValueForURL in WebCore/bridge/npapi.h. Actually the redefinition here is not necessary if the build bots (both gtk and qt) did not fail. However, if this definition is wrong here, then it is also wrong the header file I mentioned above...

The function declaration here? 

http://trac.webkit.org/browser/trunk/WebCore/bridge/npapi.h#L768

This uses uint32_t.

[Dawit A.]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (From update of attachment 55694 [details] [details] [details])
> > > > +
> > > > +// FIXME: This is added to work around build failure by the qt/gtk build boats.
> > > > +// See comments # 11,14 & 15 of https://bugs.webkit.org/show_bug.cgi?id=34539.
> > > > +#ifndef _INT32
> > > > +#define _INT32
> > > > +#ifdef __LP64__
> > > > +typedef int int32;
> > > > +typedef unsigned int uint32;
> > > > +#else /* __LP64__ */
> > > > +typedef long int32;
> > > > +typedef unsigned long uint32;
> > > > +#endif /* __LP64__ */
> > > > +#endif
> > > > +
> > > > +
> > > 
> > > This is wrong. You should use the new C99 types instead, int32_t and uint32_t.
> > > 
> > > Marking r- for this reason.
> > 
> > It is not by choice... See the function declaration for NPN{Get/Set}ValueForURL in WebCore/bridge/npapi.h. Actually the redefinition here is not necessary if the build bots (both gtk and qt) did not fail. However, if this definition is wrong here, then it is also wrong the header file I mentioned above...
> 
> The function declaration here? 

Yup...

> http://trac.webkit.org/browser/trunk/WebCore/bridge/npapi.h#L768
> 
> This uses uint32_t.

Hmmm... I see. The QtWebKit 2.0 branch contains an old npapi.h file then because the declarations I see there are completely different. Anyways, I will update the patch accordingly. Thanks...

[Dawit A.]

Created attachment 55729 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a stub for NPN_GetAuthenticationInfo [Update IV]...

Fixed patch per feedback comment #21

[WebKit Review Bot]

Attachment 55729 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/plugins/npapi.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/plugins/npapi.cpp:188:  NPN_GetValueForURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/npapi.cpp:226:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/plugins/npapi.cpp:241:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/plugins/npapi.cpp:266:  NPN_SetValueForURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/plugins/npapi.cpp:270:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/plugins/npapi.cpp:295:  NPN_GetAuthenticationInfo is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 7 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 55729 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2285327

[Dawit A.]

Can someone with karma please change the title of this bug report to reflect what the currently attached patches do ? Perhaps 

"Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a stub for NPN_GetAuthenticationInfo"

The original approach for addressing the issue is completely different from the latest patches...

[Adam Barth]

Comment on attachment 55729 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a stub for NPN_GetAuthenticationInfo [Update IV]...

This patch needs a lot of love.

WebCore/ChangeLog:6
 +          NPN_GetAuthenticationInfo,
This ChangeLog doesn't tell me any of the "why" behind this patch.  I can see the "what" by looking at the code, but I don't understand what problem we're solving with this patch.

WebCore/plugins/npapi.cpp:196
 +          KURL URL = KURL(ParsedURLString, url); // NSURL *URL = [self URLWithCString:url];
Why would |url| be a ParsedURLString?  That doesn't seem right.

WebCore/plugins/npapi.cpp:205
 +                  strcpy(*value, cookieStringUTF8.data());
please use strncpy !

WebCore/plugins/npapi.cpp:220
 +  #if defined(BUILDING_QT__)
Surely this is the wrong preprocessor check.  Also, why do we have Qt-specific code in a non-Qt-specific file?  I think we need some better kind of abstraction here.

WebCore/plugins/npapi.cpp:252
 +          memcpy(*value, proxyUTF8.data(), proxyUTF8.length());
Why do you use memcpy here but strcpy above?  One of the two must be wrong.

WebCore/plugins/npapi.cpp:289
 +      }
You should have an explicit default case in to handle garbage from our caller.

WebCore/plugins/npapi.cpp:259
 +      }
You should have an explicit default case in to handle garbage from our caller.

WebCore/plugins/npapi.cpp:271
 +              KURL URL = KURL(ParsedURLString, url); // NSURL *URL = [self URLWithCString:url];
same ParsedURLString comment here.

WebCore/plugins/npapi.cpp:273
 +                  String cookieString = String::fromUTF8(value, len);
Are you sure this is supposed to be UTF8?

WebCore/plugins/npapi.cpp:276
 +                      Frame* frame = pluginViewForInstance(instance)->parentFrame();
parentFrame?  Could be, but looks scary.

WebCore/plugins/npapi.cpp:278
 +                          setCookies(frame->document(), URL, cookieString);
URL should be in lower case.

[Dawit A.]

Created attachment 65021 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update V]...

A cleaner implementation of the aforementioned NPN functions necessary for properly handling Java applets...

[Andreas Kling]

Comment on attachment 65021 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update V]...

>WebCore/plugins/PluginView.cpp:1387
> +      if (platformGetValueForURL(variable, url, value, len, &result))
> +          return result;

platformGetValueForURL() doesn't actually modify 'result' so you're returning uninitialized data here.

>WebCore/plugins/PluginView.cpp:1393
> +          if (u.isValid()) {

If !isValid(), result should be set to an error code.

>WebCore/plugins/PluginView.cpp:1398
> +                  strncpy(*value, cookieStr.utf8().data(), cookieStrLen);

cookieStrLen should come from cookieStr.utf8().length() here.

>WebCore/plugins/PluginView.cpp:1431
> +          if (u.isValid()) {

If !isValid(), result should be set to an error code.

>WebCore/plugins/qt/PluginViewQt.cpp:71
> +  #include <QString>

Unnecessary include.

>WebCore/plugins/qt/PluginViewQt.cpp:725
> +      qDebug() << "PluginView::platformGetValueForURL:" << variable << "," << url;

Please put this inside a preprocessor conditional of some sort.

>WebCore/plugins/qt/PluginViewQt.cpp:758
> +          strncpy(*value, proxyStr.toUtf8().constData(), proxyStrLen);

Though safe in this particular case, proxyStrLen should come from proxyStr.toUtf8().length()

>WebCore/plugins/PluginView.cpp:1455
> +      LOG(Plugins, "PluginView::getAuthenticationInfo: protocol=%s, host=%d, port=%d", protocol, host, port);

"host" is const char*, so it should be %s, not %d.

Furthermore there are some coding style issues:
- Tabs should always be 4 spaces
- "type *foo" should be "type* foo"

[Dawit A.]

Created attachment 65028 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update VI]...

Corrected all of the issues pointed out in the review execpt for the "type *foo" should be "type* foo" because I cannot find where I did that and the code passes the check-webkit-style script as well. It is entirely possibe I might have missed it, but please point out where that is so it can get fixed...

[Dawit A.]

BTW, here is the link to the java applet test page:

http://www.java.com/en/download/help/testvm.xml

[Andreas Kling]

Comment on attachment 65028 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update VI]...

>WebCore/plugins/PluginView.cpp:1386
> +      NPError result;
Should probably be initialized to NPERR_GENERIC_ERROR like in setValueForURL() below.

>WebCore/plugins/PluginView.cpp:1393
> +          if (u.isValid()) {
Missing an "else" that sets result to NPERR_INVALID_URL.

>WebCore/plugins/PluginView.cpp:1397
> +                  strncpy(*value, cookieStr.utf8().data(), cookieStr.length() + 1);
cookieStr.utf8() may be of a different length than cookieStr.length() + 1.
You should use the length() of cookieStr.utf8() instead of cookieStr itself.

>WebCore/plugins/PluginView.cpp:1432
> +          if (u.isValid()) {
Missing an "else" that sets result to NPERR_INVALID_URL.

>WebCore/plugins/PluginView.cpp:1452
> +  NPError PluginView::getAuthenticationInfo(const char* protocol, const char* host, int32_t port, const char* scheme, const char *realm, char** username, uint32_t* ulen, char** password, uint32_t* plen)
Coding style, "const char *realm" should be "const char* realm". This error is repeated a number of times in the patch.

>WebCore/plugins/qt/PluginViewQt.cpp:754
> +          *value = static_cast<char*>(NPN_MemAlloc(proxyStr.length() + 1));
If NPN_MemAlloc() fails, we should return NPN_OUT_OF_MEMORY_ERROR.

>WebCore/plugins/qt/PluginViewQt.cpp:755
> +          strncpy(*value, proxyStr.toUtf8().constData(), proxyStr.length() + 1);
> +          if (len)
> +              *len = strlen(*value);
Again mixing the UTF8 data with the Unicode length.

This should be something like:

QByteArray utf8 = proxyStr.toUtf8();
memcpy(*value, utf8.constData(), utf8.length());
if (len)
    *len = utf8.length();

Furthermore, are we really supposed to return an error code when setting an empty cookie string?
Same question for when getting the cookie string returns "".

[Dawit A.]

Created attachment 65046 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update VII]...

Addressed all the issues mentioned in comment #34. Let us see if the 3rd time is the charm...

[Andreas Kling]

Comment on attachment 65046 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update VII]...

>WebCore/plugins/PluginDebug.cpp:171
> +      default: return "Unkown variable";
Spelling mistake, should be "Unknown variable".

>WebCore/plugins/PluginView.cpp:1387
> +      if (platformGetValueForURL(variable, url, value, len, &result) || (result != NPERR_NO_ERROR))
The " || (result != NPERR_NO_ERROR)" is not needed since platformGetValueForURL() returns true after setting either result code.

>WebCore/plugins/PluginView.cpp:1401
> +                  } else {
Coding style, should be "} else"

>WebCore/plugins/PluginView.cpp:1405
> +          } else {
Ditto.

>WebCore/plugins/PluginView.cpp:1416
> +          LOG(Plugins, "PluginView::getValueForURL(%s): Unknown variable type.", prettyNameForNPNURLVariable(variable).data());
This will log "PluginView::getValueForURL(Unknown variable): Unknown variable type." which isn't very helpful.


>WebCore/plugins/PluginView.cpp:1429
> +      if (platformSetValueForURL(variable, url, value, len, &result) || (result != NPERR_GENERIC_ERROR))
Same as above. platformSetValueForURL() should return true if it sets any result code IMO.

>WebCore/plugins/PluginView.cpp:1440
> +          } else {
Coding style, should be "} else"

>WebCore/plugins/PluginView.cpp:1461
> +      if (platformGetAuthenticationInfo(protocol, host, port, scheme, realm, username, ulen, password, plen, &result) || (result != NPERR_GENERIC_ERROR))
Same comment as earlier about the superfluous " || (result != NPERR_GENERIC_ERROR)"

>WebCore/plugins/qt/PluginViewQt.cpp:762
> +          } else {
Coding style, should be "} else"

Overall though, it's starting to look pretty good. I'll let an authorized reviewer take it from here. :-)

[Dawit A.]

(In reply to comment #36)
> (From update of attachment 65046 [details])
> >WebCore/plugins/PluginDebug.cpp:171
> > +      default: return "Unkown variable";
> Spelling mistake, should be "Unknown variable".
> 
> >WebCore/plugins/PluginView.cpp:1387
> > +      if (platformGetValueForURL(variable, url, value, len, &result) || (result != NPERR_NO_ERROR))
> The " || (result != NPERR_NO_ERROR)" is not needed since platformGetValueForURL() returns true after setting either result code.

For the record I did that because it did not feel right to return true while setting an error from platformGetValueForURL, but I realized
afterwards that the return value from these "platform" functions is to indicate whether or not the request got handled ; not to indicate
an error condition occurred. Anyhow, fixed...

> >WebCore/plugins/PluginView.cpp:1401
> > +                  } else {
> Coding style, should be "} else"

No clue why that would be seen as good coding practise in the first place, but whatever. One can easily
shoot oneself in the foot in a deep nested if/else statement while following that practise. Still fixed.

> >WebCore/plugins/PluginView.cpp:1416
> > +          LOG(Plugins, "PluginView::getValueForURL(%s): Unknown variable type.", prettyNameForNPNURLVariable(variable).data());
> This will log "PluginView::getValueForURL(Unknown variable): Unknown variable type." which isn't very helpful.

Yes, that is indeed useless. Fixed.

I will post the final patch... Java applet support should work fine in QtWebKit once this patch gets accepted...

[Dawit A.]

Created attachment 65051 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final Update]...

Final update based on review in comment #36.

[Simon Hausmann]

Comment on attachment 65051 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final Update]...

> WebCore/ChangeLog:3
> +        Reviewed by Andreas Kling 
Until the patch is landed this line should continue to say NOBODY (OOPS!).

> :722
> +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result)
Shouldn't this function use the QNetworkAccessManager's proxy settings instead of the application settings?

> WebCore/plugins/symbian/PluginViewSymbian.cpp:341
> +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result)
The current code duplication between the views sucks a bit, but in this "Symbian" implementation we should use the Qt bits and pieces. Qt is the system toolkit on Symbian.

I think overall this patch looks okay now, I agree with Andreas. However there is one thing missing: Test coverage for the new code. At least the cookie bits should be relatively easy to cover with the test netscape plugin.

I'm going to say r- primarily because of missing test coverage.

[Dawit A.]

(In reply to comment #39)
> (From update of attachment 65051 [details])
> > WebCore/ChangeLog:3
> > +        Reviewed by Andreas Kling 
> Until the patch is landed this line should continue to say NOBODY (OOPS!).
> 
> > :722
> > +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result)
> Shouldn't this function use the QNetworkAccessManager's proxy settings instead of the application settings?
> 
> > WebCore/plugins/symbian/PluginViewSymbian.cpp:341
> > +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result)
> The current code duplication between the views sucks a bit, but in this "Symbian" implementation we should use the Qt bits and pieces. Qt is the system toolkit on Symbian.
> 
> I think overall this patch looks okay now, I agree with Andreas. However there is one thing missing: Test coverage for the new code. At least the cookie bits should be relatively easy to cover with the test netscape plugin.
> 
> I'm going to say r- primarily because of missing test coverage.

(In reply to comment #39)
> (From update of attachment 65051 [details])
> > WebCore/ChangeLog:3
> > +        Reviewed by Andreas Kling 
> Until the patch is landed this line should continue to say NOBODY (OOPS!).
> 
> > :722
> > +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result)
> Shouldn't this function use the QNetworkAccessManager's proxy settings instead of the application settings?

Probably, but I am not entirely sure how I can access the QNetworkAccessManager's proxy from this implementation...

> > WebCore/plugins/symbian/PluginViewSymbian.cpp:341
> > +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result)
> The current code duplication between the views sucks a bit, but in this 
> "Symbian" implementation we should use the Qt bits and pieces. Qt is the system 
> toolkit on Symbian.

Well no generic way to access proxy information means that this is the only viable approach at the moment.

> I think overall this patch looks okay now, I agree with Andreas. However there is 
> one thing missing: Test coverage for the new code. At least the cookie bits 
> should be relatively easy to cover with the test netscape plugin.
>
> I'm going to say r- primarily because of missing test coverage.

Where does the test coverage go ? LayoutTests/plugin ?

[Dawit A.]

Created attachment 66372 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update IX]...

* Corrected the Reviewed By entry in the ChangeLog file.
* Use QNetworkAccessManager's proxy instead the application's.
* Copied the platformGetValueForURL implementation from qt port to symbian port.

I cannot figure out where and how the requested test coverage for the new code is supposed to be implemented though...

[Early Warning System Bot]

Attachment 66372 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3909055

[Dawit A.]

Created attachment 66374 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update X]...

*no comment*

[Simon Hausmann]

(In reply to comment #40)
> > I'm going to say r- primarily because of missing test coverage.
> 
> Where does the test coverage go ? LayoutTests/plugin ?

Yep. See also WebKitTools/DumpRenderTree/TestNetscapePlugin for the test netscape plugin :)

[Andreas Kling]

Comment on attachment 66374 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update X]...

This page is rather interesting: https://developer.mozilla.org/en/NPN_GetValueForURL

About the "value" argument:
"Out parameter. The browser passes back the requested information. If the function succeeds, the result buffer will be allocated with NPN_MemAlloc; the plugin is responsible for freeing the buffer. Note: the value may have internal NULL bytes and may not be NULL-terminated."

I think we should be nice and null-terminate the returned value, it won't hurt and will shield against broken plugins.

> +                *value = static_cast<char*>(NPN_MemAlloc(cookieStr.length()));

Allocate cookieStr.length() + 1 bytes.

> +                    strncpy(*value, cookieStr.data(), cookieStr.length());

Use memcpy() instead of strncpy(). (And cookieStr.length() + 1.)

The above two comments apply to all cases where you're NPN_MemAlloc()ing a char buffer and copying something to it.

> +        LOG(Plugins, "PluginView::getValueForURL(%s): Need to implement PluginView::platformGetValueForURL!", prettyNameForNPNURLVariable(variable).data());
> ...
> +        LOG(Plugins, "PluginView::getValueForURL: %s", prettyNameForNPNURLVariable(variable).data());

Nit: inconsistent formatting, second case should be something like:
LOG(Plugins, "PluginView::getValueForURL(%s): Unhandled variable", prettyNameForNPNURLVariable(variable).data());

r- because of lack of test, that's the important missing piece.

[Dawit A.]

(In reply to comment #45)
> (From update of attachment 66374 [details])
> This page is rather interesting: https://developer.mozilla.org/en/NPN_GetValueForURL
> 
> About the "value" argument:
> "Out parameter. The browser passes back the requested information. If the function succeeds, the result buffer will be allocated with NPN_MemAlloc; the plugin is responsible for freeing the buffer. Note: the value may have internal NULL bytes and may not be NULL-terminated."
> 
> I think we should be nice and null-terminate the returned value, it won't hurt and will shield against broken plugins.
> 
> > +                *value = static_cast<char*>(NPN_MemAlloc(cookieStr.length()));
> 
> Allocate cookieStr.length() + 1 bytes.
> 
> > +                    strncpy(*value, cookieStr.data(), cookieStr.length());
> 
> Use memcpy() instead of strncpy(). (And cookieStr.length() + 1.)
> 
> The above two comments apply to all cases where you're NPN_MemAlloc()ing a char buffer and copying something to it.

I have already changed this locally and is waiting to be committed once I figure out on how to add a test coverage for it.

> > +        LOG(Plugins, "PluginView::getValueForURL(%s): Need to implement PluginView::platformGetValueForURL!", prettyNameForNPNURLVariable(variable).data());
> > ...
> > +        LOG(Plugins, "PluginView::getValueForURL: %s", prettyNameForNPNURLVariable(variable).data());
> 
> Nit: inconsistent formatting, second case should be something like:
> LOG(Plugins, "PluginView::getValueForURL(%s): Unhandled variable", prettyNameForNPNURLVariable(variable).data());

This was done because of your own feedback in comment #36. If it's changed to what you are suggesting here, it would read:

"PluginView::getValueForURL(Unknown variable): Unhandled variable"

which is not much different than it was before comment #36. Instead it should correctly, as pointed out by yourself in comment #36, read:

"PluginView::getValueForURL: Unknown variable"

> r- because of lack of test, that's the important missing piece.

Still trying to figure that out... No clue when that will happen ; so this patch will have to sit and wait until I figure that out or someone else with better understanding of how the test coverages work provides one for it...

[Andreas Kling]

(In reply to comment #46)
> This was done because of your own feedback in comment #36. If it's changed to what you are suggesting here, it would read:

Oops! Sorry about the schizophrenia. I again agree with my past self. ;-)

[Dawit A.]

Created attachment 68348 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #2]...

With the exception of the requested test coverage, this is the final impementation of the missing NPN functions that are needed to ensure Java applets work in QtWebKit. I cannot figure out how to create the test coverage for this change ; so I have given up on doing so now... Perhaps someone more familiar will be able to add the test coverage...

[Early Warning System Bot]

Attachment 68348 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4058102

[Dawit A.]

Created attachment 68411 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #3]...

Fixed compilation issue caused by layering violation in the previous patch...

[Anders Carlsson]

Comment on attachment 68411 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #3]...

I don't like the platformGetValueForURL/platformSetValueForURL/platformGetAuthenticationInfo approach.

In http://trac.webkit.org/changeset/68951 I landed ProxyServer.h which has a proxyServersForURL function. If Qt provided an implementation of that it could be used in PluginView.cpp (I plan to do the same thing for getAuthenticationInfo).

Does that make sense?

[Dawit A.]

(In reply to comment #51)
> (From update of attachment 68411 [details])
> I don't like the platformGetValueForURL/platformSetValueForURL/platformGetAuthenticationInfo approach.
> 
> In http://trac.webkit.org/changeset/68951 I landed ProxyServer.h which has a proxyServersForURL function. If Qt provided an implementation of that it could be used in PluginView.cpp (I plan to do the same thing for getAuthenticationInfo).
> 
> Does that make sense?

Yes. If there is a platform independent way of accessing proxy and authentication information, then there is no need for keeping the platform specific methods...

[Dawit A.]

Created attachment 69582 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #4]...

Per review in comment #51, changed code to use WebCore::proxyServersForURL from ProxyServer.h for obtaining proxy information and removed the platform* functions.

[Dawit A.]

Created attachment 70193 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #5]...

Minor changes...

[Anders Carlsson]

Comment on attachment 70193 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #5]...

View in context: https://bugs.webkit.org/attachment.cgi?id=70193&action=review

Looks good otherwise!

> WebCore/platform/network/ProxyServer.h:71
> +Vector<ProxyServer> proxyServersForURL(const KURL&, const NetworkingContext* context = 0);

I think it's a bit weird to make this default to null. Couldn't we instead always require the argument to be passed?

[Dawit A.]

(In reply to comment #55)
> (From update of attachment 70193 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70193&action=review
> 
> Looks good otherwise!
> 
> > WebCore/platform/network/ProxyServer.h:71
> > +Vector<ProxyServer> proxyServersForURL(const KURL&, const NetworkingContext* context = 0);
> 
> I think it's a bit weird to make this default to null. Couldn't we instead always require the argument to be passed?

Yes, there is no reason why that cannot be the case. I did that to simply avoid breaking any of the other implementations...

[Dawit A.]

Created attachment 70343 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #6]...

Removed the default value from proxyServersForURL's second parameter...

[Dawit A.]

Comment on attachment 70193 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #5]...

Removed r+

[Early Warning System Bot]

Attachment 70343 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4320006

[Dawit A.]

Created attachment 70346 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #7]...

Attempt to fix WebKit2 compile issues for qt port...

[Andreas Kling]

Comment on attachment 70346 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #7]...

View in context: https://bugs.webkit.org/attachment.cgi?id=70346&action=review

Looks good, but please fix the following things.

> WebCore/platform/network/ProxyServer.h:71
> +Vector<ProxyServer> proxyServersForURL(const KURL&, const NetworkingContext* context);

There's no need for the NetworkingContext* to have a variable name here.

> WebCore/platform/network/qt/ProxyServerQt.cpp:44
> +    if (context) {

This function nests unnecessarily, you should use early return style, i.e:
if (!context)
    return servers;
And the same for !accessManager and !proxyFactory

> WebKit2/WebProcess/Plugins/PluginView.cpp:847
> +    const NetworkingContext* netContext = frameLoader ? frameLoader->networkingContext() : 0;
> +    Vector<ProxyServer> proxyServers = proxyServersForURL(KURL(KURL(), urlString), netContext);

s/netContext/context/

[Dawit A.]

Created attachment 70372 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #8]...

Addressed the issues from review in comment #61...

[Andreas Kling]

Comment on attachment 70372 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #8]...

View in context: https://bugs.webkit.org/attachment.cgi?id=70372&action=review

r- for the ChangeLog issue, the rest is cosmetics.

> WebCore/ChangeLog:6
> +        Fix compile error when bulding webkit's Qt port using the "--v8" option.
> +        https://bugs.webkit.org/show_bug.cgi?id=47455

Oops, looks like you have some extra ChangeLog lying around.

> WebCore/platform/network/qt/ProxyServerQt.cpp:47
> +    if (proxyFactory) {

if (!proxyFactory)
    return servers;

> WebKit2/WebProcess/Plugins/PluginView.cpp:847
> +    const NetworkingContext* netContext = frameLoader ? frameLoader->networkingContext() : 0;
> +    Vector<ProxyServer> proxyServers = proxyServersForURL(KURL(KURL(), urlString), netContext);

Nit: "netContext" -> "context"

[Dawit A.]

Created attachment 70373 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #9]...

Corrected the WebCore/ChangeLog patch...

[Andreas Kling]

Comment on attachment 70373 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #9]...

r=me, let's land this puppy.

[Dawit A.]

Created attachment 70755 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #10]...

* Factored out the code that obtains the frame to use when calling the cookie and proxy related functions.
* Added more checks to make sure invalid pointers are not used.

[Dawit A.]

Comment on attachment 70373 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #9]...

removed r+

[Early Warning System Bot]

Attachment 70755 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4426034

[Dawit A.]

Created attachment 70765 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #11]...

Fixed build problem with previous commit...

[Andreas Kling]

Blocking QtWebKit 2.1 on this. See https://bugs.webkit.org/show_bug.cgi?id=33044#c30 for more information.

[WebKit Commit Bot]

Comment on attachment 70765 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #11]...

Clearing flags on attachment: 70765

Committed r69808: <http://trac.webkit.org/changeset/69808>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

(In reply to comment #70)
> Blocking QtWebKit 2.1 on this. See https://bugs.webkit.org/show_bug.cgi?id=33044#c30 for more information.

Following this logic (bug 33044#c30), shouldn't bug #38911 be cherry-picked as well?

[Ademar Reis]

(In reply to comment #70)
> Blocking QtWebKit 2.1 on this. See https://bugs.webkit.org/show_bug.cgi?id=33044#c30 for more information.

The complexity to backport/cherry-pick this change to qtwebkit-2.1 is way too big and risky at this point. We need tons of other changes, including bug 47022 and major refactorings such as bug 42292.

I'm giving up on the cherry-pick. If really needed (I'll discuss it with the stakeholders), we should create a new bug/task for the backport.

[Ademar Reis]

Revision r69808 cherry-picked into qtwebkit-2.1 with commit 3c8023c <http://gitorious.org/webkit/qtwebkit/commit/3c8023c>

[Luiz Agostini]

*** Bug 48213 has been marked as a duplicate of this bug. ***