Bug 15766 – [GTK] WebKit sometimes spews binary data as text/plain into iframes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Rodney Dawes]

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.

[Rodney Dawes]

Created attachment 16950 [details]
Patch to validate mime type before dumping in an iframe

[Rodney Dawes]

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.

[Rodney Dawes]

Created attachment 16982 [details]
Updated patch to fix one style issue and include a ChangeLog entry in header

[Mark Rowe (bdash)]

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.

[Rodney Dawes]

Created attachment 16997 [details]
Update patch to match win32/osx and include ChangeLog as patch

[Alp Toker]

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 @@
>[phone]  Rodney Dawes  <[e-mail]>
>+
>+	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  <[e-mail]>
> 
>         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

[Rodney Dawes]

(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.

[Rodney Dawes]

Created attachment 17043 [details]
Updated patch to remove the plugin-specific bit for now

[Alp Toker]

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?

[Rodney Dawes]

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.

[Darin Adler]

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?

[Darin Adler]

(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.

[Rodney Dawes]

(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.

[Rodney Dawes]

(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.

[Mark Rowe (bdash)]

Landed in r27491.