82415 - nsIPluginStreamListener changed!

Status: VERIFIED FIXED

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

Description

[Doug Turner (:dougt)]

The public interface nsIPluginStreamListener has changed!  This breaks MRJ.  We
need to revert this interface.

TO provent this in the future, we should add a LARGE comment in the header file
regarding the "locked down" state.

Also, take a look at the nsIPluginStreamListener.idl file.  The def there is
wrong.

Comment 1

[Phil Peterson]

If this is easy, let's do it for 0.9.1. But if it's not, we'd consider backing
out the change and allowing byte range plugins to be broken. -- Phil & PDT.

Comment 2

[Peter Lubczynski]

This should be simple to fix.

1) Create a new interface with the extra argument in the signature for ODA
2) Hook it all up
3) Back out MRJ changes

Doug, let me know if there is anything else. I'm pulling a fresh tree to set 
this up. Mozilla 0.9.1 can't go out the door like this.

Comment 3

[Peter Lubczynski]

Attachment: first crack at nsIPluginStreamListener2

Comment 4

[Peter Lubczynski]

Attachment: all inclusive patch (try #1)

Comment 5

[Peter Lubczynski]

Note: You'll ALSO have to use these commands to back out changes in MRJ:

cvs update -j1.6 -j1.5 mozilla/plugin/oji/MRJ/plugin/Source/nsLiveConnect.h
cvs update -j1.5 -j1.4 mozilla/plugin/oji/MRJ/plugin/Source/nsLiveConnect.cpp
cvs update -j1.13 -j1.12 mozilla/plugin/oji/MRJ/plugin/Source/MRJPlugin.h
cvs update -j1.32 -j1.31 mozilla/plugin/oji/MRJ/plugin/Source/MRJPlugin.cpp
cvs update -j1.4 -j1.3 mozilla/plugin/oji/MRJ/plugin/Source/JSEvaluator.h
cvs update -j1.4 -j1.3 mozilla/plugin/oji/MRJ/plugin/Source/JSEvaluator.cpp

To verify this patch did the trick, the MRJ should compile as it did before 
DougT's changes.

Comment 6

[Peter Lubczynski]

Attachment: Patch to revert changes in MRJ plugin

Comment 7

[Peter Lubczynski]

Attachment: new patch for reverting changes in MRJ plugin (forgot a file)

Comment 8

[Peter Lubczynski]

I tested these patches on the Mac, and doing a complete debug build. 

Comment 9

[Peter Lubczynski]

Attachment: reverting npsimple [not part of main build]

Comment 10

[Doug Turner (:dougt)]

It is too bad that we have to add a new interface plugin interface.  Would it
not be cleaner to just QI'ing the nsIInputStream to some interface that give you
absolute offset positioning?  

Since this is needed for 0.9.1 I guess it doesn't matter too much; Couple of
things with the attached patches:

npsimple is part of the 'nix builds.

Add a comment in the header file where you declare the new OnDataAvailable()
call stating that this is part of the nsIPluginStreamListener2 interface.

is the new code for the old ns4xPluginStreamListener::OnDataAvailable exactly
the same prior to my changes?

Comment 11

[Peter Lubczynski]

Doug, I don't quite see what you mean with using nsInputStream?

And yes, ns4xPluginStreamListener::OnDataAvailable (3) is exactly the same as 
before your patch. I was trying to have both ODA call the same code but ran into 
some weird problems so I figured going back to what was there before woudln't 
hurt.

Beard, can you review these patches as well, especially the ones to the MRJ.

Comment 12

[Doug Turner (:dougt)]

Creating a new implementation behind the nsIInputStream which can be QI'ed to an
interface which can return the absolute position for the current read is cleaner
than adding a new plugin interface. 

The reason that this is a cleaner approach is that we do not have to change the
signatures of any public interfaces.  Basically, anyone can simply QI the
nsIInputStream parameter of OnDataA for an interface which returns absolute file
positioning.


So, something like:

NS_IMETHODIMP
ns4xPluginStreamListener::OnDataAvailable(nsIPluginStreamInfo* pluginInfo,
                                          nsIInputStream* input,
                                          PRUint32 length)
{
   ...
   
   // check to see if the input has an absolute offset
   nsCOMPtr<nsIByteRangeRequest> brr(do_QueryInterface(input));
   if (brr)
      brr->GetStartRange(&absoluteOffset);

   
   // use absoluteOffset as needed.
   ...

Comment 13

[Peter Lubczynski]

The next attached patch (made from modules/plugin) is ready to go. This patch 
actually makes RequestReads work! Here are some testcase from another bug:

http://access.adobe.com/browser/netscape/readtesting/gotopage5.pdf
http://access.adobe.com/browser/netscape/readtesting/starr.pdf

Issues:
=======
I like DougT's suggestion as it is much cleaner, but the mozilla 0.9.1 train is 
about to leave and this can't afford to miss it. I'll open another bug after 
this is checked in to track this issue.

Sean's check-in last week of a bug to fix a leak actually broke RequestReads 
because the stream was being destroyed too early. I've added an extra AddReff to 
the stream peer, if and only if, the stream is a byte range. The problem is that 
I'm not quite sure where I'm able to release it. I needs to change something so 
that a reference is kept to the stream peer in the instance (or instance 
peer) and then I can call release when the instance goes away. I'll open a new 
bug on that. But for now, I'd like to get in what I have.

Comment 14

[Peter Lubczynski]

Attachment: new inclusive patch for modules/plugin

Comment 15

[Doug Turner (:dougt)]

Two nits:

Let put a comment in the nsIPluginStreamListener2 IDL stating that this
interface is for internal use and plugin developer should not use it.  

Comment the heck out of that addref in RequestRead.  It is going to bite someone
if you don't.

Also, did you figure out the place where you can do a release on that extra
addref in RequestRead?  Can you do it when a non-segmented ranges come in
(0-2,1-3)?  If not, how much will we leak?

Comment 16

[Peter Lubczynski]

I've opened [Bug 83183] nsPluginStreamListenerPeer is leaked for Byte Range 
requests to deal with the leak. I think the plugin instance my need to keep 
around a ref to the stream peer to the stream will get destroyed then the 
instance goes away. Av?

Also opened Bugzilla Bug 83186 nsIPluginStreamListener2 should be removed to 
address DougT's comments.

Updated patch with Marc's and Doug's latest comments coming. If that patch is 
okay with you guys, can I get a sr/r stamp in the bug for approvals?

Thanks!

Comment 17

[Peter Lubczynski]

Attachment: update patch with Marc and Doug's comments

Comment 18

[Doug Turner (:dougt)]

r=

Comment 19

[Marc Attinasi]

sr=attinasi for a slightly improved patch I saw on Peter's machine. Peter, you
might just want to attach the new patch (it allows QI to both the old and new
interfaces).

Comment 20

[Peter Lubczynski]

Attachment: new patch using NS_IMPL_ISUPPORTS2(ns4xPluginStreamListener, nsIPluginStreamListener, nsIPluginStreamListener2)

Comment 21

[Peter Lubczynski]

PDT: I know it looks like a lot of code is being added to 
ns4xPluginStreamListener::OnDataAvailable, but I'm just reverting ODA back to 
exactly what it was before.

Comment 22

[Doug Turner (:dougt)]

Just a little more clarity:  There are TWO onDataAvailable calls defined in this
file.  The existing one allows for an absolute byte position.  The one Peter has
added in his patch is EXACTLY the same as the ODA priot to my hacking.  Net
result is both of these functions have been review, super reviewed, and tested.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

a=dbaron for 0.9.1.

One thing I noticed while looking at it, though:
---
@@ -670,11 +790,11 @@
 	if(stream == nsnull)
 
	return NS_ERROR_OUT_OF_MEMORY;
 
 	NS_ADDREF(stream);  // Stabilize
     
-    nsresult res = stream->QueryInterface(kIPluginStreamListenerIID,
(void**)listener);
+    nsresult res = stream->QueryInterface(kIPluginStreamListener2IID,
(void**)listener);
 
     // Destabilize and avoid leaks. 
     // Avoid calling delete <interface pointer>
     NS_RELEASE(stream);
---
Why did you change the IID used without changing the type of |listener|?  These
should always be the same.  (Yes, I know nsIPluginStreamListener2 inherits from
nsIPluginStreamListener, but it still seems evil even though it happens to work.)

Comment 24

[Peter Lubczynski]

Patchs checked in, marking FIXED.

I took David Baron's suggestion and changed the QI line back to what it was.

Comment 25

[shrirang khanzode]

verif that the patch was  checked in. Marking as such.