Bug 49016 - REGRESSION (r70748): latest nightly builds kills AC_QuickTime.js
: REGRESSION (r70748): latest nightly builds kills AC_QuickTime.js
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Plug-ins
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P1 Major
Assigned To: Andy Estes
https://tera.usg.utk.edu/PAMS20101104...
: InRadar, Regression
Depends on: 57071
Blocks: 57132
  Show dependency treegraph
 
Reported: 2010-11-04 12:21 PDT by Michael McGuire
Modified: 2011-03-25 17:14 PDT (History)
8 users (show)

See Also:


Attachments
Webkit errror (474.76 KB, image/png)
2010-11-05 10:13 PDT, Michael McGuire
no flags Details
Safari working (464.31 KB, image/png)
2010-11-05 10:13 PDT, Michael McGuire
no flags Details
Webkit Version (32.74 KB, image/png)
2010-12-01 13:35 PST, Michael McGuire
no flags Details
Patch (88.67 KB, patch)
2011-03-23 19:14 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (62.23 KB, patch)
2011-03-25 15:45 PDT, Andy Estes
adele: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael McGuire 2010-11-04 12:21:47 PDT
The latest nightly build kills AC_QuickTime.js and will not allow it to run and play the movie in the QuickTime player. This works in IE on Windows & and in 5.0.2 Safari on OS X and in FireFox on OS X but not in webkit. It did until today.

http://www.apple.com/library/quicktime/scripts/ac_quicktime.js
Comment 1 Jer Noble 2010-11-05 09:29:00 PDT
Would you please describe the symptoms more explicitly?  On the testcase page, I see a QuickTime movie with an HREF parameter which, when clicked, opens the HREF URL in QuickTime Player.

In the WebKit nightly build, which of the following occurs?:

*) The AC_QuickTime.js script is not loaded
*) The script loads but the object tag is not created 
*) The object tag is created, but the movie does not load
*) The movie loads, but clicking on the movie does nothing
Comment 2 Michael McGuire 2010-11-05 10:13:24 PDT
Created attachment 73075 [details]
Webkit errror
Comment 3 Michael McGuire 2010-11-05 10:13:52 PDT
Created attachment 73076 [details]
Safari working
Comment 4 Michael McGuire 2010-11-05 10:14:51 PDT
I believe that it is The AC_QuickTime.js script is not loaded. This has always work until this week..
Comment 5 Michael McGuire 2010-12-01 13:35:17 PST
Created attachment 75316 [details]
Webkit Version

Webkit Version
Comment 6 Eric Carlson 2010-12-01 14:00:01 PST
(In reply to comment #1)
> Would you please describe the symptoms more explicitly?  On the testcase page, I see a QuickTime movie with an HREF parameter which, when clicked, opens the HREF URL in QuickTime Player.
> 
> In the WebKit nightly build, which of the following occurs?:
> 
> *) The AC_QuickTime.js script is not loaded
> *) The script loads but the object tag is not created 
> *) The object tag is created, but the movie does not load
> *) The movie loads, but clicking on the movie does nothing

The AC_QuickTime.js generated markup is supposed to open the url in QuickTime Player when the user clicks on the poster:

<object classid="clsid:02BF25D5-8C17-4B23-BC80-D3488ABDDC6B" width="200px" height="100px" codebase="http://www.apple.com/qtactivex/qtplugin.cab#version=6,0,2,0">
	<param name="src" value="https://tera.usg.utk.edu/graphics/click_me.png">
	<param name="target" value="quicktimeplayer">
	<param name="controller" value="true">
	<param name="href" value="https://tera.usg.utk.edu/movies/PAMS20101104Ref.mov">
	<embed src="https://tera.usg.utk.edu/graphics/click_me.png" width="200px" height="100px" pluginspage="http://www.apple.com/quicktime/download/" target="quicktimeplayer" controller="true" href="https://tera.usg.utk.edu/movies/PAMS20101104Ref.mov">
</object>
Comment 7 Jer Noble 2010-12-01 15:13:06 PST
(In reply to comment #4)
> I believe that it is The AC_QuickTime.js script is not loaded. This has always work until this week..

It appears AC_QuickTime.js is loaded correctly, and correctly generates a nested <object> and <embed> tag.

But it appears that Safari 5.0.2 is loading the <object> tag, while WebKit is loading the <embed> tag.  The <embed> tag, however, does not use the QuickTime plugin to render the click_me.png file.

As a workaround, instead of using a .png file for your poster frame, wrap the poster frame in a QuickTime movie through QuickTime 7 Pro.  (Just drag the .png file on the QTP7 icon, then chose FIle->Save As... in the resulting window.)  This will force the <embed> tag to use the QuickTIme plugin to render the poster frame, and the href behavior will work as expected.
Comment 8 Alexey Proskuryakov 2010-12-01 15:28:44 PST
CC'ing Andy, who would know about object vs. embed precedence.
Comment 9 Eric Carlson 2010-12-02 07:15:38 PST
(In reply to comment #7)
> (In reply to comment #4)
> > I believe that it is The AC_QuickTime.js script is not loaded. This has always work until this week..
> 
> It appears AC_QuickTime.js is loaded correctly, and correctly generates a nested <object> 
> and <embed> tag.
> 
> But it appears that Safari 5.0.2 is loading the <object> tag, while WebKit is loading the <embed> 
> tag.  The <embed> tag, however, does not use the QuickTime plugin to render the click_me.png file.
> 

This is caused by http://trac.webkit.org/changeset/66992. The <object> doesn't have a type <param> and src points to a png so WebKit uses the <embed> "fallback" and renders the png itself.

> As a workaround, instead of using a .png file for your poster frame, wrap the poster frame in 
> a QuickTime movie through QuickTime 7 Pro.  (Just drag the .png file on the QTP7 icon, then chose FIle->Save As... in the resulting window.)  This will force the <embed> tag to use the QuickTIme plugin to 
> render the poster frame, and the href behavior will work as expected.

Another workaround is to include a type <param> in the <object>.

I suspect that this used to work because of the @classid. "clsid:02BF25D5-8C17-4B23-BC80-D3488ABDDC6B" uniquely identified the QuickTime plug-in, so WebKit could restore the old behavior by not rendering fallback content when an <object> has a "well known" @classid.
Comment 10 Alexey Proskuryakov 2010-12-02 09:16:56 PST
Regardless of whether there are workarounds, I don't think we should break a script advertised at www.apple.com and undoubtedly used on many sites.
Comment 11 Michael McGuire 2010-12-02 09:27:27 PST
The fix for me was to delete the current build of WK and replace it with an older version (3/10). WK would not run BUT Safari (5.0.3) worked as it should downloading the .js file and running the movie. I have since deleted WK from my box and rebooted and Safari could not be happier.
Comment 12 Andy Estes 2010-12-02 17:44:30 PST
I believe this actually regressed after http://trac.webkit.org/changeset/70748. This is the changeset that removed all of our mappings from classid to MIME type and forced fallback content to be rendered when there is a non-empty classid attribute.

Falling back in this case is the right thing to do, but it exposes a bug in the way we handle embed elements. Other browsers see the embed element and ask their plug-in database for a plug-in that can handle either the MIME type specified by the type attribute or the file extension of the src attribute. In this case, QuickTime says it can handle .png files and is loaded (although relying on this seems brittle; what if another plug-in claims to support PNGs?).

WebKit does things differently. It shares code with the object element which allows it to load images and nested browsing contexts (iframes) as well as plug-ins. WebKit sees that the src attribute points to an image and completely bypasses the plug-in database, rendering the image natively as if it were specified by an <img>.

I think this behavior is wrong. HTML5 specifies that object elements can load images, iframes and plug-ins, but it restricts embed to plug-ins alone. If we followed that, we'd let QuickTime claim support for .png files, pass all the other embed attributes to the plug-in, and things would work as expected (unless another plug-in lays claim to .png, but that's another issue).
Comment 13 Andy Estes 2010-12-02 18:06:28 PST
I'm assigning this to myself. I broke it; I bought it.
Comment 14 Andy Estes 2010-12-02 18:07:33 PST
<rdar://problem/8724339>
Comment 15 Andy Estes 2011-03-23 19:14:08 PDT
Created attachment 86733 [details]
Patch
Comment 16 Darin Adler 2011-03-23 19:46:26 PDT
Comment on attachment 86733 [details]
Patch

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

Patch looks good. I did not have time to review the whole thing, but I have one comment right now.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:45
> +    // For Gecko compatibility, prefer embeds to use plugins for loading images
> +    // instead of native image rendering.
> +    , m_preferPluginsForImages(hasTagName(HTMLNames::embedTag))

It seems suboptimal for this class to set the policy based on the tag name. Instead there should be a constructor argument to control the policy.
Comment 17 Eric Carlson 2011-03-24 07:11:00 PDT
Comment on attachment 86733 [details]
Patch

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

> Source/WebCore/loader/SubframeLoader.cpp:107
> +         // Application plugins are plugins implemented by the user agent, for example Qt plugins,
> +         // as opposed to third-party code such as flash. The user agent decides whether or not they are
> +         // permitted, rather than WebKit.

I realize that this isn't new, but as long as you are modifying the function, "flash" should be capitalized and "plugin" should be hyphenated, "plug-in" (in comments anyway)
Comment 18 Darin Adler 2011-03-24 10:29:10 PDT
Comment on attachment 86733 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInImageElement.h:46
> +    bool preferPluginsForImages() const { return m_preferPluginsForImages; }

For boolean getters like this we normally try to avoid things that can be interpreted as verb phrases. That’s why you see the name “should” so often.

So I suggest shouldPreferPlugInsForImages and m_shouldPreferPlugInsForImages.

> Source/WebCore/loader/FrameLoader.cpp:1000
> +    bool isSupportedPluginMIMEType = PluginDatabase::installedPlugins()->isMIMETypeRegistered(mimeType);

The word “supported” sounds like a statement about WebKit rather than the plug-ins.
Comment 19 WebKit Review Bot 2011-03-24 16:23:44 PDT
Attachment 86733 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8235650
Comment 20 Andy Estes 2011-03-24 16:35:15 PDT
Committed r81916: <http://trac.webkit.org/changeset/81916>
Comment 21 Andy Estes 2011-03-24 16:43:18 PDT
(In reply to comment #19)
> Attachment 86733 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/8235650

This patch landed before the EWS warning. Build fixed in http://trac.webkit.org/changeset/81917
Comment 22 WebKit Review Bot 2011-03-24 17:21:26 PDT
http://trac.webkit.org/changeset/81916 might have broken Windows 7 Release (Tests)
The following tests are not passing:
fast/images/embed-image.html
Comment 23 Andy Estes 2011-03-24 19:56:10 PDT
I rolled this out in <http://trac.webkit.org/changeset/81933>. I need to re-think the test.
Comment 24 Andy Estes 2011-03-25 15:45:45 PDT
Created attachment 86988 [details]
Patch
Comment 25 Andy Estes 2011-03-25 15:47:12 PDT
(In reply to comment #24)
> Created an attachment (id=86988) [details]
> Patch

Same patch as before, but with a test that will pass on all platforms that support TestNetscapePlugin.
Comment 26 Andy Estes 2011-03-25 16:08:03 PDT
Committed r82001: <http://trac.webkit.org/changeset/82001>
Comment 27 WebKit Review Bot 2011-03-25 17:03:45 PDT
http://trac.webkit.org/changeset/82001 might have broken Leopard Intel Release (Tests)