70286 - crash in [@ PluginArrayImpl::GetLength]

Status: VERIFIED WORKSFORME

(Core Graveyard :: Plug-ins, defect)

Description

[shrirang khanzode]

Filing this bug from the top stack trace from 6.01
No exact steps to reproduce this bug are mentioned in the user comments :(
Stack Trace: 

         PluginArrayImpl::GetLength     
[d:\builds\6.01\mozilla\dom\src\base\nsPluginArray.cpp  line 110] 
         MimeTypeArrayImpl::GetMimeTypes        
[d:\builds\6.01\mozilla\dom\src\base\nsMimeTypeArray.cpp  line 165] 
         MimeTypeArrayImpl::NamedItem   
[d:\builds\6.01\mozilla\dom\src\base\nsMimeTypeArray.cpp  line 131] 
         GetMimeTypeArrayProperty       
[d:\builds\6.01\mozilla\dom\src\base\nsJSMimeTypeArray.cpp  line 117] 
         js_GetProperty [d:\builds\6.01\mozilla\js\src\jsobj.c  line 2048] 
         js_Interpret   [d:\builds\6.01\mozilla\js\src\jsinterp.c  line 2471] 
         js_Execute     [d:\builds\6.01\mozilla\js\src\jsinterp.c  line 994] 
         JS_EvaluateUCScriptForPrincipals       
[d:\builds\6.01\mozilla\js\src\jsapi.c  line 3148] 
         nsJSContext::EvaluateString    
[d:\builds\6.01\mozilla\dom\src\base\nsJSEnvironment.cpp  line 592] 
         HTMLContentSink::EvaluateScript        
[d:\builds\6.01\mozilla\layout\html\document\src\nsHTMLContentSink.cpp  line 
4590] 
         HTMLContentSink::ProcessSCRIPTTag      
[d:\builds\6.01\mozilla\layout\html\document\src\nsHTMLContentSink.cpp  line 
4939] 
         HTMLContentSink::AddLeaf       
[d:\builds\6.01\mozilla\layout\html\document\src\nsHTMLContentSink.cpp  line 
3157] 
         CNavDTD::AddLeaf       
[d:\builds\6.01\mozilla\htmlparser\src\CNavDTD.cpp  line 3711] 
         CNavDTD::HandleScriptToken     
[d:\builds\6.01\mozilla\htmlparser\src\CNavDTD.cpp  line 2152] 
         CNavDTD::OpenContainer 
[d:\builds\6.01\mozilla\htmlparser\src\CNavDTD.cpp  line 3379] 
         0x0a280070

Comment 1

[av (gone)]

Futuring this till we have reproduceable steps and/or a testcase.

Comment 2

[Peter Lubczynski]

Shrir,

Is this a talkback reports? We could probably take a stab at fixing it.

Comment 3

[shrirang khanzode]

yeah, it;s from he 6.01 talkback list that the guys forwarded.. no steps for 
reproducing this crash were given :(

Comment 4

[Peter Lubczynski]

I don't see any possible way this can fail:

117 NS_IMETHODIMP
118 PluginArrayImpl::GetLength(PRUint32* aLength)
119 {
120   if (mPluginHost && NS_SUCCEEDED(mPluginHost->GetPluginCount(aLength)))
121     return NS_OK;
122   return NS_ERROR_FAILURE;
123 }

Looks bullet proof already. I nominate WFM?

Comment 5

[av (gone)]

It will crash in nsPluginHostImpl::GetPluginCount if aLength is zero. But it is 
not shown in the call stack. I cannot see any other reasons. We should probably 
do the following:

NS_IMETHODIMP
PluginArrayImpl::GetLength(PRUint32* aLength)
{

+  NS_ENSURE_ARG_POINTER(aLength);

  if (mPluginHost && NS_SUCCEEDED(mPluginHost->GetPluginCount(aLength)))
    return NS_OK;
  return NS_ERROR_FAILURE;
}

Comment 6

[Peter Lubczynski]

But aLength is a return value. It can be null or even garbage or am I missing 
something?

Comment 7

[av (gone)]

This is a pointer to a place for the return value. In 
nsPluginHostImpl::GetPluginCount it will do something like

  *aLength = somevalue;

which to me looks like a crasher.

Comment 8

[Syd Logan]

But aLength is not being deref'd here, just being passed on down. Perhaps
mPluginHost is non-NULL but invalid?

Comment 9

[av (gone)]

It is being passed down to nsPluginHostImpl::GetPluginCount where it gets 
dereferenced. 

Comment 10

[Peter Lubczynski]

Looks good time r=peterl Check-in to avoid crash.

Comment 11

[av (gone)]

Moving to 0.9 as it seems an easy one.

Comment 12

[Chris Waterson]

sr=waterson

Comment 13

[Johnny Stenback (:jst)]

MimeTypeArrayImpl::GetMimeTypes() never calls GetLength() with a null pointer,
adding the null check does not fix the real problem here.

Comment 14

[av (gone)]

We can check it anyway, will not hurt. So the problem is definitely mPluginHost 
being invalid although non-zero.

Get/Release every time it is needed?

Comment 15

[av (gone)]

Johnny, can you reproduce this?

Comment 16

[Johnny Stenback (:jst)]

I haven't even tried reproducing this problem.

Comment 17

[av (gone)]

Futuring till we are able to reproduce it.

Comment 18

[Jussi-Pekka Mantere]

Removing nsterprise nomination.

Comment 19

[Stephen Ostermiller]

This bug has no testcase and yet it has the testcase keyword.

Comment 20

[Peter Lubczynski]

Is this still showing up on talkback?

Comment 21

[Peter Lubczynski]

I coudn't find any more crashes in talkback with this stack so I'm marking WFM.
Feel free to reopen but we could really use a testcase.

Comment 22

[shrirang khanzode]

.