Bug 19012 – [Gtk][Qt] Temporary files passed to NPAPI plugins are deleted immediately

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

[marcoil]

Plugins that take streams as NP_ASFILE or NP_ASFILEONLY don't get a chance to read the file because it's deleted in PluginStream::destroyStream.

This can be tested with mozplugger, for example.

[marcoil]

Created attachment 21177 [details]
Fixes the issue on both Gtk and Qt

This patch ensures that temporary files are not deleted until either the plugin asks for it or the PluginView is stopped.

[Simon Hausmann]

Comment on attachment 21177 [details]
Fixes the issue on both Gtk and Qt

In principle this looks good to me, but I'm wondering about the QTempFile change in FileSystemQt. According to the documentation the QTempFile(const QString &templateName) constructor creates the file already in QDir::tempPath(). Do you remember why this change was needed?

[marcoil]

(In reply to comment #2)
> (From update of attachment 21177 [details] [edit])
> In principle this looks good to me, 

Thanks!

> but I'm wondering about the QTempFile
> change in FileSystemQt. According to the documentation the QTempFile(const
> QString &templateName) constructor creates the file already in
> QDir::tempPath(). Do you remember why this change was needed?
> 

According to http://doc.trolltech.com/4.4/qtemporaryfile.html , the constructor that puts the file in QDir::tempPath() is the one without arguments. When using the one that takes a template, it says "You can use QDir::tempPath() to construct templateName if you want use the system's temporary directory."

I think I remember getting files in the current dir until I tried it this way, although that doesn't mean it's the correct way :)

[Simon Hausmann]

Comment on attachment 21177 [details]
Fixes the issue on both Gtk and Qt

You're right about QTemporaryFile, I didn't read that correctly :). The patch looks overall good to me, but I don't know if the additional calls to cancelAndDestroyStream might cause problems on Windows. I'm putting andersca in for review.

[Alp Toker]

This patch fixes Moonlight plugin errors:

PluginXamlLoader::TryLoad: Could not load xaml file: /tmp/WKPD44EJU (error: (null) attr=(null))

Is the bug still assigned/in the correct review queue?

[marcoil]

(In reply to comment #5)
> Is the bug still assigned/in the correct review queue?
> 

Unfortunately, in the time since I made this patch I have changed projects and I'm no longer able to work on WebKit. I suppose the patch will need some adjusting to apply cleanly now.

[arno.]

Patch applys correctly and build at least for gtk backend.
Then, temporary files are no more deleted.

But, when I load a pdf file, and try to reload, webkit segfaults.

#0  0xb728855f in WebCore::ResourceLoader::setShouldBufferData () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#1  0xb72867e2 in WebCore::PluginTokenizer::writeRawData () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#2  0xb726c31d in WebCore::FrameLoader::write () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#3  0xb726c617 in WebCore::FrameLoader::addData () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#4  0xb703b845 in WebKit::FrameLoaderClient::committedLoad () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#5  0xb7268797 in WebCore::FrameLoader::committedLoad () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#6  0xb725a59c in WebCore::DocumentLoader::commitLoad () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#7  0xb7268bd5 in WebCore::FrameLoader::receivedData () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#8  0xb7280d06 in WebCore::MainResourceLoader::addData () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#9  0xb72882d9 in WebCore::ResourceLoader::didReceiveData () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#10 0xb7281061 in WebCore::MainResourceLoader::didReceiveData () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#11 0xb7287d98 in WebCore::ResourceLoader::didReceiveData () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#12 0xb74046d3 in WebCore::writeCallback () from /home/arno/midori/builddir/.libs/libwebkit-1.0.so.1
#13 0xb67fdb69 in ?? () from /usr/lib/libcurl-gnutls.so.4

[Maciej Stachowiak]

Comment on attachment 21177 [details]
Fixes the issue on both Gtk and Qt

r- since according to the tester the patch causes segfaults. So it can't be landed as-is.

Created attachment 43934 [details]
using this patch,  mozplugger works well in QT/Webkit browser, as well as in firefox

[Robert Hogan]

Can you confirm if this was fixed with:

https://bugs.webkit.org/show_bug.cgi?id=29068

and

http://trac.webkit.org/changeset/48204 ?

[arno.]

Created attachment 50655 [details]
patch v1

I don't know about qt, but for gtk, but is still not fixed.

I tried to improve upon marcoil's patch but by calling destroyStream multiple times, I always ended up with segfaults or failing patches. Then, I thought about deleting temporary file in PluginStream destructor. That seems to work.

[arno.]

Created attachment 50656 [details]
tests v1

for tests, I was thinking about testing if file exists in NPP_StreamAsFile.
But as this function returns void, the only way I could imagine was to write something in stderr in case file does not exist.
See this patch (unix only).
What do you think about this approach ?

[Eric Seidel (no email)]

Comment on attachment 50655 [details]
patch v1

+        No new tests. (OOPS!)

Please include tests as part of the diff.

[arno.]

(In reply to comment #13)
> (From update of attachment 50655 [details])
> +        No new tests. (OOPS!)
> 
> Please include tests as part of the diff.

ok, but can you tell if my approach, described in comment 12 is correct. If yes, should I consider, plugin/destroy-stream-twice.html covers the bug, or should I write another one ?

[arno.]

Created attachment 51143 [details]
patch v2

(In reply to comment #12)
> Created an attachment (id=50656) [details]
> tests v1
> 
> for tests, I was thinking about testing if file exists in NPP_StreamAsFile.
> But as this function returns void, the only way I could imagine was to write
> something in stderr in case file does not exist.
> See this patch (unix only).
> What do you think about this approach ?

This was not enough because, non manually-loaded streams were disconnected from view at the end of destroyStream method. In my new patch, I use the m_hasTempFile member from marcoil original patch. I also had to check for stream->ndata in pluginStream::destroyStream because it's set to null in destroyStream, and, if stream is not disconnected, that caused a crash if pluginView::destroyStream was called afterwards (discovered with  plugins/destroy-stream-twice.html)

(In reply to comment #13)
> (From update of attachment 50655 [details])
> +        No new tests. (OOPS!)
> 
> Please include tests as part of the diff.

I'm also including a test. I added a method testStreamFileAvailable to TestNetscapePlugin. In that method, I check if stream file is available. Unfortunately, I don't known how to test for file existence in a cross-platform way. I also don't known how to rearrange code to have a method dispatched in TestNetscapePlugin platform specific files. So, this patch has some PLATFORM defines which is probably not the right thing to do. I need help to fix this.

[Alexey Proskuryakov]

Does this problem only exist on Qt and Gtk? If so, what are other ports doing differently?

[arno.]

Created attachment 51519 [details]
patch v2.2

it seems like stat is cross platform, so no need to #if PLATFORM in PluginObject.cpp

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp:992:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 13 files


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

[arno.]

Created attachment 52407 [details]
patch v2.3

previous did not apply on trunk anymore since http://trac.webkit.org/changeset/56825
updated patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp:993:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 13 files


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

[WebKit Review Bot]

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

[arno.]

(In reply to comment #21)
> Attachment 52407 [details] did not build on win:
> Build output: http://webkit-commit-queue.appspot.com/results/1636207

maybe something related to stat system call.
Unfortunately, I don't known how to fix that.

[Martin Robinson]

Qt is gone and GTK+ no longer uses this plugin implementation, so I'm going to close this issue.