Bug 15766 - [GTK] WebKit sometimes spews binary data as text/plain into iframes
Summary: [GTK] WebKit sometimes spews binary data as text/plain into iframes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2007-10-30 12:46 PDT by Rodney Dawes
Modified: 2007-11-06 16:05 PST (History)
1 user (show)

See Also:


Attachments
Patch to validate mime type before dumping in an iframe (1.62 KB, patch)
2007-10-30 12:47 PDT, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch to match the win32 port (1.28 KB, patch)
2007-10-31 08:01 PDT, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch to fix one style issue and include a ChangeLog entry in header (1.55 KB, patch)
2007-11-01 08:28 PDT, Rodney Dawes
mrowe: review-
Details | Formatted Diff | Diff
Update patch to match win32/osx and include ChangeLog as patch (1.96 KB, patch)
2007-11-02 14:00 PDT, Rodney Dawes
alp: review-
Details | Formatted Diff | Diff
Updated patch to remove the plugin-specific bit for now (1.84 KB, patch)
2007-11-05 09:13 PST, Rodney Dawes
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rodney Dawes 2007-10-30 12:46:26 PDT
When a plug-in is not available for a type, webkit creates a new frame and appears to dump a textual representation of the file data into it, when plug-ins are enabled. The attached patch resolves this by checking the MIME type to see if we should be creating a frame or defaulting back to ObjectTypeNone.
Comment 1 Rodney Dawes 2007-10-30 12:47:03 PDT
Created attachment 16950 [details]
Patch to validate mime type before dumping in an iframe
Comment 2 Rodney Dawes 2007-10-31 08:01:50 PDT
Created attachment 16966 [details]
Updated patch to match the win32 port

This is an updated version of the patch to match the win32 port, with a small change to return use ObjectContentNone if the type is empty, so that we don't assume we might be able to display the content, as we don't know what it is. It also lacks the check for an available plug-in, which will be added back when plug-ins are available and working.
Comment 3 Rodney Dawes 2007-11-01 08:28:17 PDT
Created attachment 16982 [details]
Updated patch to fix one style issue and include a ChangeLog entry in header
Comment 4 Mark Rowe (bdash) 2007-11-02 10:44:18 PDT
Comment on attachment 16982 [details]
Updated patch to fix one style issue and include a ChangeLog entry in header

For sake of compatibility I think you should match Windows and Mac in the return value when the MIME type is empty.  A minor coding style nit that should be fixed is the lack of whitespace around the + operator.  You should also include your ChangeLog entry, with spaces instead of tabs for indentation, as part of the diff.
Comment 5 Rodney Dawes 2007-11-02 14:00:17 PDT
Created attachment 16997 [details]
Update patch to match win32/osx and include ChangeLog as patch
Comment 6 Alp Toker 2007-11-02 18:07:51 PDT
Comment on attachment 16997 [details]
Update patch to match win32/osx and include ChangeLog as patch

>Index: WebKit/gtk/ChangeLog
>===================================================================
>--- WebKit/gtk/ChangeLog	(revision 27376)
>+++ WebKit/gtk/ChangeLog	(working copy)
>@@ -1,3 +1,11 @@
>+2007-11-02  Rodney Dawes  <dobey@wayofthemonkey.com>
>+
>+	Do some MIME type comparisons to avoid spewing flash/etc... into
>+	an iframe as text/plain because the HTTP connection told us it was
>+
>+	* WebCoreSupport/FrameLoaderClientGtk.cpp:
>+	(FrameLoaderClient::objectContentType):
>+
> 2007-10-29  Alp Toker  <alp@atoker.com>
> 
>         Reviewed by Maciej.
>Index: WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp
>===================================================================
>--- WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	(revision 27376)
>+++ WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	(working copy)
>@@ -226,14 +247,23 @@ Widget* FrameLoaderClient::createJavaApp
> 
> ObjectContentType FrameLoaderClient::objectContentType(const KURL& url, const String& mimeType)
> {
>-    if (url.isEmpty())
>-        return ObjectContentType();
>+    String type = mimeType;
>+    if (type.isEmpty())
>+        type = MIMETypeRegistry::getMIMETypeForExtension(url.path().mid(url.path().findRev('.')+1));
>+
>+    if (type.isEmpty())
>+        return WebCore::ObjectContentFrame; // Match win32/OSX behavior for now

I guess we can't use MIMETypeRegistry::getMIMETypeForPath() because it returns "application/octet-stream" instead of an error status? That would explain why Win did the path parsing themselves.

> 
>-    // TODO: use more than just the extension to determine the content type?
>-    String rtype = MIMETypeRegistry::getMIMETypeForPath(url.path());
>-    if (!rtype.isEmpty())
>-        return ObjectContentFrame;
>-    return ObjectContentType();
>+    if (MIMETypeRegistry::isSupportedImageMIMEType(type))
>+        return WebCore::ObjectContentImage;
>+
>+    if (PluginDatabaseGtk::installedPlugins()->isMIMETypeRegistered(mimeType))
>+        return WebCore::ObjectContentNetscapePlugin;

Careful. There is no PluginDatabaseGtk yet, so this will break the build. I appreciate that you're juggling a lot of changes. Perhaps it'll be OK to comment this out along with a TODO.

>+
>+    if (MIMETypeRegistry::isSupportedNonImageMIMEType(type))
>+        return WebCore::ObjectContentFrame;
>+
>+    return WebCore::ObjectContentNone;
> }
> 
> String FrameLoaderClient::overrideMediaType() const
Comment 7 Rodney Dawes 2007-11-05 09:12:33 PST
(In reply to comment #6)
> Careful. There is no PluginDatabaseGtk yet, so this will break the build. I
> appreciate that you're juggling a lot of changes. Perhaps it'll be OK to
> comment this out along with a TODO.

D'oh. Forgot to remove that bit. This is what happens when you do too many things at once in the same tree. :) Attaching an updated patch now.
Comment 8 Rodney Dawes 2007-11-05 09:13:44 PST
Created attachment 17043 [details]
Updated patch to remove the plugin-specific bit for now
Comment 9 Alp Toker 2007-11-05 16:20:54 PST
My understanding is that under this condition:

  url="http://www.example.com/foo.unknownextension"
  mimeType=""

This code will return ObjectContentFrame.

And under this condition:

  url="http://www.example.com/foo"
  mimeType=""

It'll return ObjectContentNone.

I'm not sure if this is good behaviour. Problem also seems to exist in the Win port. What do you think?
Comment 10 Rodney Dawes 2007-11-05 17:15:22 PST
Yes. I think the behavior in this patch is the same for win32 and osx. It was suggested by bdash that we do the same as those for now, and then make a separate change proposal to have them all return ObjectContentNone, when we don't know what the type is. This seems to be how gecko behaves, by just ignoring the content, rather than dumping it in a frame.
Comment 11 Darin Adler 2007-11-06 07:51:50 PST
Comment on attachment 17043 [details]
Updated patch to remove the plugin-specific bit for now

+        return WebCore::ObjectContentFrame; // Match win32/OSX behavior for now

I think that's a confusing comment. What's "now"? Will this need to change in the future? If so, why?
Comment 12 Darin Adler 2007-11-06 07:53:31 PST
(In reply to comment #9)
> My understanding is that under this condition:
> 
>   url="http://www.example.com/foo.unknownextension"
>   mimeType=""
> 
> This code will return ObjectContentFrame.
> 
> And under this condition:
> 
>   url="http://www.example.com/foo"
>   mimeType=""
> 
> It'll return ObjectContentNone.
> 
> I'm not sure if this is good behaviour. Problem also seems to exist in the Win
> port. What do you think?

I'd like to see more of the high level algorithm here in platform-independent code; that way we could correct this mistake (assuming it is one) once, rather than having multiple copies of it. We could consider changing the client function to do a smaller piece of the work.
Comment 13 Rodney Dawes 2007-11-06 08:37:05 PST
(In reply to comment #11)
> (From update of attachment 17043 [details] [edit])
> +        return WebCore::ObjectContentFrame; // Match win32/OSX behavior for
> now
> 
> I think that's a confusing comment. What's "now"? Will this need to change in
> the future? If so, why?

"Now" would be "the immediate future." As soon as this patch lands, the plan was to patch all the platforms to return ObjectContentNone in this case, to avoid dumping things into frames when we have no idea what they are, matching the behavior in Gecko.
Comment 14 Rodney Dawes 2007-11-06 08:41:16 PST
(In reply to comment #12)
> I'd like to see more of the high level algorithm here in platform-independent
> code; that way we could correct this mistake (assuming it is one) once, rather
> than having multiple copies of it. We could consider changing the client
> function to do a smaller piece of the work.

Does it make sense to do this? The portion to check if a plug-in is available is platform-dependent. Would the method then have to check the parent method's return value, and then see if a plug-in is available, and return the parent method's return if not? We'd have to duplicate the bits to get the MIME type String still though, in the child function, I think.

Comment 15 Mark Rowe (bdash) 2007-11-06 16:05:23 PST
Landed in r27491.