85701 - M091, M092 problems using plugins [@ gtk_xtbin_init]

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[greer]

Talkback data for M091 shows users crashing possibly in conjusction with the 
use of plugins. The comments may provide a lead on steps to reproduce:

     (31651650) Comments: trying to get libflashplayer.so to work (mv'd 
libnullplugin.so)

     (31615765) URL: http://www.weather.com/weather/local/02115
     (31597555) URL: www.slashdot.org
     (31597555) Comments: Browsing Slashdot and our Netsaint Netmon

     (31532458) URL: http://www.weather.com/weather/local/02115

     (31532368) URL: http://www.weather.com/weather/local/02115
     (31532368) Comments: I started Mozilla

     (31519834) URL: http://dsc.discovery.com/news/reu/20010604/mammoth.html

     (31519832) URL: http://dsc.discovery.com/news/reu/20010604/mammoth.html
     (31519832) Comments: open the page and it crashes.  it doesn't crash when
flash/shockwave isn't installed.
 
    (31468297) Comments: tried to use flash

    (31467553) Comments: Trying to use flash plugin

Here's the stack:

  gtk_xtbin_init()
         libgtk-1.2.so.0 + 0xe85e6 (0x402ce5e6)
         gtk_xtbin_new()
         ns4xPluginInstance::SetWindow()
         nsPluginHostImpl::InstantiateEmbededPlugin()
         nsObjectFrame::InstantiatePlugin()
         nsObjectFrame::Reflow()
         nsContainerFrame::ReflowChild()
         nsObjectFrame::Reflow()
         nsLineLayout::ReflowFrame()
         nsBlockFrame::ReflowInlineFrame()
         nsBlockFrame::DoReflowInlineFrames()
         nsBlockFrame::DoReflowInlineFramesAuto()
         nsBlockFrame::ReflowInlineFrames()
         nsBlockFrame::ReflowLine()
         nsBlockFrame::ReflowDirtyLines()
         nsBlockFrame::Reflow()
         nsBlockReflowContext::DoReflowBlock()
         nsBlockReflowContext::ReflowBlock()
         nsBlockFrame::ReflowBlockFrame()
         nsBlockFrame::ReflowLine()
         nsBlockFrame::ReflowDirtyLines()
         nsBlockFrame::Reflow()
         nsBlockReflowContext::DoReflowBlock()
         nsBlockReflowContext::ReflowBlock()
         nsBlockFrame::ReflowBlockFrame()
         nsBlockFrame::ReflowLine()
         nsBlockFrame::ReflowDirtyLines()
         nsBlockFrame::Reflow()
         nsContainerFrame::ReflowChild()
         CanvasFrame::Reflow()
         nsBoxToBlockAdaptor::Reflow()
         nsBoxToBlockAdaptor::DoLayout()
         nsBox::Layout()
         nsScrollBoxFrame::DoLayout()
         nsBox::Layout()
         nsContainerBox::LayoutChildAt()
         nsGfxScrollFrameInner::LayoutBox()
         nsGfxScrollFrameInner::Layout()
         nsGfxScrollFrame::DoLayout()
         nsBox::Layout()
         nsBoxFrame::Reflow()
         nsGfxScrollFrame::Reflow()
         nsContainerFrame::ReflowChild()
         ViewportFrame::Reflow()
         nsHTMLReflowCommand::Dispatch()
         PresShell::ProcessReflowCommand()
         PresShell::ProcessReflowCommands()
         HandlePLEvent()
         PL_HandleEvent()
         PL_ProcessPendingEvents()
         nsEventQueueImpl::ProcessPendingEvents()
         event_processor_callback()
         our_gdk_io_invoke()
         libglib-1.2.so.0 + 0xea7a (0x4035fa7a)
         libglib-1.2.so.0 + 0x10055 (0x40361055)
         libglib-1.2.so.0 + 0x10659 (0x40361659)
         libglib-1.2.so.0 + 0x107e8 (0x403617e8)
         libgtk-1.2.so.0 + 0x91213 (0x40277213)
         nsAppShell::Run()
         nsAppShellService::Run()
         main1()
         main()

Comment 1

[greer]

Adding keywords crash, topcrash, qawanted

Comment 2

[John Morrison]

-> Plugins.

Comment 3

[Peter Lubczynski]

Srirang is this still happening?

cc:ing Serge and Dan. This appears to be a top crash on top 100 sits on Linux.

Comment 4

[serge (gone)]

Yes, this should be tough on. It's  clear from TB reports the crash happens
on derefs null pointer:
x86 Registers: 
 EAX:00000000
                   
Code Around the PC: 
 4071a9c0 8b5008           mov     edx,[eax+0x8]
 4071a9c3 8d8328020000     lea     eax,[ebx+0x228]
-----
investigating...

Comment 5

[serge (gone)]

All right, somehow XtOpenDisplay returns NULL  here
http://lxr.mozilla.org/seamonkey/source/widget/src/gtkxtbin/gtkxtbin.c#203
and the crash happens here 
http://lxr.mozilla.org/seamonkey/source/widget/src/gtkxtbin/gtkxtbin.c#214
because it actually looks like
cnumber = (((_XPrivDisplay)xtdisplay)->fd); 
and offsetof(_XPrivDisplay, fd) == 8,
which explains why we crash on 4071a9c0 8b5008 mov  edx,[eax+0x8]

The patch proposal is following

Comment 6

[serge (gone)]

Attachment: the first patch

Comment 7

[serge (gone)]

Attachment: the same patch with cvs diff -uw

Comment 8

[Blake Ross]

greer, is this still showing up as a topcrash?

serge/av, what's the latest with this patch?

Comment 9

[greer]

Trunk had two incidents of this signature as late as 7/14 from the following 
builds (one looks like testing):
 
   First Appearance Date : 2001-07-03
     Last Appearance Date : 2001-07-09
     First Build ID : 2001070308
     Latest Build ID : 2001070608

     (32692310) Comments: Logged out and then clicked "log out completely"
     (32487021) URL: http://espn.go.com
     (32487021) Comments: This is for a bug... I'll get the number next time.

However, the 0.9.2 milestone shows 31 incidents in today's data 
Here are the comments:

(32902885)        Comments: Closing browser window A
while browser window B was loading a site containing flash animation.
     (32864991) URL: http://www.channel5.co.uk/
     (32864991) Comments: Just opened that page and it crashed
     (32861928) URL: https://www.etrade.com
     (32852691) URL: https://caixadirecta.cgd.pt/cgi-bin/ibankApp/
     (32852691) Comments: Entering the page.The page has a applet and I have no 
Java installed...
     (32718206) URL: www.tudelft.nl
     (32718206) Comments: I just loaded the page. I have flash 5 installed
     (32676618) URL: opening www.libero.it
     (32654667) URL: http://www.mousepower.co.uk/
     (32654667) Comments: I think the crash was caused by the flash plugin.
     (32618010) URL: www.eye4u.com
     (32617916) URL: www.eye4u.com
     (32617916) Comments: Downloaded latest flash plugin and installed it. Dont 
know if they are
compatible with mozilla but it crashed as soon as a it was loaded going into 
www.eye4u.com40z
     (32609103) URL: www.abit.nl
     (32609103) Comments: Clicked a hyperlink.
     (32609100) Comments: While moving from ssl3-site to a new
     (32574273) URL: www.philzone.com
     (32574273) Comments: I just typed the above URL in the location field.
     (32559266) URL: http://www.mainframe.ca/
     (32547947) Comments: flash does not work

Updating summary to include M092.

Comment 10

[Blake Ross]

Andrei, serge, are these patches still good to go?

Comment 11

[av (gone)]

Serge is on vacation till the end of the week. Can somebody try to apply the 
patch and see if it is of any good?

Comment 12

[selmer (gone)]

Patch looks like almost all null check stuff except at 724 where this happened:

-      if (!mXtBin) {
-        mXtBin = gtk_xtbin_new(win, 0);
-      } 

Has anyone tested with this patch?  As much as I want topcrash fixes tonite, I
don't think this one is ready to go yet.  Of course, there are a couple hours
left until builds kick off...

Comment 13

[Chris McAfee]

Trunk linux build, I can't get this to fail w/o the patch on either url
in the orig. report, with or without flash installed.  Installing patch
doesn't seem to change anything, e.g. things seem to work for me.
rh7.1.  I suggest we bake this fix on the trunk first.

Comment 14

[serge (gone)]

Steve, 724 now is
+      if (mXtBin || (mXtBin = gtk_xtbin_new(win, 0))) {
       gtk_widget_set_usize(mXtBin, window->width, window->height);
and if mXtBin == 0 I'm not going inside the if

Comment 15

[Sitsofe Wheeler]

This may be a dup of bug 66005 (Crash on EMBED tag). It may be being caused by the plugger plugin 
in the netscape/mozilla plugin directory.

Comment 16

[serge (gone)]

This one has exact talkback signature == gtk_xtbin_init
if bug 66005 has the same signature we can say it's a dup,
I did not find any talkback reports in bug 66005, though.
I'm sure the patch, I've proposed to fix this bug, will also fix bug 79601 
(see my comment & patch proposal for 76001), which probably is a dup of 66005

Comment 17

[chris hofmann]

looks like something we would want to take on the branch

Comment 18

[av (gone)]

pavlov, can you please review and/or comment?

Comment 19

[Stuart Parmenter]

Attachment: cleaner patch

Comment 20

[Stuart Parmenter]

First off, I can't seem to reproduce this... but ignoring that fact, here's a
cleaner version which will return NULL to gtk_xtbin_new() if the display init
fails.  This allows easier checks on the outside in the plugin code.

Hope this helps...

Comment 21

[av (gone)]

Thanks! Serge, would you adopt this?

Comment 22

[serge (gone)]

gtkxtbin.c looks good.
for ns4xPluginInstance.cpp I would add 
RCS file: /cvsroot/mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp,v
retrieving revision 1.59
diff -u -r1.59 ns4xPluginInstance.cpp
--- ns4xPluginInstance.cpp	2001/08/16 04:47:16	1.59
+++ ns4xPluginInstance.cpp	2001/09/14 19:37:10
@@ -847,7 +847,8 @@
     ws = (NPSetWindowCallbackStruct *)window->ws_info;
 
     GdkWindow *win = gdk_window_lookup((XID)window->window);
-    if (win)
+    if (!win)
+      return NS_ERROR_FAILURE; 
     {

Comment 23

[elaineking]

removed keyword nsbranch since it now has nsbranch+, per pdt mtg.

Comment 24

[Jaime Rodriguez, Jr.]

Who should review this, so we can get it checked in??? -PDT

Comment 25

[Cindy Roberts]

The last engineering comment was on the 14th of September.  We'd like to get
this into eMojo.  Please let us know when the reviews are completed (or if
you're having difficulty getting reviews) by writing to [e-mail].

Comment 26

[Peter Lubczynski]

Serge/Pavlov: what's the status with the patch in this bug? Does it just need
reviews?

Comment 27

[av (gone)]

Serge, would you please take an ownership and push this for the branch asap?

Comment 28

[Jaime Rodriguez, Jr.]

pls get a r/sr= from av and pav, so we can take this one for the branch - PDT

Comment 29

[Stuart Parmenter]

r=pavlov on serge's addition to my patch.  serge: can you r= my patch?  get
someone to sr= it all.

Comment 30

[serge (gone)]

r=serge on pavlov's Attachment 49328 [details]

Comment 31

[serge (gone)]

correction: Attachment 49326 [details] [diff]

Comment 32

[av (gone)]

Over to serge

Comment 33

[Christopher Blizzard (:blizzard)]

Comment on attachment 49326 [details] [diff] [review]
cleaner patch

sr=blizzard

Comment 34

[Christopher Blizzard (:blizzard)]

( why the hell would that fail, anyway? )  Someone using --display with gtk?  No
env variable?

Comment 35

[serge (gone)]

I wasn't be able to reproduce this ever, but here is my wild guess:
gtk_xtbin_new (...)
{
...
 if (xt_is_initialized == 0) {
    char         *mArgv[1];
...
    xtdisplay = XtOpenDisplay()
...
  }
 xt_is_initialized++; /* bump up our count here */
----
after 2^32 calls of gtk_xtbin_new(),  xt_is_initialized is getting overflow, and 
we'll XtOpenDisplay and
at least in debugger I always got fail on second call here, so maybe it is worth 
to move xt_is_initialized++
inside if brackets?

Comment 36

[Jaime Rodriguez, Jr.]

check it in - PDT+

Comment 37

[Jaime Rodriguez, Jr.]

Jay - Pls watch for regressions (if any) ont his one.

Comment 38

[serge (gone)]

checked into MOZILLA_0_9_4_BRANCH
/cvsroot/mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp,v  <--  
new revision: 1.59.2.5; previous revision: 1.59.2.4
/cvsroot/mozilla/widget/src/gtkxtbin/gtkxtbin.c,v  <--
new revision: 1.8.22.1; previous revision: 1.8

Comment 39

[serge (gone)]

on the trunk
/cvsroot/mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp,v  <--  
new revision: 1.66; previous revision: 1.65
/cvsroot/mozilla/widget/src/gtkxtbin/gtkxtbin.c,v  <-- 
new revision: 1.9; previous revision: 1.8

Comment 40

[shrirang khanzode]

since there was no reproducible steps for this bug ever...I tried all url's 
mentioned in the talkback reports on today's branch on linux (1001) and verified 
that they load properly/do not crash...adding vtrunk