Bug 45679 – r66156 broke AtlasCT library, formerly affected http://map.d.co.il/

• Component: Plug-ins
• Status: RESOLVED FIXED

[anton muhin]

http://trac.webkit.org/changeset/66156/ broke http://map.d.co.il/

I don't know if HTML of the site is valid or not---I hope you're in a better position to judge.

The repro: build at revision 66155 and open the site: a map should appear.  Build the next revision, and map doesn't show up.

That affects both Safari and Chromium.

[Jeremy Moskovich]

map.d.co.il is one of the most popular maps sites in Israel so this is a pretty visible regression.

[Maciej Stachowiak]

This site has a dynamically generated nested object/embed which looks like this (copied from Web Inspector):

<object classid="clsid:D27CDB6E-AE6D-11cf-96B8-444553540000" codebase="http://download.macromedia.com/pub/shockwave/cabs/flash/swflash.cab#version=6,0,0,0" width="100%25" height="100%25" id="AtlasMap1" align=""> <param name="movie" value="http://maps.d.co.il/sdk_v3_2_1/Kernel/RemoteFlash8New.swf?3_2_0"> <param name="zmenu" value="false"> <param name="FlashVars" value="Server=&amp;Dll=&amp;customCtrl=&amp;MapWidth=100%25&amp;MapHeight=100%25&amp;FlashId=11&amp;UD=c11a6c8821cdb246&amp;SessionId=AN-ATLF76112002&amp;MapVersion=3_2_0&amp;GuiLang=Heb&amp;MapUnits=metric&amp;"> <param name="bgcolor" value="#EAEADB"> <param name="quality" value="high"> <param name="allowScriptAccess" value="always"> <param name="wmode" value="transparent"> <embed src="http://maps.d.co.il/sdk_v3_2_1/Kernel/RemoteFlash8New.swf?3_2_0" quality="high" wmode="transparent" allowscriptaccess="always" flashvars="Server=&amp;Dll=&amp;customCtrl=&amp;MapWidth=100%25&amp;MapHeight=100%25&amp;FlashId=11&amp;UD=c11a6c8821cdb246&amp;SessionId=AN-ATLF76112002&amp;MapVersion=3_2_0&amp;GuiLang=Heb&amp;MapUnits=metric&amp;" bgcolor="#EAEADB" menu="true" width="100%25" height="100%25" name="AtlasMap1" align="" type="application/x-shockwave-flash" pluginspage="http://www.macromedia.com/go/getflashplayer"></object>

[Andy Estes]

Here is what is going on:

- r66156 causes WebKit to load this site's plug-in via the object element rather than the nested embed element (see the snippet posted by Maciej above).
- The plug-in loads successfully, but then tries to call out to JavaScript to do some extra initialization work. The function it calls is <name>_DoFSCommand(), where <name> is the name attribute of the element that loaded the plug-in.
- The object element has no name attribute (name only exists on the embed element), so the plug-in tries to call a non-existant function. This leaves the plug-in in a partially initialized state. For instance, the map works (you can right click to zoom in, zoom out and re-center), but you can't drag the map or use the scroll wheel to zoom.
- Adding a name attribute to the object or removing the object all together fixes this issue.
- The site assumes the only browser that will use the object element is IE, and follows a special initialization path in that case. It uses <script event='...' for='...'> for initialization, which only works in IE.

I think what we're doing is correct according to HTML5, but we should consider implementing a workaround given how popular this site is in Israel (Alexa ranks it #53). There are several things we could do to fix this:

1) Add a site-specific hack that forces fallback to the embed element and perhaps reach out to the website to notify them of this issue.
2) Remove the mapping from "clsid:D27CDB6E-AE6D-11cf-96B8-444553540000" to "application/x-shockwave-flash". I believe the reason this site works in Firefox is that it doesn't understand the classid attribute (either in general or just for flash in particular) on the object so it falls back to the embed (removing the classid attribute and adding a type attribute causes the same issue to reproduce in Firefox). Doing this would probably result in new compatibility regressions, so it's probably a bad idea.

[Adam Barth]

I don't understand the motivation for http://trac.webkit.org/changeset/66156.  It seems like it has caused a bunch of regressions.  (I might be misunderstanding since I'm not following these issues that closely.)  Maybe there's more of an explanation in rdar?

[Andy Estes]

(In reply to comment #4)
> I don't understand the motivation for http://trac.webkit.org/changeset/66156.  It seems like it has caused a bunch of regressions.  (I might be misunderstanding since I'm not following these issues that closely.)  Maybe there's more of an explanation in rdar?

It's true that this has caused regressions, although I don't think it has caused "a bunch". The only two issues I'm aware of right now is this one and a PLT regression (https://bugs.webkit.org/show_bug.cgi?id=45524). I'm aware of https://bugs.webkit.org/show_bug.cgi?id=44933, but this issue will be fixed when chromium merges r66992. Are there other regressions you're aware of?

The intent of this change was to bring WebKit in line with how HTML5 specifies handling the object element. There really isn't any information in Radar that isn't also in the ChangeLog entries for r66156 and r66992, as well as in the bugzilla.

[Adam Barth]

Oh, I don't mean to be negative.  Just seeking to understand.  When making changes to comply with HTML5, we should also keep in mind that another option is to change the spec.

[Andy Estes]

(In reply to comment #6)
> Oh, I don't mean to be negative.  Just seeking to understand.  When making changes to comply with HTML5, we should also keep in mind that another option is to change the spec.

I didn't think you were being negative :) My quotations were not meant to be hostile.

I agree that changing the spec is also an option, although the changes I've made so far seem to make sense and also make us more closely match the behavior of other browsers, at least by my testing. I've been meaning to post something to webkit-dev about this change since it is probably significant enough to warrant some discussion. This might also shake out any additional regressions.

[Ian 'Hixie' Hickson]

Please let me know how you fix this so I can be sure to update the spec accordingly.

Do we have data on whether supporting classid="" is actually more helpful than harmful? Do any other non-IE browsers support it like Webkit?

[Yair Yogev]

i will gladly point d.co.il site admin here once it's decided as the best plan of action.

[Jeremy Moskovich]

Thanks Yair,

FYI: I've already contacted the site admins, pointed them at this bug, and asked them to add a name attribute to the object tag.

[Yair Yogev]

I discovered this issue doesn't affect d.co.il only,
it affects all of AtlasCT customers
http://www.atlasct.co.il/Our_Customers.html
which includes maps.walla.co.il (#6 in Israel according to Alexa), ynet (#5 in Israel) and more...

[Andy Estes]

(In reply to comment #8)
> Please let me know how you fix this so I can be sure to update the spec accordingly.

Will do.

> 
> Do we have data on whether supporting classid="" is actually more helpful than harmful? Do any other non-IE browsers support it like Webkit?

It looks like Gecko handles the classid attribute only if there is an ActiveX plug-in installed. WebKit understands a small set of classids that it maps to MIME types (http://trac.webkit.org/browser/trunk/WebCore/html/HTMLObjectElement.cpp?rev=67179#L121).

I don't have good data on whether this helps or hurts. Arguably it hurts in this case, but I presume these mappings were added to solve previous compatibility problems.

[Darin Adler]

(In reply to comment #12)
> (In reply to comment #8)
> > Please let me know how you fix this so I can be sure to update the spec accordingly.
> 
> Will do.
> 
> > Do we have data on whether supporting classid="" is actually more helpful than harmful? Do any other non-IE browsers support it like Webkit?
> 
> It looks like Gecko handles the classid attribute only if there is an ActiveX plug-in installed. WebKit understands a small set of classids that it maps to MIME types (http://trac.webkit.org/browser/trunk/WebCore/html/HTMLObjectElement.cpp?rev=67179#L121).
> 
> I don't have good data on whether this helps or hurts. Arguably it hurts in this case, but I presume these mappings were added to solve previous compatibility problems.

At least one of these mappings was already in KHTML, not added in the WebKit era. There’s a good chance these mappings don’t help much any more.

It’s not going to be easy to get data on how changing these will affect things, but perhaps we can just remove these entirely.

[Andy Estes]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #8)
> > > Please let me know how you fix this so I can be sure to update the spec accordingly.
> > 
> > Will do.
> > 
> > > Do we have data on whether supporting classid="" is actually more helpful than harmful? Do any other non-IE browsers support it like Webkit?
> > 
> > It looks like Gecko handles the classid attribute only if there is an ActiveX plug-in installed. WebKit understands a small set of classids that it maps to MIME types (http://trac.webkit.org/browser/trunk/WebCore/html/HTMLObjectElement.cpp?rev=67179#L121).
> > 
> > I don't have good data on whether this helps or hurts. Arguably it hurts in this case, but I presume these mappings were added to solve previous compatibility problems.
> 
> At least one of these mappings was already in KHTML, not added in the WebKit era. There’s a good chance these mappings don’t help much any more.

Yea, it looks like mappings for 'application/x-shockwave-flash' and 'audio/x-pn-realaudio-plugin' came from KHTML. We added mappings for 'video/quicktime' and 'application/x-director' in http://trac.webkit.org/changeset/3123 and a mapping for 'application/x-mplayer2' in http://trac.webkit.org/changeset/7616. The bugs for these changesets mention several sites that either no longer exist or would now work even without a classid->mime mapping.

An argument for removing these mappings might be that an author who writes an object element with a classid is intending the browser to load an ActiveX component since classid is a COM/OLE concept, and so the expectation is that browsers that don't support ActiveX will render the object's fallback content. This seems to be the assumption made by AtlasCT. Also, the lack of these mappings in Gecko might mean that the only regressions would be in content that was specifically targeting WebKit in this manner. That's just a guess.

[Andy Estes]

Opera changed their object handling to be in line with HTML5 in 10.50. Like Gecko, they also render fallback content when an object has a non-empty classid attribute. They blogged about this at http://my.opera.com/sitepatching/blog/2010/02/25/new-java-support-and-object-parsing.

The site mentions that this change caused a regression on some Norwegian banking sites and they were working with the banks to get it resolved.

[Yair Yogev]

Looks like map.d.co.il updated their code and it is now working fine in Chrome.
Things are still broken in other AtlasCT customer such as
http://maps.walla.co.il/
http://maps.yad2.co.il/
http://www.atlasct.com/israel/citymouse/

http://ymap.winwin.co.il/Navigate.aspx
http://www.atlasct.com/israel/calcalist/

http://www.homeless.co.il/map/
and probably more

[Nir Zoref]

Hi All,
We've updated our code for all websites and it is now working fine in Chrome.

BR,
Nir, AtlasCT.

[Darin Adler]

So maybe we don’t have fix anything.

[Jeremy Moskovich]

So since the various sites are fixed it seems that the only remaining open question is whether we want to match other browsers more closely by doing away with/modifying classid mappings.

[Carlos Pizano-Uribe]

Actually it seems a lot of stuff still broken. See the matching bug in chrome 

It seems many sites use the flash fscommand technique to communicate with the page.Let me copy here one of the last comments in the chrome bug:

"The reason is because content producers have simply done as they were told when writing their code. Adobe has documented that in order for fscommand to work properly, you need only to set the value of the NAME attribute of the EMBED tag or the ID property of the OBJECT tag.  Setting the value of the name property has never been a requirement of fscommand. The following documentation dates back to 2000:

http://www.adobe.com/support/flash/action_scripts/actionscript_dictionary/actionscript_dictionary372.html  

Even today, ten years later, Adobe ships their fscommand template in the latest version of Flash CS5.  Attached is the output that Flash CS5 produces and the screenshot in the tool for how it’s selected. It conforms to their published doc. As expected, this template works in other browsers.."

http://code.google.com/p/chromium/issues/detail?id=57986

I advice we remove the hardcoded mappings as Andy Estes proposes.

[adamarticulate]

(In reply to comment #20)

> I advice we remove the hardcoded mappings as Andy Estes proposes.

Yes, I'm in agreement. Otherwise, I can personally attest to millions of sites (online courses and quizzes) that break.  (Not to mention any other implementation of FSCommand per Adobe's spec.)

For the full background on just the online training sites that break, read my full comment here:

http://code.google.com/p/chromium/issues/detail?id=57986#c15

Pissed off students who don't get their scores recorded wouldn't bode so well. :)

[Andy Estes]

(In reply to comment #20)
> Actually it seems a lot of stuff still broken. See the matching bug in chrome 
> 
> It seems many sites use the flash fscommand technique to communicate with the page.Let me copy here one of the last comments in the chrome bug:
> 
> "The reason is because content producers have simply done as they were told when writing their code. Adobe has documented that in order for fscommand to work properly, you need only to set the value of the NAME attribute of the EMBED tag or the ID property of the OBJECT tag.  Setting the value of the name property has never been a requirement of fscommand. The following documentation dates back to 2000:
> 
> http://www.adobe.com/support/flash/action_scripts/actionscript_dictionary/actionscript_dictionary372.html  
> 
> Even today, ten years later, Adobe ships their fscommand template in the latest version of Flash CS5.  Attached is the output that Flash CS5 produces and the screenshot in the tool for how it’s selected. It conforms to their published doc. As expected, this template works in other browsers.."
> 
> http://code.google.com/p/chromium/issues/detail?id=57986
> 
> I advice we remove the hardcoded mappings as Andy Estes proposes.

Thanks for this feedback, Carlos. This provides good justification for changing our behavior.

[Carlos Pizano-Uribe]

Andy, Awesome. Could you fix it soon? :) I got a lot of flash devs breathing down my neck, it must be that their conference (MAX) is going on now...

I can write the patch, but is it seems to me trivial and I am not a webkit contributor so it would take me a while to get up to speed.

[Carlos Pizano-Uribe]

actually, I take that back, just removing flash clsid from createClassIdToTypeMap() does not solve the problem as webkit is clever and has other means to retrieve the mime type.

[Andy Estes]

(In reply to comment #24)
> actually, I take that back, just removing flash clsid from createClassIdToTypeMap() does not solve the problem as webkit is clever and has other means to retrieve the mime type.

Yes, even though you removed the classid->MIME mapping, WebKit asked its plug-in database for a plug-in that can handle files ending in '.swf' (taken from the movie param), and flash said that it could. The HTML5 spec says that if a non-empty classid attribute is specified that can't be handled by the UA, fallback content should be rendered (which is the embed element in these cases). So, the correct change is a little more complicated than just removing the classid mapping for flash.

[Andy Estes]

Created attachment 72147 [details]
Patch

[Andy Estes]

<rdar://problem/8603844>

[Adam Barth]

Comment on attachment 72147 [details]
Patch

Thanks Andy.

[Andy Estes]

Committed r70748: <http://trac.webkit.org/changeset/70748>

[WebKit Review Bot]

http://trac.webkit.org/changeset/70748 might have broken Qt Linux Release
The following tests are not passing:
platform/qt/plugins/qt-qwidget-plugin.html

[Andy Estes]

Created attachment 72152 [details]
Patch

[Adam Barth]

Comment on attachment 72152 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72152&action=review

> WebCore/html/HTMLObjectElement.cpp:223
> +    ASSERT(object);

ASSERT_UNUSED

[Andy Estes]

Committed r70754: <http://trac.webkit.org/changeset/70754>

[Yair Yogev]

this change probably broke videos in nana10.co.il
Steps to repro:
1. Visit
http://10tv.nana10.co.il/
(for example).
2. If you see a Hebrew message over the black video box, answer it by clicking on בטל (this button http://f.nanafiles.co.il/partner48/Common/Images//VideoPlayer/UI/CancelSaveProfile.gif )
3. wait for the video to load... it won't

[Yair Yogev]

inChromium bug related to my last reply:
http://code.google.com/p/chromium/issues/detail?id=61418

[Jeremy Moskovich]

Yair: thanks! Could you please file a new bug. Information on whether this works in Firefox and a Safari nightly would also be really helpful.

[Yair Yogev]

Bug 48757 to figure out how come that change possibly broke (still just a guess) that site for Chrome only (works fine using Safari)