127259 - Url Notifies incorrectly blocked on NULL notify Data; breaks Windows Media Player in Linux

Status: VERIFIED FIXED

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

Description

[Jeremy White]

When testing Windows Media Player, we discovered that it did not receive
NPP_URLNotifys as it should.

This was because of the following code in 
mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp, line 160
  if (callbacks->urlnotify && mNotifyData) {

This is invalid, a NULL mNotifyData is a perfectly legitimate
parameter value to pass to an NPN_GetURLNotify call
(see
http://developer.netscape.com/docs/manuals/communicator/plugin/pgfn2.htm#1007668
)

Changing this simply to:
  if (callbacks->urlnotify) {

solves the problem nicely.

Comment 1

[Jeremy White]

Attachment: Proposed fix

Comment 2

[Peter Lubczynski]

Nice patch, but won't this ALWAYS call urlnotify on destroystream, even for
non-notify streams?

What about setting a new member variable flag in
ns4xPluginInstance::NewNotifyStream? (might also have to clear it in NewStream)

Comment 3

[Jeremy White]

You're right, I completely overlooked that; your solution is better
than mine.

Comment 4

[Peter Lubczynski]

Attachment: using a flag for notify

Jeremy, does this patch work better?

Comment 5

[R.K.Aa.]

built with the patch in attachment 70923 [details] [diff] [review], and later with the patch in attachment
70935 [review]. Both work like charm. The second patch is probably more correct.

Quite a moment to see MS MediaPlayer play along in Mozilla on Linux.

Comment 6

[serge (gone)]

Peter, why do not just add an additional parameter to 
ns4xPluginInstance::NewNotifyStream(nsIPluginStreamListener** listener, void* 
notifyData, 
PRBool bCallNotify)
and call 
ns4xPluginInstance::NewStream(nsIPluginStreamListener** listener)
{
  return NewNotifyStream(listener, nsnull, PR_FALSE);
}
so, you do not have to dup a code in this case.

Comment 7

[Peter Lubczynski]

Attachment: new patch

I didn't want to change the interface, but it's not public! Here's a better
patch. Please review.

Comment 8

[serge (gone)]

Yes, I like it better.
but I just noticed the calls _geturl & _geturlnotify; _posturl & _posturlnotify 
also have 90% of duplicate code.
Do you think it is worth to implement those pair in single call?

Comment 9

[Peter Lubczynski]

Attachment: consolidate duplicate code

something more like this?

Comment 10

[R.K.Aa.]

the last patch (attachment 70973 [details] [diff] [review]) crashed my build:

ns4xPlugin.cpp: In function `NPError _geturlnotify (NPP_t *, const char 
*, const char *, void *)':
ns4xPlugin.cpp:724: no matching function for call to 
`ns4xPluginInstance::NewNotifyStream (nsIPluginStreamListener **, void 
*&, int)'
ns4xPluginInstance.h:136: candidates are: nsresult 
ns4xPluginInstance::NewNotifyStream (nsIPluginStreamListener **, void 
*)
ns4xPlugin.cpp: In function `NPError _posturlnotify (NPP_t *, const 
char *, const char *, unsigned int, const char *, unsigned char, void 
*)':
ns4xPlugin.cpp:763: no matching function for call to 
`ns4xPluginInstance::NewNotifyStream (nsIPluginStreamListener **, void 
*&, int)'
ns4xPluginInstance.h:136: candidates are: nsresult 
ns4xPluginInstance::NewNotifyStream (nsIPluginStreamListener **, void 
*)
ns4xPlugin.cpp: At top level:
ns4xPlugin.cpp:835: warning: `class ns4xStreamWrapper' has virtual 
functions but non-virtual destructor
ns4xPlugin.cpp:1408: warning: `java_lang_Class *_getJavaClass (void *)' 
defined but not used
gmake[4]: *** [ns4xPlugin.o] Error 1
gmake[4]: Leaving directory 

Comment 11

[R.K.Aa.]

heck.. i deleted the patch - i may be lying! Trying again now.

Comment 12

[serge (gone)]

well, _geturlnotify, _posturl, _posturlnotify are not implemented in last patch 
yet.

Comment 13

[serge (gone)]

I mean - not implemented with new approach.

Comment 14

[serge (gone)]

Opps, my bad I didn't read it properly:( 
I'm going to apply the patch and see how it works.

Comment 15

[serge (gone)]

Peter you forgot to include in the patch changes in ns4xPluginInstance.h

Comment 16

[serge (gone)]

+nsresult ns4xPluginInstance::NewNotifyStream(nsIPluginStreamListener** 
listener, 
+                                             void* notifyData,
+                                             PRBool aCallNotify)
...
You have to send aCallNotify to SetCallNotify(), and assign mCallNotify to it
+  stream->SetCallNotify(); 

Comment 17

[Peter Lubczynski]

Attachment: patch v.5

woops...forgot a file and had the same problem as the first patch. Try this
one.

Comment 18

[serge (gone)]

it look better,
but I'm confused
   if(target == nsnull)
-    inst->NewStream(&listener);
+    ((ns4xPluginInstance*)inst)->NewNotifyStream(&listener, notifyData, 
bDoNotify);
means we are calling  NPP_URLNotify() only for (target == 0) what about other 
targets?

Comment 19

[R.K.Aa.]

This one builds, but something is very wrong suddenly:

MS Media Player plays (allthough the frame containing it often has to be
reloaded first time, to get it going - but it was like that in the previous
working patches as well)

What's wrong now though, and which still worked after attachment 70923 [details] [diff] [review] and 70935
  is that loading of flash animations stall.

http://www.macromedia.com will load the first part of it's front page flash
animation, and will then show "Loading" forevermore. The browser is responsive
and fine, and Media Player plays like nothing had happened.

I can still click the "tabs" and make the flyers...fly out, but they too say
"Loading".

I'm 95% sure i double-checked flash animations in combo with various other
plugins after applying the first working patches, and all worked fine then.
I'll apply the earlyer working patch to another tree to verify i'm not seeing
ghosts now.

Comment 20

[serge (gone)]

Peter, you are not sending notifyData in 
NPError NP_EXPORT
_geturlnotify(NPP npp, const char* relativeURL, const char* target, void* 
notifyData)
{
  NPN_PLUGIN_LOG(PLUGIN_LOG_NORMAL, 
    ("NPN_GetURLNotify: npp=%p, target=%s, notify=%p, url=%s\n", (void*)npp, 
target, notifyData, relativeURL));

  return Create4xStream (npp, relativeURL, target, eNPPStreamType_Get, PR_TRUE);
}
and some more
what do you think on change Create4xStream to something like internal4xURLProc?

Comment 21

[R.K.Aa.]

Viva the 5 percents... I was wrong, as you probably realized:

-macromedia.com loads fine with the first patch from Jeremy (attachment 70923 [details] [diff] [review])
and MediaPlayer works.

-macromedia.com doesn't load *at all* when building w/attahment 70935, but
MediaPlayer works. Simple flash animations also work.

-macromedia.com only partly loads with patch in attachment 71007 [details] [diff] [review], then stalls.
Browser still responsive, MediaPlayer still happy.

Haven't clobbered between builds but deleted cache and appreg in between tests.
Using the native linux plugin for flash; libflashplayer.so

Comment 22

[Peter Lubczynski]

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

Comment 23

[Peter Lubczynski]

There are a lot of other bugs on URLNotify and I'm not sure it's safe to create
a 'notify' stream for non-null targets. I think this kind of stream will feed
back to the plugin.

However, we should probably be calling URLNotify in the stream's destructor so
that we catch cases where NewStream hasn't been called. See bug 56311.

Comment 24

[Peter Lubczynski]

Attachment: patch v.6

I fixed the calling arguments and names, plus added the opportunity for
URLNotify to be called more often.

It's still not fixed for non-null targets which I'd like to leave for other
bugs. I think that'd require a bit more complexitiy since we're not handeling
the stream internally.

Comment 25

[serge (gone)]

I think ns4xPluginStreamListener::CallURLNotify() should get a parameterNPReason
reason,
void ns4xPluginStreamListener::CallURLNotify(NPReason reason)
{
  if (!mCallNotify || !mInst || !mInst->IsStarted())
    return;
 
  const NPPluginFuncs *callbacks = nsnull;
  mInst->GetCallbacks(&callbacks);

  if (callbacks &&* callbacks->urlnotify) {
    NPP npp;
    mInst->GetNPP(&npp);
    mCallNotify = PR_FALSE;

    NS_TRY_SAFE_CALL_VOID(CallNPP_URLNotifyProc(callbacks->urlnotify,
                                                npp,
                                                mNPStream.url,
                                                reason,
                                                mNotifyData), mInst->fLibrary );
    NPP_PLUGIN_LOG(PLUGIN_LOG_NORMAL,
    ("NPP URLNotify called: this=%p, npp=%p, notify=%p, url=%s\n",
    this, npp, mNotifyData, mNPStream.url));
  }
}
---
+static NPError MakeNew4xStreamInternal (NPP npp, 
...
+  case eNPPStreamTypeInternal_Get:
+    {
+      if(NS_FAILED(pm->GetURL(inst, relativeURL, target, listener)))
Are we leaking ns4xPluginStreamListener object here?
Do we have to notify plugin about an error?
+        return NPERR_GENERIC_ERROR;
+      break;

Comment 26

[Peter Lubczynski]

>> Are we leaking ns4xPluginStreamListener object here?

I think I see a leak, I'm not getting matched destructor calls on this class.

Looks like in ns4xPluginInstance::Stop(), nsInstanceStream is deleted but
something is still holding a reference to mPluginStreamListener.

Comment 27

[Peter Lubczynski]

Attachment: patch v.7

Fixed the leak by ensuring we release the ns4xPluginStreamListener object when
we stop the plugin instance. Also propigated the reason code. Please review.

Comment 28

[serge (gone)]

I have only 2 questions:
1) 
ns4xPluginStreamListener::~ns4xPluginStreamListener(void)
 {
+  // for those cases when NewStream is never called, we still may need to fire 
a
+  // notification callback
+  CallURLNotify(NPRES_USER_BREAK);
Why NPRES_USER_BREAK ?
---
2)
+    // remove ourselves from the list before releasing to prevent recursive 
delete
+    NS_IF_RELEASE(listener);
this fixes the leak, but I do not understand why we are keeping one extra 
mRefCnt in this object and delete it only when  page is getting closed? 
I've used tester plugin to test NPN_GetURLNotify() with target==NULL.
the ns4xPluginStreamListener object created in MakeNew4xStreamInternal()
+    ((ns4xPluginInstance*)inst)->NewNotifyStream(&listener, notifyData, 
bDoNotify);
is getting freed only when I leave the page with tester plugin.
Is this expected behavior?

Comment 29

[Peter Lubczynski]

Attachment: patch v.8

Okay, I think this one may be better. Looks like the extra ADDREF was caused by
NewNotifyStream. I now release it after we call notify.

Comment 30

[serge (gone)]

Comment on attachment 71557 [details] [diff] [review]
patch v.8

r=serge

Comment 31

[R.K.Aa.]

The latest patch might also break the Linux flash version - tested at
http://www.macromedia.com

Just built with attachment 71557 [details] [diff] [review] on linux and it's the same as in previous
patch: Moz will now allow MediaPlayer to play at CNN.com (would otherwise be
blank) but macromedia site goes numb.

It suddenly doesn't realize i have flash at all, and when i click link to "click
here if you have flash installed" the flash animation doesn't load properly, but
remains mostly greyish. This happens also with a clean appreg.

The only patch till now that have allowed both plugins to work is the very first
one.

The solution I found to work was to trash libflashplayer.so and activate the
WINE version instead. Then things work fine. 

I'll clobber and test the latest patch in a fresh tree with libflashplayer.so as
only installed plugin, and then with the WINE plugins less flash, to make sure
nothing else influenced this in the meanwhile.

Comment 32

[serge (gone)]

Hmm, that is strange, I see no problem with the patch in attachment 71557 [details] [diff] [review]
on w2k and rh7.2.
Peter, did you try it on Mac?

Comment 33

[R.K.Aa.]

My bad: The patch is good!

The linux version of flash is 4.0 r12 and doesn't normally play flash at that
page at all. Instead, the page will load with an image. I had gotten a cookie
set for my "own" page at macromedia while testing the Windows version of Flash5.
When I later removed both appreg and the flash5 plugin, the cookie still
remained, sending me off to the flash5 enabled startpage. No wonder it didn't work.

The new patch also works better than the original patch: With that I still had
to reload the frame with the MediaPlayer plugin in order to "trigger" a load at
times. Now it starts playing all on its own, as it should.

Comment 34

[R.K.Aa.]

Mozilla now for the first time plays the games like
http://www.shockwave.com/sw/content/mphearts
(Shockwave for Director under Linux via Crossover). Couldn't get those going
before, but still have to occationally reload to trigger.

Watched memory usage and CPU and played for an hour now:
Nothing unusual. *Everything* works.

It would be just great if this fix got into 0.9.9. Crossover 1.1.0 is out real
soon and it would be nice if also Mozilla users can take advantage of it from
day one.

Comment 35

[Peter Lubczynski]

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

Comment 36

[R.K.Aa.]

Is this the same issue as in bug 56311, bug 112399 and bug 117766?

Comment 37

[Winnie Lam]

nominating to nsbeta1+ as per adt triage.  patch already exists - just needs to
be tested.

Comment 38

[R.K.Aa.]

I've been "stress testing" the patch in action for around three days now, with
these installed plugins, Windows version running via Crossover 1.1.0:

Java(TM) Plug-in 1.3.1-b24 (Linux)
Shockwave Flash 4.0 r12 (Linux)
rpnp.so (RealPlayer, Linux)
Adobe SVG Viewer (3.0 beta, Linux)
nppdf.so (Acrobat Reader v.4 plugin, Linux)
Windows Media Player Plug-in (6.4 for Windows)
Shockwave for Director (8.5 for Windows)
Shockwave Flash (5.0 r41 for Windows)
QuickTime Plug-in (5.0.2 for Windows)
Windows Media Services ("drm" for Windows)
Authorware Web Player (6.0 F1 for Windows)
iPIX Plugin (6.2 for Windows)

I haven't found a single problem. I've kept an eye on CPU and RAM usage, but
nothing unusual to see. Suddenly all works... Mozilla is simply a lot more fun
after this patch :)

Again: I *really* wish this to be checked in to 0.9.9. It's more than good
enough. And should there be problems I now haven't come across, it would be good
to give a broader audience a chance to discover them before 1.0.

Comment 39

[Joe Drew (not getting mail)]

Who can sr= this? I think everyone involved wants it to make 0.9.9.

Comment 40

[Peter Lubczynski]

Patrick,

Can I get a super review?

Comment 41

[Patrick C. Beard]

Comment on attachment 71557 [details] [diff] [review]
patch v.8

sr=beard
LONG PATCH!

Comment 42

[Joe Drew (not getting mail)]

Looking for approval. Is it possible to get this in for 0.9.9? Also, should
[e-mail] be CC:ed or e-mailed about this? Someone in the know for
bugs please do so :)

Comment 43

[Bradley Baetz (:bbaetz)]

For approval, you need to mail [e-mail]. Start the mail with [branch
approval] for the branch, and [trunk approval] for the trunk.

I don't like your chances for the branch though....

Comment 44

[R.K.Aa.]

Again: With this patch, i can use/view "Shockwave for Director" and
"MediaPlayer" files under Mozilla on Linux. Without it, they simply don't load
properly.
I'm still running with the patch and as far as I'm able to judge it is good as
gold. Better.

Comment 45

[Joe Drew (not getting mail)]

I've mailed [e-mail], for both branch and trunk. (2 separate mails
because I wasn't sure about how it should be done.) We'll see what they say...

Comment 46

[Mike Shaver (:shaver emeritus)]

Comment on attachment 71557 [details] [diff] [review]
patch v.8

I was a little nervous about the refcount management split through diverse code
paths, and the NS_RELEASE_THIS that was involved, but I don't know the code
very well.  If Patrick is happy, I'm happy.

a=shaver for the 1.0 trunk.  (This did not make the 0.9.9 branch.)

Comment 47

[Joe Drew (not getting mail)]

Okay, looks like we're all done here. Can someone with CVS commit privileges
check this in?

Comment 48

[Peter Lubczynski]

Patch in trunk, marking FIXED.

Thanks all.

Comment 49

[[not reading bugmail]]

RKA, I was curious since you've done testing on this, does this patch fix or
appear to fix any bugs on the release notes page/plugins section for 0.9.9? 
Just trying to clear out a few bugs if possible.

Comment 50

[R.K.Aa.]

I would think release note bug 66015 was fixed with this checkin.
But someone with Windows NT and WMP 6.4 must verify that.
I added comment in the bug.

Comment 51

[Peter Lubczynski]

The crash in bug 66015 with WMP is due to some older versions of the plugin
dereferencing a null pointer after calling |GetJavaEnv|.

Comment 52

[R.K.Aa.]

ehe.. bug 66015 isn't a "crash" bug. It is about

"Windows media player works as a seperate application but does not launch when 
embedded in a webpage."... "Plugin : windows media player 6.4"

Comment (#41 ) says "i don't see anything where the video is supposed to be."

This was the exact case for me as well, with the same plugin: WMP 6.4, untill i
applied patch in attachment 71557 [details] [diff] [review].

Only one user in bug 66015 has seen a crash, and it seems to be a separate bug:
Bug 66015 comment 37: "Win2k pro here with wmedia 7.1". The user was running
tests with NS6.1 at http://www.broadcast.com, which after a while failed and
crashed.

bug 66015 comment 44: aruner asks whether anyone else has seen that crash, and
says it might be a separate bug. Noone has later come forward seeing the crash.
There is no stack.

Comment 53

[R.K.Aa.]

might add: i actually did test the URL in url-field of bug 66015
http://msnbc.com/m/lv/default.asp?0cv=c642 and it now works.

Comment 54

[[not reading bugmail]]

RKA, Shockwave Flash 6.0 is available for download now, I just grabbed it for
windows.

http://www.macromedia.com/shockwave/download/index.cgi?P1_Prod_Version=ShockwaveFlash

Comment 55

[R.K.Aa.]

This has regressed.

Build 2002031908 was AOK - build 2002032008 was broken.
Mediaplayer never initiates. Shockwave plugins aren't even left with the space
they used to get - there is no trace of them at all.

Comment 56

[R.K.Aa.]

Might add: Shockwave is as broken on WinXP now.

Comment 57

[shrirang khanzode]

reopening based on RkAa's comment.

Comment 58

[Peter Lubczynski]

Please open a NEW bug. There is no evidence this is the same problem and lets
not morph this bug.

Comment 59

[shrirang khanzode]

v

Comment 60

[R.K.Aa.]

filed bug 132867