164043 - Mozilla crashes in calls to NPN_Status [trunk]

Status: VERIFIED FIXED

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

Description

[Steve K]

Plugins calling NPN_Status seem to make mozilla crash reliably.

This happens, on the trunk, since as early as 1.1B, and continuing to this
morning's build (20020822xx).  It does not happen with branch build 2002062705.

The Url http://stevek.com/ForMozilla/NPN_Status/start.html contains an example
plugin (auto installing), which should just start and say some stuff to you.  Of
course, on these mozilla builds, it has a side-effect of causing a crash.

Truncated Crash log follows.  Easily reproducable, they all pretty much point to
the same place:

**********

Date/Time:  2002-08-22 11:12:39 -0400
OS Version: 10.2 (Build 6C106)
Host:       dyn133.horizonlive.com

Command:    Mozilla
PID:        3010

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_PROTECTION_FAILURE (0x0002) at 0x00000000

Thread 0 Crashed:
 #0   0x02520d8c in status
 #1   0x025a58f8 in NPN_Status
 #2   0x025aa614 in StartDoorInstallProcessing__FP4_NPPRCQ23std59basic_string<c,Q2
 #3   0x025af564 in StartDoorStartProcessing__FP4_NPPRCQ23std59basic_string<c,Q23s
 #4   0x025a8368 in NPP_New
 #5   0x025a5bac in Private_New(char *, _NPP *, unsigned short, short, char **,
char **, _NPSavedData *)
 #6   0x0251e878 in InitializePlugin__18ns4xPluginInstanceFP21nsIPluginInstancePee

Comment 1

[serge (gone)]

yep, dereferencing of  null ptr:(
it's all platform.
taking this bug, patch is following

Comment 2

[serge (gone)]

Attachment: patch v1

Comment 3

[Steve K]

Jeez, 149 minutes from report to fix.  you guys are such slackers :)

(change platform, etc).

Note to testers: although the bug seems to be XP, the URL currently points to a
test case only set up for Mac OS X (I already got 2 mails from Windows users
about this).

Comment 4

[Peter Lubczynski]

Comment on attachment 96325 [details] [diff] [review]
patch v1

Since |GetPeer| may be implemented by a plugin vendor, should 'peer' be
initilized to NULL here? In any case, r=peterl

Comment 5

[serge (gone)]

Attachment: patch v2, more complete

yeah, since |GetPeer| may be implemented by a plugin vendor it'll be more
safety to check both return value & out param.
thanks.

Comment 6

[serge (gone)]

Attachment: patch v3

with null initilizer for peer

Comment 7

[Peter Lubczynski]

Comment on attachment 96373 [details] [diff] [review]
patch v3 

r=peterl

Comment 8

[Patrick C. Beard]

Comment on attachment 96373 [details] [diff] [review]
patch v3 

While you're at it, why not switch to nsCOMPtrs?

Comment 9

[serge (gone)]

well, I think nsCOMPtrs support is a little bit heavy than raw ptr, at least I
did some testing of MS disassemler debug code,
here is what I got
for raw prt:
  {
   nsIPluginInstancePeer *peer = nsnull;
   if (NS_SUCCEEDED(inst->GetPeer(&peer)) && peer){
     peer->ShowStatus(message);
     NS_RELEASE(peer);
   }
  }
29 disassemler statements
grep call as_raw
016B6DA2   call        dword ptr [edx+10h]
016B6DC1   call        dword ptr [eax+1Ch]
016B6DCD   call        dword ptr [eax+8]
3 calls
----
for nsCOMPtrs:
  {
    nsCOMPtr<nsIPluginInstancePeer> peer;
    if (NS_SUCCEEDED(inst->GetPeer(getter_AddRefs(peer))) && peer) {
      peer->ShowStatus(message);
    }
  }
51 disassemler statements
grep call as_com
016B6DF0   call   
@ILT+6865(nsCOMPtr<nsIPluginInstancePeer>::nsCOMPtr<nsIPluginInstancePeer>)
016B6DFD   call        @ILT+4575(getter_AddRefs) (016b21e4)
016B6E07   call        @ILT+7535(nsGetterAddRefs<nsIPluginInstancePeer>::operator 
016B6E16   call        dword ptr [ecx+10h]
016B6E2F   call        @ILT+8490(nsCOMPtr<nsIPluginInstancePeer>::operator 
016B6E56   call       
@ILT+7455(nsGetterAddRefs<nsIPluginInstancePeer>::~nsGetterAddRefs<nsIPluginInstancePeer
016B6E6F   call        @ILT+7230(nsCOMPtr<nsIPluginInstancePeer>::operator->)
016B6E80   call        dword ptr [ecx+1Ch]
016B6E86   call       
@ILT+5910(nsCOMPtr<nsIPluginInstancePeer>::~nsCOMPtr<nsIPluginInstancePeer>)(016b271b)
9 calls
----
but this is debug code, for optimized one the result definitely is going to be
different, I haven't test it yet, but I doubt nsCOMPtr will get any gain compare
to raw ptr.
of course, I've read scc's http://www.mozilla.org/projects/xpcom/nsCOMPtr/bloat.html

Comment 10

[serge (gone)]

Attachment: raw ptr disassembly

Patrick, here is disassembler code I got from optimized (-O1) build on 
msdev6.0 for blocks of code I've mentioned above.
wc c:\temp\raw.txt
  19	  79	 748 c:\temp\raw.txt
grep call c:\temp\raw.txt
016E434F   call        dword ptr [eax+10h]
016E4366   call        dword ptr [ecx+1Ch]
016E436F   call        dword ptr [ecx+8]

Comment 11

[serge (gone)]

Attachment: nsCOMPtr disassembly

wc c:\temp\com.txt
  42	 181	1855 c:\temp\com.txt
grep call c:\temp\com.txt
016E437E   call        @ILT+2490(getter_AddRefs) (016e19bf)
016E4392   call        dword ptr [ecx+8]
016E4399   call        dword ptr [eax+10h]
016E43B4   call       
@ILT+3430(nsCOMPtr<nsIPluginInstancePeer>::Assert_NoQue..
016E43D7   call        dword ptr [__imp_?PreCondition@nsDebug@@SAXPBD00H@Z 
016E43E9   call        dword ptr [ecx+1Ch]

Comment 12

[serge (gone)]

Comment on attachment 97663 [details]
nsCOMPtr disassembly

please disregard this attachment
i did not turn off debug macros, so this code has debug info in.

>016E43C8   push        offset string "../../../../dist/include/xpcom\\n"... (01711610)
>016E43CD   push        offset string "mRawPtr != 0" (01711600)
>016E43D2   push        offset string "You can't dereference a NULL nsC"... (017115bc)
>016E43D7   call        dword ptr [__imp_?PreCondition@nsDebug@@SAXPBD00H@Z

Comment 13

[serge (gone)]

Attachment: nsCOMPtr disassembly -O1 w/o debug

wc c:\temp\com-2.txt
  20	  79	 792 c:\temp\com-2.txt
grep call  c:\temp\com-2.txt
016E44A9   call        dword ptr [eax+10h]
016E44C5   call        dword ptr [edx+1Ch]
016E44CC   call        dword ptr [__imp_??1nsCOMPtr_base@@QAE@XZ (01716a78)]
...
the conclusion:
the number of machine code lets say are the same;
the number of calls are the same too:)
So I'm changing my mind and I'll use nsCOMPtrs whenever it's possible,
the source looks more clear & there's less chance forgot to release something.

Comment 14

[serge (gone)]

Attachment: patch v4, with nsCOMPtr instead of raw ptr

Comment 15

[Peter Lubczynski]

Comment on attachment 97711 [details] [diff] [review]
patch v4, with nsCOMPtr instead of raw ptr

r=peterl

Comment 16

[serge (gone)]

Patrick, would you sr= here, please?

Comment 17

[Steve K]

We're just waiting for a review here, right..  Seems we've missed the milestone.
  Can we try to get this in for the beta?

Comment 18

[serge (gone)]

changing TM

Comment 19

[Patrick C. Beard]

Comment on attachment 97711 [details] [diff] [review]
patch v4, with nsCOMPtr instead of raw ptr

sr=beard

Comment 20

[serge (gone)]

Checked in ns4xPlugin.cpp;
new revision: 1.91; previous revision: 1.90
Thanks all.

Comment 21

[serge (gone)]

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

Comment 22

[shrirang khanzode]

verified for sure this is fixed.(1014 trunk)