75293 - Plugin continues to play even after leaving page

Status: VERIFIED FIXED

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

Description

[shrirang khanzode]

Seen on mac trunk 0405 , using Beatnik plugin 2.2
Do not have quicktime installed.

Steps:

Get the latest beatnik player plugin from beatnik.com (2.2)
Go to above url
url plays in beatnik player
After a while, leave the page and surf to another page
Notice that the music continues in the background..plugin does not stop

Comment 1

[Peter Lubczynski]

Sean says this is a case of Stop() not being called for full-page plugins. 

Comment 2

[sean echevarria]

I'm updating my mac build.  Will let you know when I know.

Comment 3

[sean echevarria]

I've confirmed that nsIPluginInstance::Stop() is not getting called.

Comment 4

[sean echevarria]

I think the problem is that PluginViewerImpl::Destroy() in nsPluginViewer.cpp 
is not implemented.  It should probably do something similar to 
nsObjectFrame::Destroy() in nsObjectFrame.cpp.

Comment 5

[sean echevarria]

Here's Beatnik specific url testcase (Beatnik is the only plugin that handles 
rmf content):

http://www.beatnik.com/software/documentation/music_object34/music/trance.rmf

This will allow you to test with the Beatnik plugin even if Quicktime is 
installed.

Comment 6

[Peter Lubczynski]

Upping priority. I'll see if I can fix this sooner.

Thanks Sean.

Comment 7

[Peter Lubczynski]

Attachment: possible patch to fix

Comment 8

[sean echevarria]

The patch fixes this particular problem.  I'm getting some pretty wierd mouse 
behavior though - when I get some time I'll file bugs.

Comment 9

[Peter Lubczynski]

Andrei,

Can I get a review?

Comment 10

[Marc Attinasi]

OK, I guess you coppied this from ObjectFrame, but You can probably change this:

+        if (doCallSetWindowAfterDestroy) {
+          inst->Stop();
+          inst->Destroy();
+          inst->SetWindow(nsnull);
+        }
+        else {
+          inst->SetWindow(nsnull);
+          inst->Stop();
+          inst->Destroy();
+        }

to this:

          !doCallSetWindowAfterDestroy ? inst->SetWindow(nsnull) : 0;
          inst->Stop();
          inst->Destroy();
          doCallSetWindowAfterDestroy ? inst->SetWindow(nsnull) : 0;

not very important though.

As we spoke about earlier, having this code duplicate in two places is not good
- can it be factored? Should you open a bug on that. Lastly, is the wierd mouse
behavior a result of the patch or does it happen with or without the patch?
Assuming the latter, and assuming that you get appropriate review, sr=attinasi

Comment 11

[sean echevarria]

Opened bug 75582 for the mouse problem I commented on.

Comment 12

[Peter Lubczynski]

Bug 75582 is not related to this and I think I know how to fix it.

Bug 74457 was opened on refactoring code from nsObjectFrame.cpp and 
nsPluginViewer.cpp.

Comment 13

[av (gone)]

I like this language thing! Also 'traditional' if statement may look easier to 
read to some.

I agree with Marc on factorizing the code but this is definitely not important 
now. May be we could somehow to write a common base class for both full-page and 
embedded plugins to inherit from. Task for the future, if any.

Another thing. Once you are touching the file. May be I am missing something, 
but is there any sense to have class declaration in the .cpp file?

With or without all this -- ra=av

Comment 14

[Peter Lubczynski]

Patch checked in, marking FIXED.

/cvsroot/mozilla/modules/plugin/nglsrc/nsPluginViewer.cpp,v  <--  
nsPluginViewer.cpp
new revision: 1.52; previous revision: 1.51

Comment 15

[Peter Lubczynski]

I've always liked the "quickie" conditional statement, especially in this case 
as, I think, it reads well with the variable name. I've always found braces more 
confusing unless your IDE "helps" you as CodeWarrior.

Comment 16

[shrirang khanzode]

verified on mac.(0416)