91921 - pressing Back button while movie is playing crashes browser

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[shrirang khanzode]

mac only , 0723 branch, dunno if this is a regression , am yet to check on 0721 
build.

quicktime 5.0.2

steps:
go to the above link 
click on 'LARGE" (480x260 Broadband)
let the movie load and once it starts playing, click BACK button 

Observe that the browser crashes

stack :

Stack Signature   MixedMode + 0x1f4c (0x003fa18c) 9f2db325
Bug ID
Trigger Time 2001-07-23 09:59:14
User Comments quicktime 480x movie BACK button crash
Build ID 2001072303
Product ID Netscape6.10
Platform ID MacOS
Stack Trace
MixedMode + 0x1f4c (0x003fa18c)
ns4xPluginStreamListener::~ns4xPluginStreamListener() [ns4xPluginInstance.cpp, 
line 69]
ns4xPluginStreamListener::Release() [ns4xPluginInstance.cpp, line 144]
nsPluginStreamListenerPeer::~nsPluginStreamListenerPeer() [nsPluginHostImpl.cpp, 
line 1447]
nsPluginStreamListenerPeer::Release() [nsPluginHostImpl.cpp, line 1467]
nsCOMPtr_base::~nsCOMPtr_base() [nsCOMPtr.cpp, line 49]
nsStreamListenerTee::~nsStreamListenerTee() [nsStreamListenerTee.h, line 18]
nsStreamListenerTee::Release() [nsStreamListenerTee.cpp, line 6]
nsCOMPtr_base::assign_with_AddRef() [nsCOMPtr.cpp, line 58]
nsHttpChannel::OnStopRequest() [nsHttpChannel.cpp, line 2114]
nsOnStopRequestEvent::HandleEvent() [nsRequestObserverProxy.cpp, line 160]
nsARequestObserverEvent::HandlePLEvent() [nsRequestObserverProxy.cpp, line 63]
PL_HandleEvent() [plevent.c, line 590]
PL_ProcessPendingEvents() [plevent.c, line 520]
nsEventQueueImpl::ProcessPendingEvents() [nsEventQueue.cpp, line 374]
nsMacNSPREventQueueHandler::ProcessPLEventQueue() [nsToolkit.cpp, line 135]
nsMacNSPREventQueueHandler::RepeatAction() [nsToolkit.cpp, line 99]
Repeater::DoRepeaters() [nsRepeater.cpp, line 119]
nsMacMessagePump::DispatchEvent() [nsMacMessagePump.cpp, line 465]
nsMacMessagePump::DoMessagePump() [nsMacMessagePump.cpp, line 269]
nsAppShell::Run() [nsAppShell.cpp, line 110]
nsAppShellService::Run() [nsAppShellService.cpp, line 424]
Netscape 6 + 0x4c58 (0x175b2908)

Comment 1

[Peter Lubczynski]

I think this is a regression from bug 91140. Over to Andrei.

Comment 2

[Peter Lubczynski]

This WFM on W2K with today's build. Mac only?

Comment 3

[shrirang khanzode]

yeah, mac only !

Comment 4

[Peter Lubczynski]

Nope, this happens on W2K too, I wasn't testing correctly.

Note: You must have Quicktime 5.02 and use the joy_ride testcase. I have a hard
time reproducing with Quicktime 5.01 or lower and using other movie trailers.

Comment 5

[shrirang khanzode]

yawah, my win quicktime is 5.0.2...upgrading now...

Comment 6

[shrirang khanzode]

oops..i meant 5.0.1...

Comment 7

[Peter Lubczynski]

Okay, this sometimes happens and somtimes not. I watched Petersen play and
reload several other Quicktime 5.02 movies without crashing in today's build on Mac.

Comment 8

[av (gone)]

We call NPP_DestroyStream in ns4xPluginStreamListener destructor which happens 
after we supposedly destroy the plugin instance itself by NPP_Destroy. So looks 
like the better and more correct place for postponed 
ns4xPluginStreamListener::CleanUpStream is right before we call NPP_Destroy 
which happens in ns4xPluginInstance::Stop(). The problem is that 
ns4xPluginInstance object doesn't keep track of open streams. The simple linked 
list would be sufficient for this purporse.

Comment 9

[av (gone)]

Attachment: first try -- this patch is against branch code base

Comment 10

[av (gone)]

Peter, please make mStreams member variable public and modify the stream 
listener destructor to be

ns4xPluginStreamListener::~ns4xPluginStreamListener(void)
{
#ifdef NS_DEBUG
  printf("\nns4xPluginStreamListener::~ns4xPluginStreamListener\n\n");
#endif

  // remove itself from the instance stream list
  if(mInst) {
    nsInstanceStream * prev = nsnull;
    for(nsInstanceStream *is = mInst->mStreams; is != nsnull; is = is->mNext) {
      if(is->nsPluginInstanceStream == this) {
        if(prev == nsnull)
          mInst->mStreams = is->mNext;
        else
          prev->mNext = is->mNext;

        delete is;
        break;
      }
      prev = is;
    }
  }

  NS_IF_RELEASE(mInst);
}

I am building the tree so cannot to test it right away. The new patch reflecting 
these changed is coming if solves the problem.

Comment 11

[av (gone)]

Ah, I can build debug and release simulteniously in the same tree, cool. There 
is a typo in the above code, sorry:

-  if(is->nsPluginInstanceStream == this) {
+  if(is->mPluginStreamListener == this) {

Comment 12

[Peter Lubczynski]

Andrei,

Can you attach a new, complete patch? Thanks.

Also, I'm worred about the *listener = stream assignment in
ns4xPluginInstance::NewNotifyStream(). I think we need the code at the bottom of
the above ::NewStream():

	NS_ADDREF(stream);  // Stabilize
    
    nsresult res = stream->QueryInterface(kIPluginStreamListenerIID,
(void**)listener);

    // Destabilize and avoid leaks. 
    // Avoid calling delete <interface pointer>
	NS_RELEASE(stream);

The reason for this is that "new" doesn't ADDREF so we are returning an
interface pointer that hasn't been addrefed yet! This is probably causing other
problems. In the above code, the QI should addref when it returns the interface
pointer which cancels out with the NS_RELEASE after it leaving a refcount of one
because by the NS_ADDREF before the QI.

Comment 13

[av (gone)]

Attachment: second try -- added removing listener from the list if destroyed somewhere

Comment 14

[av (gone)]

In this iteration I also by Peter's suggestion changed code in NewNotifyStream 
making it exactly follow the lines of NewStream which is correct.

Comment 15

[av (gone)]

Attachment: third try -- limiting new destroy stream logic by seekable streams only

Comment 16

[Peter Lubczynski]

Much better!

Andrei, your thrid patch really looks more along the lines of "the correct way"
of fixing bug 91140.  Basically, the problem from bug 91140 was that
DestroyStream() was either being called too often or not enough. Now by
introducing the new struct nsInstanceStream we can keep track of a 4.x plugin
instance's open streams. Therefore, we are in a much better position of ensuring
DestroyStream() is called at the appropriate time. 

r=peterl

Comment 17

[av (gone)]

Attachment: fourth try -- a bit cleaner, jst's and pollmann's suggestions addressed

Comment 18

[Johnny Stenback (:jst)]

sr=jst

Comment 19

[selmer (gone)]

bonsai claims you checked in, updating whiteboard.  Thanks for the ultra-fast
turnaround on this!

Comment 20

[shrirang khanzode]

working on win and mac branch build 0724. no longer crashing...

Comment 21

[av (gone)]

It is in the trunk now.

Comment 22

[av (gone)]

Didn't I mark it fixes?

Comment 23

[shrirang khanzode]

vtrunk

Comment 24

[shrirang khanzode]

removing vtrunk, verified fixed on mac/ windows trunk 0801. No longer crashes...