Bug 16814 - Give plugin a chance to handle ActiveX objects
Summary: Give plugin a chance to handle ActiveX objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Peter Kasting
URL: http://adv.jxnews.com.cn/2007/wqlyj/v...
Keywords:
Depends on:
Blocks:
 
Reported: 2008-01-10 00:51 PST by Rui Jiang
Modified: 2008-12-08 16:36 PST (History)
4 users (show)

See Also:


Attachments
Patch for the proposed change (3.79 KB, patch)
2008-01-10 13:46 PST, Rui Jiang
andersca: review-
Details | Formatted Diff | Diff
Patch that allow custom build WebKit to handle ActiveX objects differently (5.97 KB, patch)
2008-08-06 16:48 PDT, Rui Jiang
eric: review-
Details | Formatted Diff | Diff
patch v3 (8.13 KB, patch)
2008-11-04 12:34 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v4 (7.17 KB, patch)
2008-11-12 18:02 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v5 (7.16 KB, patch)
2008-11-12 18:05 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v6 (7.19 KB, patch)
2008-11-19 17:14 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
updated patch v6 (7.19 KB, patch)
2008-11-26 11:04 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v7 (19.21 KB, patch)
2008-11-26 14:27 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v8 (19.22 KB, patch)
2008-11-26 14:36 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v9 (19.39 KB, patch)
2008-11-26 18:45 PST, Peter Kasting
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rui Jiang 2008-01-10 00:51:19 PST
The current WebKit converts OBJECT tag with well-known classids (e.g., Windows Media Player) to corresponding plugin-ins. e.g.: 

<object classid="clsid:6BF52A52-394A-11D3-B153-00C04F79FAA6" width="280"
height="280" id="WMP">
      <param name="Name" value="WMP1" />
      <param name="autoStart" value="1" />
      <param name="defaultFrame" value="false" />
      <param name="URL"
value="http://adv.jxnews.com.cn/2007/wqlyj/videos/video.wmv" />
      <param name="stretchToFit" value="1" />
    </object>

in RenderPartObject.cpp, it will map the class id to service type: application/x-mplayer2 then try to load the media player plugin to handle it.

However, this may not be always the way users wanted. There has been some ActiveX plugin which could handle the ActiveX object better. If we just map the type to application/x-oleobject and let the Activex plugin handle it, we may have better rendering. e.g., the URL given works well in FireFox with the npActiveX.dll plugin downloaded from http://forums.mozine.cn/index.php?showtopic=2350. While it doesn't work in Safari.

If we could add some #ifdef to turn on/off whether we want to do th mapping, that would be great.

Another smaller problem I see is, it maps unknown ActiveX class id to type : application/x-activex-handler. While usually it's application/x-oleobject in most other places I see. Shall we make it more like others?

I have made some tentative changes and will upload the patch later.
Comment 1 Rui Jiang 2008-01-10 13:46:20 PST
Created attachment 18374 [details]
Patch for the proposed change
Comment 2 Anders Carlsson 2008-01-11 13:04:39 PST
Comment on attachment 18374 [details]
Patch for the proposed change

I like the general idea of this patch. However, it would be better if you could check that a plug-in exists that can handle "application/x-oleobject" before setting the service type. 

There is a PlugInInfoStore method called supportsMIMEType that can be used for this. I don't think it has been implemented on Windows though, so you would have to do that (just have it call PluginDatabaseWin::isMIMETypeRegistered).

Come to think of it, we might want to always check that there is a plug-in available that supports a MIME type before mapping a classid to that MIME type.
Comment 3 Rui Jiang 2008-01-14 12:00:25 PST
Thanks for the comment. That makes very good sense. How about change the logic to:

If application/x-oleobject handler is not available:
  Try to convert to other types as possible.
Else
  Depends on ActiveX type, have the option to do conversion or not.

This will give maximum freedom as to control which ActiveX type shall we convert. While based on available plugins, we will try to render the content as much as possible.
Comment 4 Eric Seidel (no email) 2008-08-01 01:10:29 PDT
Curious what ever happened to this patch?  Looks like Rui might have been waiting for Anders' "ack" of his comment.
Comment 5 Rui Jiang 2008-08-06 16:48:05 PDT
Created attachment 22688 [details]
Patch that allow custom build WebKit to handle ActiveX objects differently
Comment 6 Rui Jiang 2008-08-06 16:57:51 PDT
I submitted a new patch, which also fixes:

--The original mapClassIdToServiceType function is case sensitive. However CLSID s are supposed to be case-insensitive. Fixed.
--If there is an embed tag inside an object, it will always use properties of the embed tag. This is not desired when we have an ActiveX handler, that it should only use the properties of object tag and param tags.
--Another fix is to change the service type from application/x-activex-handler to application/x-oleobject because the latter is used much more often in the world, while the former is never seen (by me).

Could any review the new patch and comment? Thanks.

Comment 7 Eric Seidel (no email) 2008-08-06 22:40:48 PDT
Comment on attachment 22688 [details]
Patch that allow custom build WebKit to handle ActiveX objects differently

I assume Rui meant to mark this for review.
Comment 8 Glenn Wilson 2008-08-26 11:44:02 PDT
Is this patch ready to go?  I was going to add a test reduction/layout test for this, but I wasn't sure how to do that with the #ifndefs.
Comment 9 Rui Jiang 2008-08-27 17:24:29 PDT
Thanks for looking at this issue. The patch ready to go. If you don't make any #defs, it should behave pretty much the same as before, except:

--CLSIDs are now case-insensitive
--Unmapped ActiveX Object will have service type "application/x-oleobject".

If you define DISABLE_ACTIVEX_TYPE_CONVERSION_MPLAYER2, for Windows Media player object it will not try to map it to "application/x-mplayer2" plugin. Thus if you have a plugin that can handle ActiveX Object, that plugin will be created and initialize wmp control instead.

e.g.: 

<object classid="clsid:22d6f312-b0f6-11d0-94ab-0080c74c7e95" id="mmsplayer" width="310" height="65"> 
  <PARAM NAME="FileName" VALUE="abc.mp3"> 
  <PARAM NAME="autoStart" VALUE="-1">
</object>

With the definition of DISABLE_ACTIVEX_TYPE_CONVERSION_MPLAYER2, this object won't be rendered by plugin type "application/x-mplayer2". Instead, it will be rendered by plugin type "application/x-oleobject". 

(In reply to comment #8)
> Is this patch ready to go?  I was going to add a test reduction/layout test for
> this, but I wasn't sure how to do that with the #ifndefs.
> 

Comment 10 Eric Seidel (no email) 2008-08-27 18:07:47 PDT
Comment on attachment 22688 [details]
Patch that allow custom build WebKit to handle ActiveX objects differently

Another way to do this would be to  have this code ask the platform via PluginInfoStore.h or a similar abstraction.  That would allow platforms which do or don't have these plugins to override these mappings at runtime, which is probably more useful (As then you can test for the presence of the plugin) instead of using a compile-time check.

Also, "ShouldUseEmbedForObject" is not following WebKit-style naming conventions.  WebKit style asks that we use camelCase such as: "shouldUseEmbedForObject()"

Also, we generally try to use early-returns instead of long ifs, which would mean this function would become:

if (!attributes)
    return true;
for (... ) {
   ...

Again, I think that this is probably better suited as a platform-specific question, which means we should add a function to PluginInfoStore.h which answers this question on a per-platform basis.

Looking forward to Anders' thoughts.
Comment 11 Eric Seidel (no email) 2008-08-27 18:15:21 PDT
I think this would really be as simple as just moving mapClassIdToServiceType from being a static-inline in that file to being in PluginInfoStore.h (Again Anders can provide greater clarity as to where this function should actually end up).

The name could be something like:

bool findFallbackServiceTypeForClassId(classId, serviceType);

returning true would indicate that a service type was found for that class ID on that platform.
Comment 12 Anders Carlsson 2008-08-27 21:33:04 PDT
Comment on attachment 22688 [details]
Patch that allow custom build WebKit to handle ActiveX objects differently

>-    if (classId.contains("D27CDB6E-AE6D-11cf-96B8-444553540000"))
>+#ifndef DISABLE_ACTIVEX_TYPE_CONVERSION_FLASH

In WebCore we tend to not use #ifndef DISABLE_... Instead, we use the 

feature macros from 

http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/Platform.h#L29

I also think that this chain of ifs could perhaps be refactored into a static HashMap?  

>+// To decide whether we should use the embed tag inside the object tag or not.
>+static bool ShouldUseEmbedForObject(HTMLObjectElement* o)
>+{

Like Eric said, this is not how we name methods. How about something like

shouldUseNestedEmbedIfExists (not sure exactly)


>       // If we still don't have a type, try to map from a specific CLASSID to a type.
>-      if (serviceType.isEmpty() && !o->classId().isEmpty())
>+      // However, if we have the embed tag we shouldn't do so because the outer object
>+      // may be converted to application/x-oleobject, while we are not getting the paramaters

Typo here, should be "parameters".

Overall, I think the patch is definitely a step in the right direction, and I don't think we need to move it to another file just yet.
Comment 13 Eric Seidel (no email) 2008-08-28 14:07:17 PDT
(In reply to comment #12)
> (From update of attachment 22688 [details] [edit])
So I agree that this should move to a HashMap.  Your suggestion was that each of these additions to the hashmap could be controlled by these ifdefs.

So it would look something like this: (pseudocode)

static inline void mapClassIdToServiceType(const String& classId, String& serviceType)
{

   static HashMap<String, String, CaseFoldingHash>* serviceTypeFallbackForClassId = 0;
   if (! serviceTypeFallbackForClassId) {
       serviceTypeFallbackForClassId = new HashMap<String, String, CaseFoldingHash>;
#if ENABLE(PLATFORM_HANDLES_FLASH_CLASS_ID)
       serviceTypeFallbackForClassId->add("D27CDB6E-AE6D-11cf-96B8-444553540000", "application/x-shockwave-flash");
#endif
...
    }
    serviceType = serviceTypeFallbackForClassId->get(classId);
}

One problem with that approach is that the current version uses "contains" and this would use "equals" for comparing the class-ids.  I'm not sure if we care?

I'm also not sure why we'd want to add 4 #ifdefs instead of just moving this out into a separate file where platforms could decide these mappings at runtime.  But I'm comfortable w/ either solution, really, just looking for something long-term.
Comment 14 Peter Kasting 2008-11-04 12:34:58 PST
Created attachment 24892 [details]
patch v3

Attempting to resurrect this patch since Rui is busy.

Preliminary warning: I don't know anything about OBJECT or EMBED tags, or this code, so I'm liable to do stupid things.

This makes the following changes over the previous patch:
* Uses a HashMap instead of a series of conditionals.
* Checks isMIMETypeSupported() before adding a mapping from ActiveX->NPAPI.
* Simplifies and expands logic on when not to proceed into nested EMBEDs.  I think the previous logic was too specific.  I explained my rationale in a comment.
* Removes the addition of the "!embed" check to the call to mapClassIdToServiceType().  I am not sure this is correct.  My logic is, the only cases where this makes a difference are OBJECT tags with classIds that don't map to a well-known plugin, that have nested EMBEDs.  This seems rare and when it does happen I feel like we ought to be able to get the service type from the object.  However, Rui told me privately that he's seen cases in the wild of code that has an outer OBJECT with a RealAudio classId, containing a nested EMBED with a .wma.  If the RealAudio plugin doesn't handle .wma, then Rui's patch would probably work better than mine.  But I wonder if there's anything consistent that makes sense when authors do this kind of thing.

This patch has not been tested (at all).  I'm not precisely sure how to write a layout test for it, but I haven't even smoketested things.  Sample URLs of pages in the wild affected by this code would probably be useful?
Comment 15 Peter Kasting 2008-11-04 12:37:10 PST
Forgot one change.  I switched the raw #ifdefs to using ENABLE macros.

Like Eric I'm rather uncomfortable with adding four more ENABLE_xxx macros for this stuff.  Is there some other way this could be done?  Perhaps add another param to isMIMETypeSupported, or a parallel function call, that lets the port specify whether it wants specific MIME types converted?
Comment 16 Rui Jiang 2008-11-04 17:38:35 PST
The only concern with the hashmap in mapClassIdToServiceType is, when user dynamically refreshes the plugin store, we won't be able to refresh the hashmap here. Plain comparison seems OK to me since we have only a small set of ids to worry about.

shouldUseChildEmbedOfObject seems to be a better approach than my previous patch.

I agree create a platform specific function shouldUseNestedEmbedIfExists would be better than ifdefs...
Comment 17 Peter Kasting 2008-11-04 21:22:00 PST
We don't need a platform-specific function for deciding if we should use a nested embed so much as for deciding if we should do the classId type conversion (the ENABLE_xxx macros).

I'm not sure I understand the issue with "dynamically refreshing the plugin store".  You're concerned that this code will be called before all the plugins are loaded and then again after?  (I didn't think Chromium or Safari supported on-the-fly plugin additions.)

One way to address this without losing the hashmap, if it's a concern, would be to pull the isSupportedMIMEType() calls out from inside the hash population, and instead place them after the call to retrieve things from the map -- so we first get the MIME type regardless, but then check whether we actually support it.  This would probably be better anyway in the sense of making the code shorter and having fewer copies of hardcoded MIME type strings.  I'll try and do that in the next version of the patch (not happening this week, I'm on vacation).
Comment 18 Rui Jiang 2008-11-04 21:36:39 PST
In Chrome we do refresh Plugin Store. The default plugin (which is responsible for plugin installation) will do this after a successful installation of a new plugin. The change you proposed seems good for this case.
Comment 19 Peter Kasting 2008-11-12 18:02:34 PST
Created attachment 25111 [details]
patch v4

Check supported MIME types after pulling things out of the map instead of before.  This is shorter and supports the plugin-DB-refreshed-on-the-fly case.

Remove the ENABLEs for everything but wmplayer.  Chromium only needs that one and no one else needs any of these so why add more than we need to?  It just clutters the code.
Comment 20 Peter Kasting 2008-11-12 18:05:40 PST
Created attachment 25112 [details]
patch v5

I am an idiot and managed to leave in the "!embed" check in both previous patches despite claiming to have removed it.
Comment 21 Peter Kasting 2008-11-19 17:14:46 PST
Created attachment 25292 [details]
patch v6

Finally had a chance to do some local testing.  Need to add "clsid:" on all the classids now that we're using string comparison instead of substring search.  Verified in MSDN and by testing IE that this will work (we won't miss cases that IE would accept).

Local testing of a variant of this patch in Chromium seems to do what we want there.
Comment 22 Peter Kasting 2008-11-26 11:04:14 PST
Created attachment 25522 [details]
updated patch v6

Updated patch to latest trunk (no changes, just trying to get it out of my local tree to work on something else).

Opening review to all because andersca seems to have died.  Or something.
Comment 23 Anders Carlsson 2008-11-26 11:21:18 PST
Comment on attachment 25522 [details]
updated patch v6

Sorry for not looking at this earlier!

Looks great! r=me.
Comment 24 Peter Kasting 2008-11-26 14:27:45 PST
Created attachment 25537 [details]
patch v7

Gah.  The plugin interface has changed, which I found out when this got test-compiled before checkin.  Now, at least on Mac, we need to go through PluginData (which is scoped to a page) instead of the global PluginInfoStore.

The key thing I want to know here is whether, by the time we reach this code, the Frame's Page or the Page's PluginData can be NULL.  If so, I need to check these instead of blindly dereffing them like this patch does.  However I kinda think they can't be NULL here?
Comment 25 Peter Kasting 2008-11-26 14:36:12 PST
Created attachment 25538 [details]
patch v8

I'm stupid, fix a trivial compile error.

I forgot to mention that in the last patch I made whitespace-only changes to fix one of the functions being two-space indented, and remove a bunch of whitespace from blank lines, hence the larger diff -- there weren't functional changes beyond the PluginData change.
Comment 26 Peter Kasting 2008-11-26 14:51:07 PST
I have just noticed there's a PluginDatabase class too, which, if it does what I want, would avoid the hassle of creating a PluginData* object on the page everywhere, and be more like the original code I wrote (which I think was better).  I can't wade through the mess of different plugin interfaces here.  Could someone chime in and tell me what the One True Way is?
Comment 27 Peter Kasting 2008-11-26 18:45:29 PST
Created attachment 25547 [details]
patch v9

This fixes a crash that could happen with the previous patch.
Comment 28 Eric Seidel (no email) 2008-12-02 12:26:05 PST
Comment on attachment 25522 [details]
updated patch v6

clearing r+ on this patch to remove it from the commit queue.
Comment 29 Anders Carlsson 2008-12-05 14:20:47 PST
Comment on attachment 25547 [details]
patch v9

r=me
Comment 30 Peter Kasting 2008-12-08 16:36:25 PST
Committed in r39115.