80105 - navigator.plugins.refresh(1) crashes on pages with xpcom plugin - M091 topcrash [@ nsPluginInstanceOwner::GetInstance]

Status: VERIFIED FIXED

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

Description

[greer]

This one is showing up in the Linux M09 build (2001050822). Looking at the 
comments it may be related to bug 76936 
(nsPluginInstanceOwner::~nsPluginInstanceOwner)

(30155720) Comments: quitting netscape after a longish sessionnote that
this page has a java applet
(30196733) Comments: I closed netscape done through "file/quit"
(30185093) Comments: I quit Navigator
(30170956) Comments: I just wanted to exit the browser.
(30157054) Comments: I just quit the browser with Ctrl-Q...
(30144960) Comments: closing mozilla
(30142454) Comments: Picked the "Close"-menuitem from the adressbook
windows "file" menu.



nsPluginInstanceOwner::GetInstance()
         nsObjectFrame::Destroy()
         nsLineBox::DeleteLineList()
         nsBlockFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsTableFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsTableOuterFrame::Destroy()
         nsLineBox::DeleteLineList()
         nsBlockFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsTableFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsTableOuterFrame::Destroy()
         nsLineBox::DeleteLineList()
         nsBlockFrame::Destroy()
         nsLineBox::DeleteLineList()
         nsBlockFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsBoxFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         nsBoxFrame::Destroy()
         nsGfxScrollFrame::Destroy()
         nsFrameList::DestroyFrames()
         nsContainerFrame::Destroy()
         ViewportFrame::Destroy()
         FrameManager::Destroy()
         PresShell::~PresShell()
         PresShell::Release()
         nsCOMPtr_base::~nsCOMPtr_base()
         DocumentViewerImpl::~DocumentViewerImpl()
         DocumentViewerImpl::Release()
         nsCOMPtr_base::assign_with_AddRef()
         nsDocShell::Destroy()
         nsWebShell::Destroy()
         nsDocShell::DestroyChildren()
         nsDocShell::Destroy()
         nsWebShell::Destroy()
         nsXULWindow::Destroy()
         nsWebShellWindow::Destroy()
         nsChromeTreeOwner::Destroy()
         GlobalWindowImpl::Close()
         nsAppShellService::Quit()
         XPTC_InvokeByIndex()
         nsXPCWrappedNativeClass::CallWrappedMethod()
         WrappedNative_CallMethod()
         js_Invoke()
         js_Interpret()
         js_Invoke()
         js_InternalInvoke()
         JS_CallFunctionValue()
         nsJSContext::CallEventHandler()
         nsJSEventListener::HandleEvent()
         nsEventListenerManager::HandleEventSubType()
         nsEventListenerManager::HandleEvent()
         nsXULElement::HandleDOMEvent()
         PresShell::HandleDOMEventWithTarget()
         nsMenuFrame::Execute()
         nsMenuFrame::HandleEvent()
         PresShell::HandleEventInternal()
         PresShell::HandleEvent()
         nsView::HandleEvent()
         nsView::HandleEvent()
         nsView::HandleEvent()
         nsView::HandleEvent()
         nsViewManager::DispatchEvent()
         HandleEvent()
         nsWidget::DispatchEvent()
         nsWidget::DispatchWindowEvent()
         nsWidget::DispatchMouseEvent()
         nsWidget::OnButtonReleaseSignal()
         nsWindow::HandleGDKEvent()
         dispatch_superwin_event()
         handle_gdk_event()
         libgdk-1.2.so.0 + 0x17507 (0x406cc507)
         libglib-1.2.so.0 + 0x10309 (0x406fc309)
         libglib-1.2.so.0 + 0x10913 (0x406fc913)
         libglib-1.2.so.0 + 0x10aac (0x406fcaac)
         libgtk-1.2.so.0 + 0x93c57 (0x40610c57)
         nsAppShell::Run()

Comment 1

[greer]

Adding keywords crash, topcrash, qawanted for tracking

Comment 2

[edburns]

Peter, can you please look at this?  I think this is related to bug 73196.

Comment 3

[greer]

edburns, would you please verify that you meant bug 73196 in your previous 
comment. That is a Windows bug related to GIF corruption.

Comment 4

[shrirang khanzode]

bug 76936 ?

Comment 5

[Peter Lubczynski]

Eeek...where did my last comments go?

Anyway, this is a problem that we try to do an Addref when the vtable looks 
toasted. Another strong ref problem? See bug 79872 for more info. 

Comment 6

[greer]

Here are some extended comments and a few URLs (from this morning's reports):

     (30442798) URL: http://cs.nyu.edu/visual/home/demos/tigerDemo
     (30442798) Comments: I exited the program (the Quit option from the File 
menu) after lookingat a couple of pages with java applets.  The first applet I 
looked at was http://cs.nyu.edu/been/demo/latest.applet.html.  I also had the 
java console open.  I hadn't noticed that there was any problem.

     (30423591) URL: http://www.benchmarkiv.com
     (30423591) Comments: I had a chance to check out the auto resizingof 
animated gifs a little further. Any time that an html height and width 
specification differs from the dimentions inherent in the gif mozilla 9 crops 
the gif to some unspecifiied size.  This happens whether or not the gif is part 
of a javascript.. Thus making this a very serious problem. Also why don't you 
specify in your release notes that only IBMJava2-13 works with
your browser on Redhat 7.1 instead of providing automatic plugin support to 
netscape. Konquer Java works fine with undeprecated glibc libraries. So I'm 
wondering if the problem isJava or your browser. Gene
     
     (30397961) URL: http://www.benchmarkiv.com
     (30397961) Comments: there seems to be a problem with the auto resizing of 
animateid gif's when they are embedded in a javascript. I fixed the problem on 
this site bymaking the gif exactly the right size. IBMJava2-13seems to be 
working fine.

     (30269103) URL: http://www.benchmarkiv.com
     (30269103) Comments: I finally got java to work using the IBM Java2-13. and 
the redhat 7.1jvm workaround. Your java does not work even with their work 
around which makes the kernel use old 2.2 glibc. IBMJava works but someanimated 
gifs only dispay half their area and I get this screen every timeI quit Mozilla. 

     (30284572) Comments: I had actually just closed the browser because it was 
eating up an inordinate amount of CPU cycles and didn't seem to want to stop.   
The only thing out of the ordinary on the page I was viewing was a Java Applet I 
believe.  I'm using the java plugin that came with JDK 1.3.0_01

     (30196733) URL: http://www.dft.nl/ and http://www.detelefoongids.nl/ 
     (30341616) URL: /usr/java/jdk1.3.0_02/demo/jfc/Java2D/Java2Demo.html
     (30185093) URL: http://www.student.carleton.edu/m/meadowa
     (30155720) URL: http://www.rijkswacht.be/rw/verkeer/module.htm

Comment 7

[Peter Lubczynski]

nsPluginInstanceOwner::GetInstance() addrefs, that's probably the problem

Comment 8

[chris hofmann]

good if we could get this fixed for 0.9.1... is that possible?

Comment 9

[Peter Lubczynski]

This bug needs to be re-evaluated with Talkback reports. I think recent
check-ins have fixed this and I can not reproduce but I'm unwilling to close out 
a topcrash bug without any evidence. 

Can someone see if they can kick cyclone into spitting out a report on this 
starting with Friday evening builds?

Comment 10

[chris hofmann]

looks like the last time saw incoming reports for 'plugininstanceowner' crashes
was on the 18th....  here are the counts of such crashes

   6 2001051813
   3 2001051812
  21 2001051710
   9 2001051707

would the post May 18 lack of crashes match up with checkin fixes?

Comment 11

[Peter Lubczynski]

Yup, thanks! Marking FIXED.

Comment 12

[shrirang khanzode]

my stack traces do not show this anymore. Also, based on chofmann's comment, 
marking VERIF.

Comment 13

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

Reopening.  Today's
http://ftp.mozilla.org/pub/data/crash-data/seamonkey-crash-analysis-detailed.txt
contains:

nsPluginInstanceOwner::GetInstance   5
			 80105 	 VERI 	 FIXE 	 [e-mail] 	 mozilla0.9.1
     First BBID : http://climate/reports/stackcommentemail.cfm?dynamicBBID=31148923
     Last BBID  : http://climate/reports/stackcommentemail.cfm?dynamicBBID=31201391
     Min Runtime :270
     Max Runtime :124585
     Min seconds since last crash :270
     Max seconds since last crash :82576
     First Appearance Date : 2001-05-31
     Last Appearance Date : 2001-06-01
     First Build ID : 2001053006
     Latest Build ID : 2001060108

Stack Trace: 

    nsPluginInstanceOwner::GetInstance adf24497
	http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/html/base/src/nsObjectFrame.cpp line 1874
	Build: 2001053006 CrashDate: 2001-06-01 UptimeMinutes: 675  Total: 2076 
	OS: Windows NT  4.0 build 1381
	 Detailed : http://climate/reports/incidenttemplate.cfm?bbid=31201391
	 StackTrace: http://climate/reports/stackcommentemail.cfm?dynamicBBID=31201391
     (31201391)	URL: http://www.mlb.com
     (31201391)	Comments: shutting down

    nsPluginInstanceOwner::GetInstance() c4e106db
	 line 
	Build: 2001060108 CrashDate: 2001-06-01 UptimeMinutes: 4  Total: 4 
	OS: Linux 2.4.3
	 Detailed : http://climate/reports/incidenttemplate.cfm?bbid=31195523
	 StackTrace: http://climate/reports/stackcommentemail.cfm?dynamicBBID=31195523

    nsPluginInstanceOwner::GetInstance() 719917b7
	 line 
	Build: 2001053008 CrashDate: 2001-05-31 UptimeMinutes: 71  Total: 71 
	OS: Linux 2.2.16-3
	 Detailed : http://climate/reports/incidenttemplate.cfm?bbid=31172555
	 StackTrace: http://climate/reports/stackcommentemail.cfm?dynamicBBID=31172555
     (31172555)	URL: java.sun.com
     (31172555)	Comments: selected file->exit from drop dowm menu

    nsPluginInstanceOwner::GetInstance() 0d252591
	 line 
	Build: 2001053007 CrashDate: 2001-05-31 UptimeMinutes: 129  Total: 129 
	OS: Linux 2.2.12-20smp
	 Detailed : http://climate/reports/incidenttemplate.cfm?bbid=31157638
	 StackTrace: http://climate/reports/stackcommentemail.cfm?dynamicBBID=31157638
     (31157638)	Comments: Exiting the application

    nsPluginInstanceOwner::GetInstance 16686d73
	http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/html/base/src/nsObjectFrame.cpp line 1874
	Build: 2001053009 CrashDate: 2001-05-31 UptimeMinutes: 1376  Total: 1376 
	OS: Windows NT  4.0 build 1381
	 Detailed : http://climate/reports/incidenttemplate.cfm?bbid=31148923
	 StackTrace: http://climate/reports/stackcommentemail.cfm?dynamicBBID=31148923
     (31148923)	Comments: real crash

Comment 14

[Peter Lubczynski]

Looking closer at the crash data, looks like it happens on NT too, and on 
exiting the app. From that info, I think this is a different bug than I fixed 
last time, but I guess we can use this bug as the summary fits.

I've seen this before but I thought it had got fixed or I got it mixed up with 
the "service manager held past XPCOM shutdown/java bug". The problem as I 
remember it was that we were trying to addref a stale pointer when GetInstance 
was called. In other words, the Instance went away too soon.

I *really* need some testcases to reproduce the crash. It would be good to see 
how bad this is as exiting the app while in a simple Java 
(http://www.javasoft.com) or Real testcase seem to work fine for me on Win32. 
Also shuting down while at http://www.mlb.com worked for me as well. I wonder if 
it's something to do with scripting and the instance's refcount as well as 
platform??

If I recall the questionable code correctly, we need to either keep a better 
track of the plugin instance lifetime or (I just started reading about this) use 
something like an nsIWeakRef which will I think should NULL out the dangling 
pointer.

Comment 15

[Peter Lubczynski]

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

Comment 16

[selmer (gone)]

Is there any "avoid the crash" hack that can go in for m.9.1?  We're almost
completely out of time...

Comment 17

[shrirang khanzode]

the steps in bug 83499 is a good testcase to reproduce this crash. I crash 100% 
of the time.

Comment 18

[Peter Lubczynski]

Andrei, could you take a look at this if you get the chance?
http://dailynews.yahoo.com/h/a/g/bs/?u

It seems to be doing something similar to the Shockwave registration bug in that 
they do ReloadPlugins() but this time with both XPCOM Java and Real. The 
instance is probably garbage because Shutdown() was called Java and Real when 
doing ReloadPlugins(). It probably has to do with TryUnloadPlugin()

Comment 19

[Marek Z. Jeziorek]

is av on vacation still?

Comment 20

[selmer (gone)]

Phil, you were going to reassign this or something.

Comment 21

[Phil Peterson]

av is back

Comment 22

[av (gone)]

Is my understanding correct that this bug is about XPCOM plugins?

Peter, I am getting a stack trace which may look interesting for you:
EMBD3260! 61508633()
EMBD3260! 6150754a()
NPPL3260! 615549e3()
NPPL3260! 6155dbcd()
nsPluginStreamListenerPeer::SetUpStreamListener(nsIRequest * 0x0645f190, nsIURI 
* 0x063de710) line 1929 + 25 bytes
nsPluginStreamListenerPeer::OnStartRequest(nsPluginStreamListenerPeer * const 
0x06487760, nsIRequest * 0x0645f190, nsISupports * 0x00000000) line 1596 + 21 
bytes
nsHttpChannel::OnStartRequest(nsHttpChannel * const 0x0645f194, nsIRequest * 
0x06409464, nsISupports * 0x00000000) line 2023
nsOnStartRequestEvent::HandleEvent() line 108 + 53 bytes
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x0641d6f4) line 64
PL_HandleEvent(PLEvent * 0x0641d6f4) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00d508d0) line 520 + 9 bytes

Comment 23

[av (gone)]

After removing yahoo cookies (to initiate this checking process) I cannot even 
get the place of supposed crash as I crash earlier. The stack trace follws.

Comment 24

[av (gone)]

Attachment: stack trace for my previous comment

Comment 25

[Peter Lubczynski]

Attachment: complete stack of ::GetInstance()

Comment 26

[Peter Lubczynski]

Okay, I can now reporduce this easily with the test url. As you can see from the
*complete* stack, the problem happens when there is a
javascript://navigator.plugins.refresh(true); The (true) part is causing the
page to be torn down, we've changed the state of the plugin such that the
instance pointer (mInstance probably went away) is now pointing to garbage,
hence the crash way farther down the road in page destruction, in
::GetInstance() when we try to get the instance to stop it. What is strange is
that the Real player appears and then the crash happens. The console also spits out:
Inside ns4xPluginInstance::SetWindow(00000000)...
ns4xPluginInstance::Stop()

Andrei and Nick: How were XPCOM Plugins designed to deal with a Refresh()?

Comment 27

[av (gone)]

There is no actual difference. It all goes through the same code path 
(nsPluginHostImpl::LoadPlugins) as during start up.

BTW, did you notice that there are three object frames being created: one for 
object tag, one for embed tag and one for applet tag?

Comment 28

[Peter Lubczynski]

Ahh...an easier way to reproduce, with a different code path but same crash is
visit ANY Real site, and while the plugin is playing, issue a
javascript.plugins.refresh(1) in the Javascript console.

Comment 29

[Peter Lubczynski]

Crashes even with simple testcases on Rrefresh();

http://panther.mcom.com/testcases/plugins/testcases/real1.html

Comment 30

[Peter Lubczynski]

Attachment: simplified testcase

Comment 31

[Peter Lubczynski]

This patch seems to sort of help on simple testcases. The big Yahoo one still fails.

Index: nsPluginHostImpl.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/plugin/nglsrc/nsPluginHostImpl.cpp,v
retrieving revision 1.257.4.1
diff -u -5 -r1.257.4.1 nsPluginHostImpl.cpp
--- nsPluginHostImpl.cpp        2001/06/06 16:37:16     1.257.4.1
+++ nsPluginHostImpl.cpp        2001/06/07 02:21:49
@@ -2140,11 +2140,11 @@
     // if we have currently running plugins we should set a flag not to
     // unload them from memory, see bug #61388
     // and form a list of libs to be unloaded later
     for(nsPluginTag * p = mPlugins; p != nsnull; p = p->mNext)
     {
-      if(IsRunningPlugin(p) && (p->mFlags & NS_PLUGIN_FLAG_OLDSCHOOL))
+      if(IsRunningPlugin(p))
       {
         p->mCanUnloadLibrary = PR_FALSE;
         AddToUnusedLibraryList(p->mLibrary);
       }
     }

Comment 32

[Peter Lubczynski]

Something that Andrei just metioned and I confirm is that during a reload of any
kind, the new instance of a plugin is created before the old one is destroyed.
In the case of ReloadPlugins(), we probably want the old document to go away
completely so that the instances will be destroyed then thereby preventing this
crash and others like them. Didn't Hyatt make a change like that?

One idea is in nsPluginArray to save the url, navigate to about:blank, reload
the plugins, and then re-navigate back to old url. It won't be pretty but it's
not like it happens often. But alas, I'm tired, something to try tomorrow. 

Comment 33

[av (gone)]

I have tried the following modification in nsPluginArrayImpl::Refresh and it did 
not help.

Comment 34

[av (gone)]

Attachment: prototype to test an idea of going to some other page first

Comment 35

[Peter Lubczynski]

Attachment: hacked patch to navigate to about:blank first

Comment 36

[Peter Lubczynski]

The above hacked patch seem to work for the simplified testcase. However, the
big Yahoo one doens't crash anymore, but goes in an loop of reloading the
current document forever. 

Notice I'm telling the plugin host to do a normal reload instead of a refresh.

Comment 37

[av (gone)]

If you do pm->ReloadPlugins(PR_FALSE) everything else is probably irrelevant 
because the plugin remains in memory. But this approach will crash the thing if 
the dll is updated. (Will Windows allow us to replace the dll if the plugin is 
playing?)

Comment 38

[av (gone)]

If you do pm->ReloadPlugins(PR_FALSE) everything else is probably irrelevant 
because the plugin remains in memory. But this approach will crash the thing if 
the dll is updated. (Will Windows allow us to replace the dll if the plugin is 
playing?)

Comment 39

[av (gone)]

urgh. Sorry for duplicate comment. Is there a way to edit bug report?

Comment 40

[av (gone)]

I think I figured out a way to set up a web progress listener correctly and now 
can wait for the blank page to load completely and then to refresh the plugin 
list. (It fixes simple case.) Everything goes more or less smooth on the yahoo 
site and I can even see the player after refresh but the it comes to refresh 
again and crashes. I tried to turn off our plugin refresh stuff completely (just 
put return statement in the very beginnig of nsPluginArrayImpl::Refresh(...)) 
and it still crashes! I also crash with 4.x. Starting to get an impression that 
we are fighting ghosts here. Peter, have you ever seen this thing working on any 
browser?

Comment 41

[Peter Lubczynski]

You may be right!!! However, all the videos at Yahoo.com work very nicely and
don't crash! We should at least not crash.

Okay, we should fix the simple testcase so it doesn't crash. My changes (which
are similar to yours) prevent the crash in both the simple testcase and the
yahoo one. 

Question: instead of using nsIWebNavigation, could you directly use
nsIDocShell->LoadURI with setting SetStopActiveDocument to PR_FALSE? Anyone more
familiar with WebNav/DocShell code?

However, the Yahoo testcase, is eitehr INVALID or possibly a dup of a Java bug.
Andrei, check out the source, they are using document.write to try to
create an instance of this Java class. Opening the Java console (be sure you
have java installed) reveals an exception and probably the reason my patch ended
up in a loop:

java.lang.NoClassDefFoundError: RMObserver

	at java.lang.ClassLoader.defineClass0(Native Method)

	at java.lang.ClassLoader.defineClass(Unknown Source)

	at java.security.SecureClassLoader.defineClass(Unknown Source)

	at sun.applet.AppletClassLoader.findClass(Unknown Source)

	at sun.plugin.security.PluginClassLoader.findClass(Unknown Source)

	at java.lang.ClassLoader.loadClass(Unknown Source)

	at sun.applet.AppletClassLoader.loadClass(Unknown Source)

	at java.lang.ClassLoader.loadClass(Unknown Source)

	at sun.applet.AppletClassLoader.loadCode(Unknown Source)

	at sun.applet.AppletPanel.createApplet(Unknown Source)

	at sun.plugin.AppletViewer.createApplet(Unknown Source)

	at sun.applet.AppletPanel.runLoader(Unknown Source)

	at sun.applet.AppletPanel.run(Unknown Source)

        at java.lang.Thread.run(Unknown Source)

Comment 42

[Peter Lubczynski]

To clarify, I do not crash in 4.x, it works nicely.

Comment 43

[David Hyatt]

Attachment: Patch that forces synchronous destruction of doc if plugins array is accessed

Comment 44

[av (gone)]

No, doesn't seem to help with simplified test case.

Just to confirm, hitting Reload button works fine, as well as similar test case 
with QT. So the problem is only with xpcom pluigns on 
javascript:navigator.plugins.refresh(1) command.

Comment 45

[David Hyatt]

Can you send me a link or pointer to the QT test case?

Thanks!

Comment 46

[av (gone)]

Attachment: qt-test.zip file. Just unzip it somewhere and load qt-test.html

Comment 47

[av (gone)]

And please also note that QT test worked before applying the latest patch too. 
As well as simple Reload.

Comment 48

[David Hyatt]

Ok, I'm trying something else.  Will post a new patch momentarily.

Comment 49

[David Hyatt]

I cannot stop this crash.  I've written a lot of code now, and I'm fairly
certain this is not my bug.

I can tell you this for sure:
  (1) The crash is happening in the new document after painting has been
unsuppressed.
  (2) Even if I synchronously destroy the old document before the reload from
the refresh even happens, the new document crashes in the same place.
  (3) If i add back the original code that nulls out the content viewer
immediately, i.e., completely remove the hack to keep the old page alive, I
*still* crash in the same place.

This bug does not appear IMO to have anything to do with my patch.  I would
theorize that a leak of the old document's presshell is perhaps keeping the old
frame tree alive.

I'm going to back away from this bug now, although I might incidentally fix this
bug when I tackle 80203.  My fix for 80203 will force a presshell (even if
leaked) to destroy its frame tree.  That is not currently targeted at RTM,
however, although, if it fixes this bug, perhaps it should be.

Comment 50

[Peter Lubczynski]

Hyatt,

I've tried your patch on the simple testcase:  
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=37463

and things seem to be a bit better. I don't crash. And it appears to function
corectly, even playing. However, loading up the big testcase at Yahoo! News
comes up blank with a bunch of style errors in the console. On a Reload, more
frame "aCombinedWidth" ASSERTIONS until a final crash while trying to set up a
new stream with a dangling instance pointer.

However, one thing I did notice was that frame destruction/creation seemed not
to be effected. Put printf's in the destructor and constructor of
nsPluginInstanceOwner in nsObjectFrame.cpp. This has an ownening reference to
the instance and notice that still a new one is creted before the old one is
destroyed.

Comment 51

[av (gone)]

With recent build I cannot get to the point it crashes on the Yahoo site -- 
blank window appears doing nothing. Simlified test case still crashes. Changing 
summary to make it more informative. Setting eta.

Just not to loose, the original Yahoo URL was 
http://dailynews.yahoo.com/h/a/g/bs/?u

Comment 52

[Peter Lubczynski]

Andrei, this patch may help as it prevents the simple testcase from crashing:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=37658

Weird, though, about:plugins goes blank!

Comment 53

[av (gone)]

I also have a patch to avoid a crash in the simplified case. Will investigate 
tomorrow which one is better. Or will come up with yet another one.

Comment 54

[Jan Carpenter]

M091 topcrash. Adding  [@ nsPluginInstanceOwner::GetInstance] for tracking

Here are comments/urls from recent crashes:

   (31750805)
Comments: I selected File/Close while I had 6 open browser
windows.
     (31738479) URL: www.eet.com
     (31738479) Comments: running from an exceed session. exceed
also seem to be having problems
     (31737630) Comments: Exiting Mozilla after leaving it on for
more than 3 days browsing a lot of pages in the meantime.
     (31725369) URL: sportsoul.tiscalinet.it
     (31720639) Comments: closed it
     (31718843) Comments: loading yahoo realvideo site
     (31714158) Comments: crashed when loading Font2DTest.html
from Java 1.3.1 distribution.
     (31700433) Comments: closing (with ^q) - it had closed 5 or 6
of the 6 windows i had open before it crashed
     (31694583) Comments: quitting
     (31690418) Comments: Closing the addressbook window.  I had
navigator and mail open but mail was unable to reach my POP server
due to firewall.
     (31689866) Comments: File->Quit
     (31685992) Comments: selected "File/Quit" to quit browser
suite after listening to Netscape Radio radio.netscape.com
     (31682916) Comments: Actually
     (31677080) Comments: Quality Feedback Agent appears when
closing mozilla via File->Quit
     (31676345) URL: http://www.broadcast.com
     (31676345) Comments: New Broadcast tests passed
     (31658032) URL: http://www.saferinternet.org/
     (31658032) Comments: closing mozilla from this page
     (31624122) Comments: Closed Mozilla with Ctrl-Q. Had Java
Applet running. Copied Realplayer plugin files to mozilla/plugins
directory while mozilla was running.Maybe I shouldn't have done
that?
     (31621788) Comments: quitting...


Here is a stack trace from the 2001060713 build

Incident ID 31752015 
nsPluginInstanceOwner::GetInstance() 
nsObjectFrame::Destroy() 
nsLineBox::DeleteLineList() 
nsBlockFrame::Destroy() 
nsLineBox::DeleteLineList() 
nsBlockFrame::Destroy() 
nsFrameList::DestroyFrames() 
nsContainerFrame::Destroy() 
nsFrameList::DestroyFrames() 
nsContainerFrame::Destroy() 
nsBoxFrame::Destroy() 
nsFrameList::DestroyFrames() 
nsContainerFrame::Destroy() 
nsBoxFrame::Destroy() 
nsGfxScrollFrame::Destroy() 
nsFrameList::DestroyFrames() 
nsContainerFrame::Destroy() 
ViewportFrame::Destroy() 
FrameManager::Destroy() 
PresShell::~PresShell() 
PresShell::Release() 
nsCOMPtr_base::~nsCOMPtr_base() 
DocumentViewerImpl::~DocumentViewerImpl() 
DocumentViewerImpl::Release() 
nsCOMPtr_base::assign_with_AddRef() 
nsDocShell::Destroy() 
nsWebShell::Destroy() 
nsHTMLFrameInnerFrame::~nsHTMLFrameInnerFrame() 
nsFrame::Destroy() 
nsFrameList::DestroyFrames() 
nsContainerFrame::Destroy() 
nsFrameList::DestroyFrames() 
nsContainerFrame::Destroy() 
nsBoxFrame::Destroy() 
nsFrameList::DestroyFrames() 
nsContainerFrame::Destroy() 
nsBoxFrame::Destroy() 
nsFrameList::DestroyFrames() 
nsContainerFrame::Destroy() 
nsBoxFrame::Destroy() 
nsFrameList::DestroyFrames() 
nsContainerFrame::Destroy() 
nsBoxFrame::Destroy() 
nsFrameList::DestroyFrames() 
nsContainerFrame::Destroy() 
nsBoxFrame::Destroy() 
nsFrameList::DestroyFrames() 
nsContainerFrame::Destroy() 
ViewportFrame::Destroy() 
FrameManager::Destroy() 
PresShell::~PresShell() 
PresShell::Release() 
nsCOMPtr_base::~nsCOMPtr_base() 
nsView::HandleEvent() 
nsViewManager::DispatchEvent() 
HandleEvent() 
nsWidget::DispatchEvent() 
nsWidget::DispatchWindowEvent() 
nsWidget::DispatchMouseEvent() 
nsWidget::OnButtonReleaseSignal() 
nsWindow::HandleGDKEvent() 
dispatch_superwin_event() 
handle_gdk_event() 
libgdk-1.2.so.0 + 0x1753b (0x4031653b) 
libglib-1.2.so.0 + 0x10186 (0x40346186) 
libglib-1.2.so.0 + 0x10751 (0x40346751) 
libglib-1.2.so.0 + 0x108f1 (0x403468f1) 
libgtk-1.2.so.0 + 0x8c8e9 (0x4026a8e9) 
nsAppShell::Run() 
nsAppShellService::Run() 
main1() 
main() 
libc.so.6 + 0x189cb (0x4043f9cb) 

Comment 55

[Peter Lubczynski]

I don't think that the talkback report is for this bug or this bug has morphed
as people's comments in the report indicate, the crash in ::GetInstance() is a
more general probelm than just with navigator.plugins.refresh(1). Perhaps we
should open a new bug?

Andrei, as a hack to fix the more general problem, perhaps we could publicly
expose nsActivePluginList in such a way that InstanceOwner can  get at the them
and check the "stopped" state or even if the instance with the stale pointer is
in the list. I can see this "bullet proofing" here and in other places where we
need to know the active plugin instances but I don't like the idea that this is
needed because the "instance" went away without it's "instance owner"!!!!

Comment 56

[av (gone)]

Peter, the fact that the patch 
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=37658
help us prevent crash has nothing to do with redirection to another page first. 
It is all about doing ReloadPlugins(PR_FALSE) rather than ReloadPlugind(1), so 
the simplest patch with the same effect would be just this one-liner:

 nsCOMPtr<nsIPluginManager> pm(do_QueryInterface(mPluginHost));

 if(pm)
-  pm->ReloadPlugins(aReloadDocuments);
+  pm->ReloadPlugins(PR_FALSE);

 if (aReloadDocuments && mDocShell) {
   nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(mDocShell));

   if (webNav)
     webNav->Reload(nsIWebNavigation::LOAD_FLAGS_NONE);
 }

Comment 57

[av (gone)]

Attachment: What a patch!

Comment 58

[av (gone)]

The patch above just notifies the PluginInstanceOwner that the plugin instance 
has been destroyed by setting its member variable mInstance to null. Note that 
nsPluginInstanceOwner destructor was not bulletproofed againt the situation when 
mInstance in null.

Comment 59

[Peter Lubczynski]

wonderful! r=peterl

Comment 60

[Marc Attinasi]

sr=attinasi

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

a=roc on behalf of drivers

Comment 62

[av (gone)]

Checked in. Marking fixed.

Comment 63

[Peter Lubczynski]

Reopening. Andrei, you forgot to add 

+    NS_IF_RELEASE(mInstance);

to nsPluginViewer.cpp. This causes a crash.

Comment 64

[Peter Lubczynski]

Since I'm touching that file in bug 59749 anyway, I've included the fix in
there. Take or leave the new interface ;) Here's the patch:

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=39274

Comment 65

[av (gone)]

Ah! My bad. Do you think you will check 59749 in soon or I should push this 
one-liner? Thanks for the catch!

Comment 66

[Peter Lubczynski]

Either way is fine with me. Both need to go through the review/approval process,
but a one liner is quicker than a new interface with a cross module patch.. That
last patch fixs the bug with no side effects (that I have seen) except we now
have a public nsIPluginViewer interface (in IDL too!) which currently only
serves the one simple task of cleaning the titlebar from the docshell. We kind
of needed that anyway, and can add functionality like scripting to it later.

Could your review that patch?

Comment 67

[av (gone)]

Patch to fix the remaining issue:

Index: nsPluginViewer.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/plugin/nglsrc/nsPluginViewer.cpp,v
retrieving revision 1.58
diff -u -r1.58 nsPluginViewer.cpp
--- nsPluginViewer.cpp  2001/06/18 21:41:02     1.58
+++ nsPluginViewer.cpp  2001/06/20 21:52:41
@@ -951,7 +951,7 @@
       if(host)
         host->StopPluginInstance(mInstance);
     }
-    NS_RELEASE(mInstance);
+    NS_IF_RELEASE(mInstance);
   }

   mWindow = nsnull;

Comment 68

[av (gone)]

We should stop this crash as soon as possible. Please review and approve.

Comment 69

[Peter Lubczynski]

r=peterl

Comment 70

[Marc Attinasi]

sr=attinasi - since it was in the original patch, but you forgot to check that
part in, I'd say that the original r/sr/a all apply (but that is just my opinion)

Comment 71

[chris hofmann]

a-chofmann

Comment 72

[Peter Lubczynski]

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

Comment 73

[av (gone)]

Checked in. ==> fixed.

Comment 74

[Christopher Blizzard (:blizzard)]

...just tracking...

Comment 75

[shrirang khanzode]

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

Comment 76

[shrirang khanzode]

crash not happening anymore on today's win 0621 trunk. verif all dups...

Comment 77

[varada]

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