Bug 16814 – Give plugin a chance to handle ActiveX objects

• Component: Plug-ins
• Status: RESOLVED FIXED

[Rui Jiang]

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.

[Rui Jiang]

Created attachment 18374 [details]
Patch for the proposed change

[Anders Carlsson]

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.

[Rui Jiang]

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.

[Eric Seidel (no email)]

Curious what ever happened to this patch?  Looks like Rui might have been waiting for Anders' "ack" of his comment.

[Rui Jiang]

Created attachment 22688 [details]
Patch that allow custom build WebKit to handle ActiveX objects differently

[Rui Jiang]

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.

[Eric Seidel (no email)]

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.

[Glenn Wilson]

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.

[Rui Jiang]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Anders Carlsson]

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.

[Eric Seidel (no email)]

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

[Peter Kasting]

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?

[Peter Kasting]

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?

[Rui Jiang]

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

[Peter Kasting]

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

[Rui Jiang]

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.

[Peter Kasting]

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.

[Peter Kasting]

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.

[Peter Kasting]

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.

[Peter Kasting]

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.

[Anders Carlsson]

Comment on attachment 25522 [details]
updated patch v6

Sorry for not looking at this earlier!

Looks great! r=me.

[Peter Kasting]

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?

[Peter Kasting]

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.

[Peter Kasting]

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?

[Peter Kasting]

Created attachment 25547 [details]
patch v9

This fixes a crash that could happen with the previous patch.

[Eric Seidel (no email)]

Comment on attachment 25522 [details]
updated patch v6

clearing r+ on this patch to remove it from the commit queue.

[Anders Carlsson]

Comment on attachment 25547 [details]
patch v9

r=me

[Peter Kasting]

Committed in r39115.