Description
Garth Dahlstrom
2009-12-29 23:07:00 PST
Comment on attachment 45643 [details] Qt 4.5.3 Windows Java 1.6.0u17 Applet patch by John Spicer I will do a very raw review. Please run check-webkit-style on the patch if you haven't done so already. Basically this patch should be two, one for the Qt specific part and one for the Windows/WebCore specific part. > --- E:/Qt/4.5.3-shared/src/3rdparty/webkit/WebCore/plugins/win/PluginDatabaseWin.cpp.org Tue Sep 29 06:04:12 2009 > +++ E:/Qt/4.5.3-shared/src/3rdparty/webkit/WebCore/plugins/win/PluginDatabaseWin.cpp Mon Dec 21 04:01:39 2009 I wonder what Safari and Chrome uses on Windows for supporting Java. > @@ -33,6 +33,8 @@ > #include <windows.h> > #include <shlwapi.h> > > +#include <QDebug> This file (PluginDatabaseWin.cpp) is not Qt specific so please try avoiding using non-WebCore/Windows method calls, thus the include here is wrong > + > #if COMPILER(MINGW) > #define _countof(x) (sizeof(x)/sizeof(x[0])) > #endif > @@ -325,6 +327,69 @@ > directories.append(macromediaDirectoryStr); > } > > +static inline void addJavaPluginDirectory(Vector<String>& directories) > +{ > + qDebug() << "PluginDatabase::addJavaPluginDirectories"; Same thing here > + HKEY key; > + HRESULT result = RegOpenKeyEx(HKEY_LOCAL_MACHINE, TEXT("Software\\JavaSoft\\Java Plug-in"), 0, KEY_READ, &key); > + if (result != ERROR_SUCCESS) > + return; > + > + WCHAR name[128]; > + FILETIME lastModified; > + > + Vector<int> latestJavaVersion; > + String latestJavaVersionString; > + > + qDebug() << "- checking Java Plugin versions"; > + // Enumerate subkeys > + for (int i = 0;; i++) { > + DWORD nameLen = sizeof(name) / sizeof(WCHAR); > + result = RegEnumKeyExW(key, i, name, &nameLen, 0, 0, 0, &lastModified); > + > + if (result != ERROR_SUCCESS) > + break; > + > + Vector<int> javaVersion = parseVersionString(String(name, nameLen)); > + if (compareVersions(javaVersion, latestJavaVersion)) { > + latestJavaVersion = javaVersion; > + latestJavaVersionString = String(name, nameLen); > + qDebug() << "- setting latest Java Plugin version to " << latestJavaVersionString; > + } > + } > + > + if (!latestJavaVersionString.isNull()) { > + DWORD type; > + WCHAR javaInstallPathStr[_MAX_PATH]; > + DWORD javaInstallPathSize = sizeof(javaInstallPathStr); > + DWORD useNewPluginValue; > + DWORD useNewPluginSize; > + > + String javaPluginKeyPath = "Software\\JavaSoft\\Java Plug-in\\" + latestJavaVersionString; > + result = SHGetValue(HKEY_LOCAL_MACHINE, javaPluginKeyPath.charactersWithNullTermination(), TEXT("UseNewJavaPlugin"), &type,(LPVOID)&useNewPluginValue, &useNewPluginSize); > + if (result == ERROR_SUCCESS) { > + if (useNewPluginValue == 1) { > + result = SHGetValue(HKEY_LOCAL_MACHINE, javaPluginKeyPath.charactersWithNullTermination(), TEXT("JavaHome"), &type, (LPBYTE)javaInstallPathStr, &javaInstallPathSize); > + if (result == ERROR_SUCCESS) { > + String javaPluginDirectory = String(javaInstallPathStr, javaInstallPathSize / sizeof(WCHAR) - 1) + "\\bin\\new_plugin"; > + directories.append(javaPluginDirectory); > + qDebug() << "- adding Java Plugin directory: " << javaPluginDirectory; > + } else { > + qDebug() << "Error: failed to read JavaHome value (" << result << ")"; > + } > + } else { > + qDebug() << "Error: failed to find a 'new' Java Plugin"; > + } Yes the coding style is wrong, so please run the check-webkit-style :-) > + } else { > + qDebug() << "Error: failed to read 'UseNewJavaPlugin' value (" << result << ")"; > + } > + } else { > + qDebug("ERROR: failed to find a java version!!!"); > + } > + > + RegCloseKey(key); > +} > + > Vector<String> PluginDatabase::defaultPluginDirectories() > { > Vector<String> directories; > @@ -337,7 +402,7 @@ > addMozillaPluginDirectories(directories); > addWindowsMediaPlayerPluginDirectory(directories); > addMacromediaPluginDirectories(directories); > - > + addJavaPluginDirectory(directories); > return directories; > } > > --- E:/Qt/4.5.3-shared/src/3rdparty/webkit/WebCore/plugins/win/PluginPackageWin.cpp.org Tue Sep 29 06:04:12 2009 > +++ E:/Qt/4.5.3-shared/src/3rdparty/webkit/WebCore/plugins/win/PluginPackageWin.cpp Mon Dec 21 04:01:39 2009 > @@ -38,6 +38,8 @@ > #include <wtf/OwnArrayPtr.h> > #include <shlwapi.h> > > +#include <QDebug> > + Do not use QDebug here > namespace WebCore { > > static String getVersionInfo(const LPVOID versionInfoData, const String& info) > @@ -223,6 +225,7 @@ > > bool PluginPackage::load() > { > + qDebug() << "Loading plugin: " << this->m_path; > if (m_freeLibraryTimer.isActive()) { > ASSERT(m_module); > m_freeLibraryTimer.stop(); > --- E:/Qt/4.5.3-shared/src/3rdparty/webkit/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp.org Tue Sep 29 06:04:14 2009 > +++ E:/Qt/4.5.3-shared/src/3rdparty/webkit/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp Tue Dec 22 22:22:14 2009 > @@ -58,6 +58,8 @@ > #include "qwebhistoryinterface.h" > #include "qwebpluginfactory.h" > > +#include <QtDebug> Here it is OK to use. > + > #include <qfileinfo.h> > > #include <QCoreApplication> > @@ -199,7 +201,7 @@ > return true; > } > > -void FrameLoaderClientQt::savePlatformDataToCachedPage(CachedPage*) > +void FrameLoaderClientQt::savePlatformDataToCachedPage(CachedPage*) > { > notImplemented(); > } > @@ -1076,8 +1078,8 @@ > Widget* FrameLoaderClientQt::createPlugin(const IntSize& pluginSize, Element* element, const KURL& url, const Vector<String>& paramNames, > const Vector<String>& paramValues, const String& mimeType, bool loadManually) > { > -// qDebug()<<"------ Creating plugin in FrameLoaderClientQt::createPlugin for "<<url.prettyURL() << mimeType; > -// qDebug()<<"------\t url = "<<url.prettyURL(); > + qDebug()<<"------ Creating plugin in FrameLoaderClientQt::createPlugin for "<<url.prettyURL() << mimeType; > + qDebug()<<"------\t url = "<<url.prettyURL(); > > if (!m_webFrame) > return 0; > @@ -1148,6 +1150,7 @@ > } else { // NPAPI Plugins > PluginView* pluginView = PluginView::create(m_frame, pluginSize, element, url, > paramNames, paramValues, mimeType, loadManually); > + qDebug() << "Create the PluginView: " << pluginView; > return pluginView; > } > > @@ -1156,16 +1159,19 @@ > > void FrameLoaderClientQt::redirectDataToPlugin(Widget* pluginWidget) > { > + qDebug() << "FrameLoaderClientQt::redirectDataToPlugin"; > ASSERT(!m_pluginView); > m_pluginView = static_cast<PluginView*>(pluginWidget); > m_hasSentResponseToPlugin = false; > } > > -Widget* FrameLoaderClientQt::createJavaAppletWidget(const IntSize&, Element*, const KURL& baseURL, > +Widget* FrameLoaderClientQt::createJavaAppletWidget(const IntSize& pluginSize, Element* element, const KURL& baseURL, > const Vector<String>& paramNames, const Vector<String>& paramValues) > { > - notImplemented(); > - return 0; > + qDebug() << "createJavaAppletWidget"; > + //notImplemented(); > + //return 0; > + return createPlugin(pluginSize, element, baseURL, paramNames, paramValues, "application/x-java-applet", true); > } > > String FrameLoaderClientQt::overrideMediaType() const Looks good to me, but please remove the qDebug() and fix the coding style. What problems did you face with 4.6.0 integration? It looks like the PluginDatabaseWin patch should merge easily. Created attachment 47837 [details]
Qt 4.5.3 Windows Java 1.6.0u17 Applet patch -- Updated: Feb 1, 2010
@Kenneth
Here is the the patch run through check-webkit-style and with qDebug()'s replaced with LOG statements (hoping that is the right thing to replace qDebug's with). Note: I did not fix all of the check-webkit-style issues for the class, there are numerous issues that were present in the original classes, I just made sure any new code does not add to the check-webkit-style issues list...
@Girish
patching file src/3rdparty/webkit/WebCore/plugins/win/PluginDatabaseWin.cpp
Hunk #1 succeeded at 463 with fuzz 2 (offset 138 lines).
Hunk #2 FAILED at 535.
1 out of 2 hunks FAILED -- saving rejects to file src/3rdparty/webkit/WebCore/plugins/win/PluginDatabaseWin.cpp.rej
patching file src/3rdparty/webkit/WebCore/plugins/win/PluginPackageWin.cpp
Hunk #1 FAILED at 223.
1 out of 1 hunk FAILED -- saving rejects to file src/3rdparty/webkit/WebCore/plugins/win/PluginPackageWin.cpp.rej
patching file src/3rdparty/webkit/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp
Hunk #1 succeeded at 1215 with fuzz 1 (offset 139 lines).
Hunk #2 succeeded at 1305 with fuzz 2 (offset 157 lines).
Hunk #3 FAILED at 1314.
1 out of 3 hunks FAILED -- saving rejects to file src/3rdparty/webkit/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp.rej
(In reply to comment #3) > @Kenneth > Here is the the patch run through check-webkit-style and with qDebug()'s > replaced with LOG statements (hoping that is the right thing to replace > qDebug's with). Note: I did not fix all of the check-webkit-style issues for > the class, there are numerous issues that were present in the original classes, > I just made sure any new code does not add to the check-webkit-style issues > list... That is why you can make check-webkit-style only check what you changed and not the file itself. Please check its arguments. If you use git you can do check-webkit-style --git-index=<GIT-HASH> (In reply to comment #4) > (In reply to comment #3) > > @Kenneth > > Here is the the patch run through check-webkit-style and with qDebug()'s > > replaced with LOG statements (hoping that is the right thing to replace > > qDebug's with). Note: I did not fix all of the check-webkit-style issues for > > the class, there are numerous issues that were present in the original classes, > > I just made sure any new code does not add to the check-webkit-style issues > > list... > > That is why you can make check-webkit-style only check what you changed and not > the file itself. Please check its arguments. If you use git you can do > > check-webkit-style --git-index=<GIT-HASH> ~/mnt/qtwebkit/WebKitTools/Scripts/check-webkit-style --git-index=eb3afcbfb4006de4015047555cb256fcde93b954 FrameLoaderClientQt.cpp Syntax: check-webkit-style [--verbose=#] [--git-commit=<SingleCommit>] [--output=vs7] [--filter=-x,+y,...] [file] ... It doesn't matter anyway, there weren't that many style violations to track them by hand looking by running check-webkit-style against the original and then the new.
> ~/mnt/qtwebkit/WebKitTools/Scripts/check-webkit-style
> --git-index=eb3afcbfb4006de4015047555cb256fcde93b954 FrameLoaderClientQt.cpp
>
> Syntax: check-webkit-style [--verbose=#] [--git-commit=<SingleCommit>]
> [--output=vs7]
> [--filter=-x,+y,...] [file] ...
>
> It doesn't matter anyway, there weren't that many style violations to track
> them by hand looking by running check-webkit-style against the original and
> then the new.
Ah sorry, use --git-commit=<hash>, it is prepare-ChangeLog which has --git-index
(In reply to comment #6) > Ah sorry, use --git-commit=<hash>, it is prepare-ChangeLog which has > --git-index No worries, I tried that one also... Perhaps I used it wrong, or perhaps it doesn't work because the WebKit source tree that is shipped with the Qt 4.5.3 source is an export and not actually under Git. ~/mnt/qtwebkit/WebKitTools/Scripts/check-webkit-style --git-commit=eb3afcbfb4006de4015047555cb256fcde93b954 FrameLoaderClientQt.cpp ERROR: It is not possible to check files and a specific commit at the same time.
> ~/mnt/qtwebkit/WebKitTools/Scripts/check-webkit-style
> --git-commit=eb3afcbfb4006de4015047555cb256fcde93b954 FrameLoaderClientQt.cpp
> ERROR: It is not possible to check files and a specific commit at the same
> time.
The check-webkit-style told you exactly what you did wrong, please leave our the file argument
check-webkit-style --git-commit=eb3afcbfb4006de4015047555cb256fcde93b954
(In reply to comment #8) > The check-webkit-style told you exactly what you did wrong, please leave our > the file argument > > check-webkit-style --git-commit=eb3afcbfb4006de4015047555cb256fcde93b954 ~/mnt/qtwebkit/WebKitTools/Scripts/check-webkit-style --git-commit=eb3afcbfb4006de4015047555cb256fcde93b954 fatal: Not a git repository (or any of the parent directories): .git Right so it fails because as I mentioned it's an export of the source and not actual Git tree. It looks as though in order to get Java Applets access to both cookies and proxy info, the hosting app must implement NPAPI hooks for NPN_GetValueForURL and NPN_SetValueForURL. Qt 4.6's webkit snapshot has those methods defined in it's WebCore/bridge/npapi.h header, while Qt 4.5 does not.... So it would be very helpful to have this ported up to 4.6 (rather then back port a newer NPAPI to a dead branch). References: * http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6796470 * https://developer.mozilla.org/en/Gecko_Plugin_API_Reference * https://developer.mozilla.org/en/NPN_GetValueForURL * https://developer.mozilla.org/en/NPN_SetValueForURL Created attachment 48989 [details]
Qt 4.6.2 Windows Java 1.6.0u17 Applet patch (Segfaults upon Java loading) -- Updated: Feb 18, 2010
I've tried to port the previous patch up to Qt 4.6.2.
Unfortunately, it seg faults whenever a page with Java is hit.
Pretty sure the problem lies with either how FrameLoaderClientQt::createJavaAppletWidget (\src\3rdparty\webkit\WebKit\qt\WebCoreSupport\FrameLoaderClientQt.cpp:1323) is creating it's PassRefPtr<Widget> return value or with the caller of createJavaAppletWidget whom mishandles the value. In Qt 4.5.3 this method returned *Widget and worked.
Of note: returning like so on 1330 causes a Seg Fault:
return createPlugin(pluginSize, element, baseURL, paramNames, paramValues, "application/x-java-applet", true);
Calling
createPlugin(pluginSize, element, baseURL, paramNames, paramValues, "application/x-java-applet", true);
return 0;
Doesn't work (will not even spark a JVM in the tray), but doesn't seg fault either.
This is what is shown in gdb at crash during a seg fault...
Thread 1 (thread 2712.0xac):
#0 0x00000000 in ?? ()
#1 0x6da936d6 in npjp2!_Java_sun_plugin2_main_server_MozillaPlugin_getProxy0@20 () from C:\Program Files (x86)\Java\jre6\bin\new_plugin\npjp2.dll
#2 0x48259e37 in ?? ()
#3 0x00000000 in ?? ()
Obviously I'm struggling here, I sure could use a hand from someone with more C++ troubleshooting expertise. [I can post my Qt 4.6.2 build tree (w/ full debug symbols enabled), if that would be helpful.]
Created attachment 49066 [details]
1 line patch that enables Java Applet support in Arora-0.10.2
I'm adding the patch needed to turn on Java Applets in Arora browser. When Arora is built against Qt 4.5.3 on Windows w/ Java Applet patch -> jar-based applets work, when built against Qt 4.6.0 on Windows /w segfault Java Applet patch -> Arora segfaults.
I know building Qt from scratch can be painfully slow, so I've posted my applet-segfaulting build of Qt 4.6.2 for Windows. It is available at: http://www.northern.ca/projects/qt/Qt4.6.2-E-Drive-2010-02-19.zip (1.5 GB) Notes: * It includes the Qt 4.6.2 Java Applet patch which currently causes segfaults when applets start * Qt Debug Target's WebKit libraries are built with full debugging symbols (CFLAGS,CXXFLAGS = -g ... ) * It has a custom builder batch file (E:\qt\build-qt.cmd) to help facilitate building/rebuilding and includes SSL libraries. Please follow the QtWebKit bug reporting guidelines when reporting bugs. See http://trac.webkit.org/wiki/QtWebKitBugs Specifically: - The 'QtWebKit' component should be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit http://trac.webkit.org/wiki/QtWebKitBugs#Component Created attachment 51212 [details] patch against the QtWebKit week9 / week10, fixes the seg fault by reverting the NPAPI API minor number - Note it works on Windows, but returns a blue plug-in brick on OSX The seg fault is caused by the commit: Anders Carlsson <andersca@apple.com> 2009-02-05 15:58:34 (sha1 3e01afdf183c8248ed49e9ba8c27573c470a19ca) The NP_VERSION_MINOR version number from 20 to 23 and in a later commit to 24 (sha1 dd28484e008819aa59c5549a703ecee8bcbe4f22). In c0baac9e0ae1ed075e2ff78b4bbe0d1b6872cb35, an Objective-C implementation for NPN_GetValueForURL, NPN_SetValueForURL() and NPN_GetAuthenticationInfo() would be committed to ./WebKit/mac/Plugins/npapi.mm, but no corresponding C++ implementation of those methods was added to ./WebCore/plugins/npapi.cpp. What this means is when QtWebKit starts Java or any other NPAPI plug-in it returns an NPVersion value for an API level (Gecko NPAPI v1.9) that it does not completely implement. When Java tries to invoke those unimplemented callback methods, everything seg faults and you are left with a worthless stack of Java/Kernel threads with no debug info at all. To get around this the attached patch, in addition to enabling Java, reverts the NP_VERSION_MINOR back to 20, which tells Java not to invoke the unimplemented callbacks. Adding depends to a related bug about NP_VERSION_MINOR being wrong for some ports. Created attachment 51265 [details] patch against the QtWebKit week11, requires the NPN_(Get/Set)ValueForURL patch attached to #34539 This patch requires https://bugs.webkit.org/attachment.cgi?id=51264 \-> This patch has the required Gecko NPAPI v1.9 methods in crude working form. i.e. Is there a non-Qt way to figure out proxy settings that WebKit is using to provide that to the plugin? Feel free to improve the patches! Created attachment 51268 [details]
1 line patch that enables Java Applet support in QtLauncher
Turns on Java Applets in QtLauncher
*** Bug 36902 has been marked as a duplicate of this bug. *** I do not know about the Windows related change, but on Linux the one line patch in FrameLoaderClientQt::createJavaAppletWidget is sufficent to activate the Java applet support in QtWebKit granted the patch in bug# 34539 is also applied. (In reply to comment #20) > I do not know about the Windows related change, but on Linux the one line patch in FrameLoaderClientQt::createJavaAppletWidget is sufficent to activate the Java applet support in QtWebKit granted the patch in bug# 34539 is also applied. Hey Dawit, thanks for taking up the banner on this stuff. I'm sure Benjamin will be happy to hear that it runs on Linux. You probably also need the method sig line above it to get the pluginSize args, etc (so 2 lines on Linux). The rest of that patch is debug statements (which aren't really needed now that it works) and locating the Java Plug-in DLL on Windows via the registry (which is needed but might be refactorable). (In reply to comment #21) > (In reply to comment #20) > > I do not know about the Windows related change, but on Linux the one line patch in FrameLoaderClientQt::createJavaAppletWidget is sufficent to activate the Java applet support in QtWebKit granted the patch in bug# 34539 is also applied. > > Hey Dawit, thanks for taking up the banner on this stuff. I'm sure Benjamin will be happy to hear that it runs on Linux. No problem... It is one of the items on my TODO list for the QtWebKit KDE library (kdewebkit). > You probably also need the method sig line above it to get the pluginSize args, etc > (so 2 lines on Linux). Ahh yes indeed... > The rest of that patch is debug statements (which aren't really needed now > that it works) and locating the Java Plug-in DLL on Windows via the registry > (which is needed but might be refactorable). Perhaps breaking the patch for locating the Java Plugin-DLL into a separate fix as suggested in comment #1 by Kenneth would result the whole thing being accepted quickly ? > I'm sure Benjamin will be happy to hear that it runs on Linux. Yep. :) > Perhaps breaking the patch for locating the Java Plugin-DLL into a separate fix as suggested in comment #1 by Kenneth would result the whole thing being accepted quickly ? Sounds good. I can test for Mac if you need. Created attachment 55712 [details]
Activate Java applet support...
Portion of the original patch that activates java support in QtWebKit. This patch splits out the code that is used to locate the Java plugin DLL on Windows into its own separate bug report, #38911.
(In reply to comment #24) > Created an attachment (id=55712) [details] > Activate Java applet support... > > Portion of the original patch that activates java support in QtWebKit. This patch splits out the code that is used to locate the Java plugin DLL on Windows into its own separate bug report, #38911. Garth you might want to change the dependency of this bug report to include bug #38911 as well... Added #38911 as a dependency, though it is only a dependency for Windows. (In reply to comment #23) > > I'm sure Benjamin will be happy to hear that it runs on Linux. > > Yep. :) > > > > Perhaps breaking the patch for locating the Java Plugin-DLL into a separate fix as suggested in comment #1 by Kenneth would result the whole thing being accepted quickly ? > > Sounds good. I can test for Mac if you need. Yes, please do that... Comment on attachment 55712 [details] Activate Java applet support... Clearing flags on attachment: 55712 Committed r59449: <http://trac.webkit.org/changeset/59449> All reviewed patches have been landed. Closing bug. (In reply to comment #29) > All reviewed patches have been landed. Closing bug. Commmiting this patch without dependencies is problematic, specially the one provided in bug# 34539. Without that patch attempting to view pages with embeded Java applet will surely cause a crash in QtWebKit now. Additionally without the fix in bug #38911, this patch along with the one in bug# 34539 will only enable Java applet support on Linux and Unix like OSes. IOW, it will not work on Windows. Is there anyways, those patches can also be reviewed and committed soon ? Thanks. |