12153 - nsIPluginInstance::Stop( ) and ::Destroy( ) not being called properly

Status: RESOLVED WORKSFORME

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

Description

[fourstar]

1.  When exiting the browser (apprunner.exe) via the Close button of the title
bar, the function nsIPluginInstance::Stop() is called, as expected.  However,
when exiting via the 'Quit' item of the File menu, Stop() is not called.

2.  The function nsIPluginInstance::Destroy( ) appears to never be called.

Note: I am e-mailing a zip of the source for a sample plugin illustrating these
problems. Also included in the zip is a small html doc used to invoke the
plugin. The zip is called 'NpFntSvr_Test081999'.

Comment 1

[drapeau]

This bug also causes a crash on exit if the Java Plug-In is enabled and
running.  Specifically, if you go to a page that has an applet (e.g.,
http://java.sun.com/) and then quit the browser, the browser will crash upon
exit.

If necessary, Stanley Ho (whom I've cc'd on this bug) can provide a few
supporting details on why this leaves the JVM open and causes the crash upon
exit.

I believe this should be fixed for Beta 1.  Sorry I didn't notice it before.

Comment 2

[av (gone)]

Should we add 'beta1' keyword for PDT team to evaluate it?

Comment 3

[stanley.ho]

Yes, av, please add the beta1 keyword. We need this fix for Java to work 
properly in Beta 1.

Comment 4

[Jim Roskind]

PDT+, but we can't hold for Java support changes after 3/7, and it will become 
PDT-

Comment 5

[stanley.ho]

I took a closer look of this bug. It appeared that when the browser window is 
closed, nsObjectFrame::Destroy() in layout/html/base/src is called to invoke 
nsIPluginInstance::Stop(). However, this call is too late, and the native plugin 
window is already destroyed, so it confused AWT in Java, and null pData 
exception is thrown. This is similar to the page-switch problem in #27486, but 
apparently, the fix for #27486 doesn't solve this bug.

In webshell/src/nsWebShell.cpp, nsWebShell::DestroyPluginHost() try to call 
nsIPluginInstance::Destroy() when mPLuginInitCnt is 0. However, it appears that 
mPluginInitCnt is 1 (on my machine) when the browser window is closed, so 
nsIPluginInstance::Destroy() is never called.

Hope these information will help fixing this bug.

Comment 6

[edburns]

Av, could you please look into this today.  Stanley tells me we have to have 
the fix in by tomorrow.

Ed

Comment 7

[bruenae.stuessi]

Testing some applets using Java Plug-in 1.3rc1 with M14, I have seen
that stop and destroy are never called.

The desired behaviour for (symmetricity) is that stop and detroy are called when 
html page containing the applet is exited, the same as init and start are 
correctly called when the html page is entered.

Closing the browser window is not the right event for calling stop and destroy 
of an applet but exiting/leaving the applet's html page, otherwise "reload", 
"back" and "forward" won't work.

This bug is a beta blocker for me.

Comment 8

[av (gone)]

OK. Looks like we are in trouble here. This is not plugin problem per se. After 
talking to Troy and Vidur the understanding is that we have deeper underlying 
problem in ownership relationship between pres shell and view manager. This 
particular case reveals wrong destruction order of the two, while in most other 
cases the order is right.

Is my understanding correct that the only situation when the Java plugin crashes 
on destruction is when we close the browser window via the title bar? And we do 
not crash if we, say, just leave the page. I do not have a fix in hand, the 
problem is bigger than previously thought, now we need to decide what to do with 
the bug beta status. Should we concentrate on finding workaround in the browser 
code or maybe some workaround could be easily done in the plugin itself? Or, 
alternatively, to releasenote it for beta1?

Troy, I am adding you to the cc list for now in case you have something to add.

Comment 9

[stanley.ho]

The crash will happen whenever you exit the browser. It is less serious if you 
switch the page to a non-applet page first before exiting because it will force 
the Applet.stop() to be called. However, it will still crash during exit.

I don't think there is any workaround on the OJI plug-in side (if it exists, I 
would have already implemented it) ;). I would suggest that we should try to fix 
it in the browser side, or find some workarounds in plug-in module. This crash 
is quite serious, and I don't think we want to release Beta 1 with it, even if 
it is documented in the Release note.

Comment 10

[Jim Roskind]

If I understand the problem: We will crash ONLY if we are displaying a Java
applet on a page when we shut down.
We are past the 3/7 target date, so this needs to be release noted.
PDT- for beta1
PDT+ for beta2

Comment 11

[bruenae.stuessi]

layout/html/base/src/nsObjectFrame.cpp

407 NS_IMETHODIMP
408 nsObjectFrame::Destroy(nsIPresContext* aPresContext)
409 {
410   // we need to finish with the plugin before native window is destroyed
411   // doing this in the destructor is too late.
412   if(mInstanceOwner != nsnull)
413   {
414     nsIPluginInstance *inst;
415     if(NS_OK == mInstanceOwner->GetInstance(inst))
416     {
417       inst->SetWindow(nsnull);
418       inst->Stop();
419     }
420   }
421   return nsObjectFrameSuper::Destroy(aPresContext);
422 }


417       inst->SetWindow(nsnull);
418       inst->Stop();

Two questions:
- Why is setWindow(null) called BEFORE stop and or destroy?
- Why is only stop called and not destroy of the plugin instance?
...       inst->Stop();
...       inst->Destroy();
...       inst->SetWindow(nsnull);

Comment 12

[edburns]

av, Do you have an answer to Bruenae's comment?

Comment 13

[av (gone)]

M... Was that an idea not to destroy the instance when just leaving the page?
To have it handy if we come back. I need to have a closer look. As to calling 
SetWindow after Stop and Destroy, I do not see why we should do that. SetWindow 
eventually call NPP_SetWindow, should it be destroyed by then?

Comment 14

[leger]

Putting on beta2 keyword radar since this was "plus for beta2" during beta1 
triaging

Comment 15

[stanley.ho]

SetWindow(nsnull) was called because we need to reparent the applet window to 
something else, before Stop() and Destroy() are called. Otherwise, if Stop() 
and Destroy() are called before SetWindow(nsnull) is called, the applet will 
not be able to stop and destroy the AWT window properly.

Comment 16

[av (gone)]

The originally reported problem seem to get fixed by recent code changes. I see 
both Stop and Destroy methods always called on nsIPluginInstance with sample 
plugin. Marking fixed. If anybody thinks other issues described here still need 
to be on the radar please open a separate bug.

Comment 17

[stanley.ho]

I was seeing the problem with M15 build. If you visit java.sun.com and let the 
applets running, then close the browser windows, you should see 
NullPointerException thrown from the Java Console because the Stop() and 
Destroy() are not called. AV, can you verify this with M16/M17?

Comment 18

[av (gone)]

I do not see the crash with PR2. I tried both File/Quit and the cross button in 
the right top corner.

Comment 19

[drapeau]

Sorry, Stanley, there seems to be no choice but for you to try it with a recent
daily build.  av: can you be more specific about exactly *what* changed in
recent builds to fix this problem?  Otherwise, it's hard for me to believe it's
an actual fix.

Comment 20

[av (gone)]

The code which Stanley mentioned in his 3.5.00 comments no longer exists. Looks 
like there were some profound changes in other area. I just built a dummy plugin 
which would dbg out every possible API call and see all the calls in place.

Comment 21

[Jan Carpenter]

Not fixed on Mac

I used the example of going to http://java.sun.com and exiting the browser.  It 
worked on Win32 and Linux, but on Mac I crashed.  The Talkback report follows.

reopening

2000-07-12-13-M17 : Mac

Verified
2000-07-12-11-M17 : Linux
2000-07-12-09-M17 : WinNT & Win98

TALKBACK REPORT FOR MAC:

Incident ID 14053548 
 Trigger Time 
            2000-07-12 17:33:02 
 Email Address 
            [e-mail] 
 User Comments 
            Go to the URL: java.sun.com, wait to load, click past alert boxes 
saying it can't find the plugin, exit browser, crash 
 Build ID
            2000071213 
 Product ID
            Netscape6 
 Platform ID
            MacOS 
 Stack Trace

nsWalletlibService::OnEndDocumentLoad() [nsWalletService.cpp, line 327] 
nsDocLoaderImpl::FireOnEndDocumentLoad() [nsDocLoader.cpp, line 808] 
nsDocLoaderImpl::FireOnEndDocumentLoad() [nsDocLoader.cpp, line 815] 
nsDocLoaderImpl::FireOnEndDocumentLoad() [nsDocLoader.cpp, line 815] 
nsDocLoaderImpl::DocLoaderIsEmpty() [nsDocLoader.cpp, line 613] 
nsDocLoaderImpl::OnStopRequest() [nsDocLoader.cpp, line 542] 
nsLoadGroup::RemoveChannel() [nsLoadGroup.cpp, line 544] 
nsLoadGroup::Cancel() [nsLoadGroup.cpp, line 222] 
nsDocLoaderImpl::Stop() [nsDocLoader.cpp, line 274] 
nsURILoader::Stop() [nsURILoader.cpp, line 619] 
nsDocShell::StopLoad() [nsDocShell.cpp, line 325] 
nsDocShell::InternalLoad() [nsDocShell.cpp, line 2590] 
nsDocShell::LoadHistoryEntry() [nsDocShell.cpp, line 3434] 
nsDocShell::LoadURI() [nsDocShell.cpp, line 256] 
nsSHistory::LoadEntry() [nsSHistory.cpp, line 470] 
nsSHistory::GotoIndex() [nsSHistory.cpp, line 427] 
nsSHistory::GoBack() [nsSHistory.cpp, line 344] 
nsBrowserInstance::Back() [nsBrowserInstance.cpp, line 457] 
CODE.10 
XPTC_InvokeByIndex() [xptcinvoke_mac.cpp, line 129] 
nsXPCWrappedNativeClass::CallWrappedMethod() [xpcwrappednativeclass.cpp, line 
908] 

Comment 22

[Jan Carpenter]

Also, as I discovered on trying to restart Mozilla on NT, after running the 
previous test (go to http://java.sun.com and exit browser) my browser will not 
restart without rebooting my computer.  This is on NT only, I didn't have the 
problem on Win98, Linux, or Mac.  

I get past the splash screen, & the profile selection, then I get the message:
The instruction at "0x6028da45" referenced memory at "0x00000000". The memory 
could not be "read".

Comment 23

[Jay Patel [:jay]]

Putting on [NEED INFO] radar. PDT needs to know impact to user and risk of fix 
to make a call on this bug. 

Comment 24

[Jay Patel [:jay]]

Putting on [nsbeta2-] radar. Not critical to beta2. 

Comment 25

[edburns]

In a 4 Sept 00 Mozilla build with a 2 Sept 00 Java Plugin build, I find that 
Stop is called on exit.  Destroy is currently not called on page switch, and 
that is the essence of bug 50547.  I'm closing this and making it depend on 
50547.