88998 - Current plug-in files need to be reviewed and updated to reflect the current state and status of each file, making adjustments and modifications as demed appropriate (formally titled: [meta] Plugin code cleanup tracking bug

Status: RESOLVED INVALID

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Chris Waterson]

So, I spent the day today trying to figure out why the plugin code can't 
properly handle a reframe. I started by discovering that nsObjectFrame doesn't 
implement nsIStatefulFrame, and wound up realizing that there is quite a bit of 
stuff that could use some rework here. Some rework is major, some is fairly 
minor; for example,

  - We appear to have lost any notion of abstraction; nsObjectFrame,
    nsPluginInstanceOwner, nsPluginHostImpl, and nsPluginInstance all
    seem to be randomly inter-twined. These objects should be teased
    apart so that (e.g.) nsObjectFrame isn't twiddling the plugin instance
    directly.

  - We should be using nsIStatefulFrame rather than maintaining a list
    of stopped plugin instances in the plugin host object.

  - nsIObjectFrame should only have one method, |GetPluginInstance()|

Filing this bug as a placeholder to track cleanup, simplification, and 
refactoring of the plugin code.

Comment 1

[Chris Waterson]

Attachment: fix nsIObjectFrame, move nsPluginInstanceOwner into its own file, cleanup nsObjectFrame::Reflow()

Comment 2

[Chris Waterson]

The above patch

1. Removes all the unnecessary methods from nsIObjectFrame

2. Separates nsPluginInstanceOwner into its own file, mostly for my own
   sanity.

3. Makes a first stab at pulling all the insanity out of
   nsObjectFrame::Reflow(), while cleaning up string-fu and poor style
   at the same time.

4. Dribbles stylistic cleanup and XXX comments throughout.

Comment 3

[Peter Lubczynski]

Wow what a patch! Since youv'e sperated nsPluginInstanceOwner,
nsPluginViewer.cpp could really use that version instead of the duplicate code
inside the file now.

Comment 4

[Chris Waterson]

- The way we handle <object>s that are images is really gross. nsObjectFrame
  should really be a leaf, but it's a container because it might contain an
  image frame. Ick. I wonder if we should merge nsImageFrame and nsObjectFrame,
  or make each inherit from a common base class (nsReplacedFrame)?

- Seems like the "clsid:browser" hack (does this even work?) should be moved
  out to the plugin module, and dealt with ``cleanly'' by registering a plugin
  handler for that clsid that instantiates a nested browser.

Comment 5

[Peter Lubczynski]

There is also some old ActiveX stuff that can be removed. cc:ing Karanze as he
had fixed up nsObjectFrame::Reflow() before, but those changes caused some
regressions.

Comment 6

[Peter Lubczynski]

See bug 76602 for re-arranging directories and files.

Comment 7

[Chris Waterson]

Yeah, that patch above is a work-in-progress. It breaks Java, at a minimum 
(although I'm not sure why). Anyway, I'm going to try to break it out into bite-
sized patches.

On another tangent, I'm wondering why we need to have a separate object 
implementing nsIPluginInstanceOwner at all? One obvious reason would be if the 
back-end needed a refcountable object to hang its hat on (e.g., because the 
nsObjectFrame can go away).

- nsPluginStreamListenerPeer wants to hold a strong ref to the owner,
  apparently so that it can instantiate the real plugin when the stream
  starts to arrive. This seems like the toughest case to handle, because
  >1 stream could be created per plugin instance; but it also seems like
  the only time that it really _needs_ the owner is when its the first
  inbound stream, and the plugin hasn't yet been instantiated.

- The nsPluginStreamToFile also wants an owner, so that it can get the URL
  (but it seems like this could be computed at creation time).

- The nsPluginInstancePeer obviously has an owner, but it seems like it
  doesn't need to hold an owning reference, and clearing its back-pointer on
  frame destruction would be sufficient to make it work right.

Comment 8

[Chris Waterson]

On another tangent, do we have a good set of plugin regression tests?

Comment 9

[Peter Lubczynski]

I don't know of any formal regression tests. I think we could really use some.

You can find some tests at:
http://slip.mcom.com/shrir
http://panther.mcom.com/testcases

I think it's hard to have automated regression tests for plugins because events
and timing play a big role in reprouding bugs. Also the 4.x API is very strict,
even a small could change the order of calls and break fragile plugins like
Shockwave which do weird things like swap our plugin entry points while running
and installing.

I don't quite understand what you mean about nsIPluginInstanceOwner? One base
class for both full-page and embeded plugins would be ideal instead of the dup
code today. But I do think that nsIPluginInstanceOwner is needed. It's sort of a
bridge between the plugin instances and layout.

There is also the byte range case to handle, which is kind of tricky. In this
case, any byte range streams for any plugin instances must stay open until
either the plugin kills the stream or the plugin is stopped (for like a page
transition).

Comment 10

[Peter Lubczynski]

Adding...
bug 90268  object frame should be able to withstand a reframe
bug 90256  Can't script Fullpage Plugins (Jazmin should attach a patch that
claims to remove the need for dual nsPluginInstanceOwners)

Comment 11

[Peter Lubczynski]

adding...
bug 83186 nsIPluginStreamListener2 should be removed

Comment 12

[Peter Lubczynski]

The meta tracking plugin clean up bug has the the starter patch by Waterson to
split nsPluginInstanceOwner to it's own file.

-->Andrei..

Comment 13

[rubydoo123]

based on the milestone for bug 89077, moving to future

Comment 14

[rubydoo123]

*** Bug 39907 has been marked as a duplicate of this bug. ***

Comment 15

[rubydoo123]

Since the Summary on this bug has created much anxiety for a few folks, I am 
updating the Summary so as to help with that issue. Please note, this is the 
bug that the plug-ins team will use to annotate issues in regard to files 
within the plug-in module. 

Comment 16

[Marek Z. Jeziorek]

batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html

Comment 17

[av (gone)]

Peter, I guess this is a kind of duplicate of what you are working on now. 

Comment 18

[rubydoo123]

moving to 1.4 beta: plug-in branch work

Comment 19

[Josh Aas]

This bug is no longer useful for anything.