Bug 15457 – Realplayer plugin does not work correctly if the OBJECT tags defined with the "DATA" attribute has a URI

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Anantha Keesara]

The Realplayer plugin does not work correctly for
the following URLs.
http://www.mozilla.org/quality/browser/front-end/testcases/plugins/ra3.html
http://www.mozilla.org/quality/browser/front-end/testcases/plugins/ra4.html
http://www.mozilla.org/quality/browser/front-end/testcases/plugins/ra2.html

The issue occurs for OBJECT tags defined with the "DATA" attribute which contains a URI.

<object data="http://www.wrn.org/audio/kol_eng1.ram"
  type="audio/x-pn-realaudio-plugin" width="300" height="120" autostart="true">
    <param name="controls" value="default">
    Audio
</object>

This works in Firefox and it seems like it adds an additional "SRC" attribute to the list of parameters
passed to the plugin. This attribute contains the same URI as that in the "DATA" attribute.

[Robert Blaut]

I can confirm the bug as PlatformOnly. The bug is visible only on Windows with RealPlayer 11. Safari for Mac OS X works fine with all of the above mentioned test cases. 

[John Abd-El-Malek]

Created attachment 30940 [details]
Fix

Fix follows what we've been doing in Chrome, and what Firefox does as well (see http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#3045)

[Eric Seidel (no email)]

Comment on attachment 30940 [details]
Fix

Shouldn't we only do this hack for a specific set of plugins?

I would think this code should also be moved into a static function and then only called for that set of plugins which need it.

mapDataParamToSrc() or something.

Also we have a caseInsensitiveEqual which doesn't copy the string like lower() does.

single line ifs don't get { } in WK style.

[Eric Seidel (no email)]

Comment on attachment 30940 [details]
Fix

r- for my above nits.

[John Abd-El-Malek]

Eric: thanks for the review, comments:

> Shouldn't we only do this hack for a specific set of plugins?

Firefox does it for all plugins (and Chrome has done it likewise since launch).  Since I don't know what other old plugins are done this way, I'd prefer to leave as is.  I think if it had caused any problems, FF would have changed it.

> Also we have a caseInsensitiveEqual which doesn't copy the string like lower() does.

I can't find this function?  The only thing I found is stringIsCaseInsensitiveEqualToString but that's in WebCore\platform\mac\WebCoreNSStringExtras.h.

> single line ifs don't get { } in WK style.

done

[John Abd-El-Malek]

Created attachment 31082 [details]
Took out brace brakets

[Anders Carlsson]

Comment on attachment 31082 [details]
Took out brace brakets

I do not think that this is the way to go - What if other plug-ins are confused if they see both a src and a data attribute. I'm not sure what the bug is here - does this affect Firefox? Is it Windows only? Couldn't you just put this workaround in the chrome specific frame loader client?

[John Abd-El-Malek]

(In reply to comment #7)
> (From update of attachment 31082 [details] [review])
> I do not think that this is the way to go - What if other plug-ins are confused
> if they see both a src and a data attribute. I'm not sure what the bug is here

The bug is that some pages are written with data=url attribute, instead of src=url.

> - does this affect Firefox? 

No, because Firefox does the renaming which this patch follows from.

> Is it Windows only? 

I only saw this on Windows, although Firefox does this on all platforms (see the link above to the Firefox source).

> Couldn't you just put this
> workaround in the chrome specific frame loader client?

That's what we have now to fix the problem in Chrome, but I thought moving it to WebKit code would benefit all ports.  These pages don't render correctly on WebKit browsers unless they add this code.

[Eric Seidel (no email)]

(In reply to comment #5)
> Eric: thanks for the review, comments:
> 
> > Shouldn't we only do this hack for a specific set of plugins?
> 
> Firefox does it for all plugins (and Chrome has done it likewise since launch).
>  Since I don't know what other old plugins are done this way, I'd prefer to
> leave as is.  I think if it had caused any problems, FF would have changed it.

Agreed, we should match FF here.

> > Also we have a caseInsensitiveEqual which doesn't copy the string like lower() does.
> 
> I can't find this function?  The only thing I found is
> stringIsCaseInsensitiveEqualToString but that's in
> WebCore\platform\mac\WebCoreNSStringExtras.h.

Yeah, I meant "equalIgnoringCase", I just couldn't remember the name.

[Eric Seidel (no email)]

Comment on attachment 31082 [details]
Took out brace brakets

I don't fully understand Anders comment above since you already seem to have answered at least one of his questions in comment 5 and comment 2.

I still think this would read better if the code was broken out into a static function something like:
static  void mapDataToSrcForOldPlugins(Vector<String>& paramNames, Vector<String>& paramValues)
{ ... }

I'm not sure if .lower() or equalIgnoringCase is faster.  equalIgnoringCase does avoid the allocation caused by lower() though.

Having looked at the FF source and read your additional comments, I agree, we should make this change.  You should consider referencing the FF source in your ChangeLog.

We still need a reply from Anders, but I'm ready to r+ this otherwise.  I'd love to see a new patch w/ the nits fixed, but otherwise this is ready to go.

[John Abd-El-Malek]

Created attachment 31102 [details]
Updated patch

[Eric Seidel (no email)]

Comment on attachment 31102 [details]
Updated patch

This looks good to me.  Anders, please feel free to r- this if you're still opposed.  As far as I can tell jam's change makes us match firefox.

[Eric Seidel (no email)]

Comment on attachment 31102 [details]
Updated patch

Actually, it should be possible to make a test case for this, no?

We'd put the test case under platform/win I expect, or maybe we'd just skip it on the Mac.  But we should be able to make a simple layout test which confirms this is fixed, no?

[John Abd-El-Malek]

Created attachment 31120 [details]
Added regression test

I spent half a day on this, but it doesn't look like layout tests are runnable on Windows at the moment.  I've verified what I can by running the plugin dll in the browser and tracing through the plugin.  I don't have a mac (one on order), so I couldn't build and verify that this test passes.  Can you verify that the test passes for you on Windows?

[John Abd-El-Malek]

oops, I meant "verify it works for you on Mac"

[John Abd-El-Malek]

Created attachment 31121 [details]
Fixed misspelling.

[John Abd-El-Malek]

btw here's the companion change to chromium: http://codereview.chromium.org/118486

I was able to successfully run the layout test there on Windows.

[Eric Seidel (no email)]

Comment on attachment 31121 [details]
Fixed misspelling.

You did more than what I had expected.  I figured you'd just add a test which used realplayer.

That said, I think I would change this a little bit to just store off the parameters to that JS can access them later.  that way we can test things like param ordering too. :)

I'll fix up the test some when I land this for you later.

[John Abd-El-Malek]

ping?  I think this patch is ready to check in, since it's independent of checking for order of parameters.

[Eric Seidel (no email)]

Agreed.  Sorry for the holdup.

[David Levin]

I went to land this but the changelogs haven't been filled out for LayoutTests and WebKitTools

[John Abd-El-Malek]

Created attachment 31567 [details]
added missing ChangeLogs

[Eric Seidel (no email)]

Transfered review flag to newest patch.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/plugins/netscape-plugin-map-data-to-src-expected.txt
	A	LayoutTests/plugins/netscape-plugin-map-data-to-src.html
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderPartObject.cpp
	M	WebKitTools/ChangeLog
	M	WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp
	M	WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.h
	M	WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp
	M	WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp
Committed r45324
http://trac.webkit.org/changeset/45324

[Eric Seidel (no email)]

Sorry that took so long.  In the end, I did not make any modifications to the posted patch.