292114 - InstantiateFullPagePlugin should take nsIURI*, not nsString&

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement, P4)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

InstantiateFullPagePlugin has exactly one caller. That caller would have an
easier time passing an nsIURI than an nsString&. Additionally, the first thing
that this function does with the string is to construct an nsIURI* from it. This
also means that the originCharset is lost.

So, it should just take an nsIURI* (like the other two functions taking an URI
on this interface)

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181981 [details] [diff] [review]
patch

>Index: modules/plugin/base/src/nsPluginHostImpl.cpp
>+#ifdef PLUGIN_LOGGING

#if, not #ifdef, to be consistent with header...

>+        mType.AssignASCII(aPluginTag->mMimeTypeArray[aMimeTypeIndex]); // MIME Types better be ASCII

This comes from the plugin, so I'm not sure we should trust it, particularly...
it could send all sorts of garbage data.  If this is safe enough to do on
non-ASCII data, then ok.

sr=bzbarsky, with those addressed.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

> #if, not #ifdef, to be consistent with header...

but #ifdef to be consistent with the rest of the .cpp file...

> If this is safe enough to do on non-ASCII data, then ok.

hm, no, AssignASCII sorta fails for nonascii data (sign-extends it); I'll remove
that change

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 181981 [details] [diff] [review]
patch

r=jst

Comment 5

[Boris Zbarsky [:bzbarsky]]

> but #ifdef to be consistent with the rest of the .cpp file...

Yay.. Want to just fix the header to follow the same convention?  That would
make more sense anyway.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

- use ifdef PLUGIN_LOGGING everywhere
- removed AssignASCII block

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 182003 [details] [diff] [review]
patch v2

low risk code cleanup patch

Comment 8

[Asa Dotzler [:asa]]

Comment on attachment 182003 [details] [diff] [review]
patch v2

a=asa

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in.

Comment 10

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050429
Firefox/1.0+

plugins won't install anymore since today

Comment 11

[Boris Zbarsky [:bzbarsky]]

Won't install?  Or won't run if you load a relevant URL in the browser?  This
patch could only have affected the latter..

Comment 12

[Steve England [:stevee]]

For me
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050429
Firefox/1.0+ (10:07 PDT)
Existing plugins in my /plugins dir aren't recognised. Going to a page that
requires Flash results in the plugin placeholders inviting me to install flash.
Upon clicking on a plugin placeholder to install it, the motions are gone
through, the plugin downloaded, the installation "apparently" complete, but when
the page refreshes the plugin placeholders are still present and inviting me to
install the plugin I have just installed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Steve, Peter, do plugins using <object> or <embed> elements work?

Comment 14

[Mike Pesce (:By-Tor)]

The point is, upon loading FF no plugins are found.  Existing plugins are
ignored and new plugins - while in the \plugins directory - are not recognized.
 This seems to be a major issue.

Comment 15

[Steve England [:stevee]]

about:plugins returns "No plug-ins are installed" even tho
1) My /plugins dir has many plugins listed in it
2) I try and install plugins when requested and it doesn't work (in that, after
installation, the about:config msg is still "No plug-ins are installed" and the
page I was trying to install plugins for still informs me that the plugins
aren't installed)

Comment 16

[Boris Zbarsky [:bzbarsky]]

If no plugins are found at all (check about:plugins, say) then it's not caused
by this bug.  So how about answering the question in comment 13, which would
help us sort out what's actually going on, eh?

Comment 17

[Mike Pesce (:By-Tor)]

Neither <object> or <embed> objects work.  In both cases I'm prompted to install
a plugin.

Comment 18

[Steve England [:stevee]]

1. Create a new profile and start firefox with it.
2. Go to http://www.macromedia.com/shockwave/welcome/
3. Note that additional plugins are required to display all the media on this page.
4. Choose to install Flash.
5. Next > I Agree > Next. Download of plugin starts. Progress bar rises.
6. Download completes. Click Finish.
7. Page reloads itself. Note that flash is not installed, and the animation
playing that should show that Flash has installed is still the plugin
placeholder inviting you to install Flash.

I can only find pages that use Flash that use <EMBED>. I can not comment upon
whether this also happens for <OBJECT>.

Want one of us to file a new bug bz?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment 16 was in response to comment 14.

Assuming Steve meant about:plugins, not about:config, in comment 15, it sounds
like there is a general issue with plugin registration.  All this bug did was
change plugin _instantiation_, and only for full-page plugins...

Now this bug _did_ change the pluginhost IID.  So if code was relying
specifically on the old IID (say depend builds with broken dependencies, or
people hardcoding IIDs or something) that could cause issues.  There really
shouldn't be any code like that in the tree, though...

Note also that there were some changes to plugin installation for toolkit
recently.  Could those be causing the problem here?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Steve, please do file a new bug.  Mark it smoketest blocker, and if you can get
a one-day or better regression range, that would rock.  Please cc me on that bug.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

I want to note that I did of course test my patch, and flash was not broken.
that was in seamonkey.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

hm, but then, it was on linux...

Comment 23

[Peter van der Woude [:Peter6]]

(In reply to comment #20)
> Steve, please do file a new bug.  Mark it smoketest blocker, and if you can get
> a one-day or better regression range, that would rock.  Please cc me on that bug.
He filed Bug 292373

Comment 24

[Steve England [:stevee]]

Apologies for the wild and inaccurate accusations Christian Biesinger :)
Plugin installation works fine in the official nightly, which includes this
patch - so you are not to blame.
I assume too much stuff - plugins stop working, I see a patch that was for
plugins, and put 2 and 2 together and get 5.