Closed Bug 90951 Opened 23 years ago Closed 23 years ago

Trunk & N610 crash in [@ 0x00000000 - MimeTypeElementImpl::GetEnabledPlugin] after plugin.refresh()

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.3

People

(Reporter: peterlubczynski-bugs, Assigned: peterlubczynski-bugs)

References

()

Details

(Keywords: crash, topcrash, Whiteboard: PDT+)

Crash Data

Attachments

(3 files)

Click reload while on this page (or the page it redirects to).
Notice the crash.

Looks like something in the MIME-type array, the plugin's address looks invalid.
ns_if_addref(nsIDOMPlugin * 0x037bba88) line 1129 + 15 bytes
MimeTypeElementImpl::GetEnabledPlugin(MimeTypeElementImpl * const 0x037bc528,
nsIDOMPlugin * * 0x0012de34) line 217 + 21 bytes
XPTC_InvokeByIndex(nsISupports * 0x037bc528, unsigned int 4, unsigned int 1,
nsXPTCVariant * 0x0012de34) line 139
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_GETTER) line 1881 + 42 bytes
XPCWrappedNative::GetAttribute(XPCCallContext & {...}) line 1771 + 14 bytes
XPC_WN_GetterSetter(JSContext * 0x03283138, JSObject * 0x0354db78, unsigned int
0, long * 0x0368a178, long * 0x0012e07c) line 1284 + 9 bytes
js_Invoke(JSContext * 0x03283138, unsigned int 0, unsigned int 2) line 807 + 23
bytes
js_InternalInvoke(JSContext * 0x03283138, JSObject * 0x0354db78, long 56825000,
unsigned int 0, unsigned int 0, long * 0x00000000, long * 0x0012edd4) line 896 +
20 bytes
js_GetProperty(JSContext * 0x03283138, JSObject * 0x0354db78, long 56342688,
long * 0x0012edd4) line 2408 + 45 bytes
js_Interpret(JSContext * 0x03283138, long * 0x0012f000) line 2535 + 1998 bytes
js_Execute(JSContext * 0x03283138, JSObject * 0x03316890, JSScript * 0x036ffdd0,
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0012f000) line 986 + 13 bytes
JS_EvaluateUCScriptForPrincipals(JSContext * 0x03283138, JSObject * 0x03316890,
JSPrincipals * 0x036970d8, const unsigned short * 0x0012f160, unsigned int 47,
const char * 0x036ffbf8, unsigned int 81, long * 0x0012f000) line 3273 + 25 bytes
nsJSContext::EvaluateString(nsJSContext * const 0x03311518, const nsAString &
{...}, void * 0x03316890, nsIPrincipal * 0x036970d4, const char * 0x036ffbf8,
unsigned int 81, const char * 0x01008688, nsAString & {...}, int * 0x0012f06c)
line 608 + 85 bytes
nsScriptLoader::EvaluateScript(nsScriptLoadRequest * 0x0398ee38, const
nsAFlatString & {...}) line 565
nsScriptLoader::ProcessRequest(nsScriptLoadRequest * 0x0398ee38) line 477 + 22 bytes
nsScriptLoader::ProcessScriptElement(nsScriptLoader * const 0x036c05e8,
nsIDOMHTMLScriptElement * 0x036d4388, nsIScriptLoaderObserver * 0x036d438c) line
420 + 15 bytes
nsHTMLScriptElement::SetDocument(nsHTMLScriptElement * const 0x036d4360,
nsIDocument * 0x03921d00, int 0, int 1) line 145
nsGenericHTMLContainerElement::AppendChildTo(nsGenericHTMLContainerElement *
const 0x036e6d80, nsIContent * 0x036d4360, int 0, int 0) line 3788
HTMLContentSink::ProcessSCRIPTTag(const nsIParserNode & {...}) line 5010
HTMLContentSink::AddLeaf(HTMLContentSink * const 0x036c0470, const nsIParserNode
& {...}) line 3435 + 12 bytes
CNavDTD::AddLeaf(const nsIParserNode * 0x03731188) line 3789 + 22 bytes
CNavDTD::HandleScriptToken(const nsIParserNode * 0x03731188) line 2259 + 12 bytes
CNavDTD::OpenContainer(const nsCParserNode * 0x03731188, nsHTMLTag
eHTMLTag_script, int 1, nsEntryStack * 0x00000000) line 3447 + 12 bytes
CNavDTD::HandleDefaultStartToken(CToken * 0x036d3128, nsHTMLTag eHTMLTag_script,
nsCParserNode * 0x03731188) line 1337 + 20 bytes
CNavDTD::HandleStartToken(CToken * 0x036d3128) line 1746 + 22 bytes
CNavDTD::HandleToken(CNavDTD * const 0x036991e0, CToken * 0x00000000, nsIParser
* 0x0264f018) line 910 + 12 bytes
CNavDTD::BuildModel(CNavDTD * const 0x036991e0, nsIParser * 0x0264f018,
nsITokenizer * 0x0364e5d8, nsITokenObserver * 0x00000000, nsIContentSink *
0x036c0470) line 540 + 20 bytes
nsParser::BuildModel() line 2210 + 34 bytes
nsParser::ResumeParse(int 1, int 0) line 2081 + 11 bytes
nsParser::OnDataAvailable(nsParser * const 0x0264f020, nsIRequest * 0x036e0df0,
nsISupports * 0x00000000, nsIInputStream * 0x0364bc28, unsigned int 16687,
unsigned int 13764) line 2684 + 19 bytes
nsDocumentOpenInfo::OnDataAvailable(nsDocumentOpenInfo * const 0x036e0490,
nsIRequest * 0x036e0df0, nsISupports * 0x00000000, nsIInputStream * 0x0364bc28,
unsigned int 16687, unsigned int 13764) line 235 + 46 bytes
nsStreamListenerTee::OnDataAvailable(nsStreamListenerTee * const 0x03670ff8,
nsIRequest * 0x036e0df0, nsISupports * 0x00000000, nsIInputStream * 0x0369ad48,
unsigned int 16687, unsigned int 13764) line 56 + 51 bytes
nsHttpChannel::OnDataAvailable(nsHttpChannel * const 0x036e0df4, nsIRequest *
0x0369af40, nsISupports * 0x00000000, nsIInputStream * 0x0369ad48, unsigned int
16687, unsigned int 13764) line 2146 + 57 bytes
nsOnDataAvailableEvent::HandleEvent() line 175 + 70 bytes
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x036fbdf4) line 64
PL_HandleEvent(PLEvent * 0x036fbdf4) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00d3c618) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x000b054a, unsigned int 49482, unsigned int 0,
long 13878808) line 1071 + 9 bytes
USER32! 77e12e98()
USER32! 77e130e0()
USER32! 77e15824()
nsAppShellService::Run(nsAppShellService * const 0x00df7558) line 422
main1(int 3, char * * 0x00358010, nsISupports * 0x00000000) line 1174 + 32 bytes
main(int 3, char * * 0x00358010) line 1478 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
I thought someone on the install team did the plugins.refresh code?
This bug is also in Bugscape http://bugscape/show_bug.cgi?id=7003
Peter, how is this page different (in whatever matters) from our regular 
about:plugins page which also does plugins.refresh() but doesn't crash on 
reload?
*** Bug 91569 has been marked as a duplicate of this bug. ***
Bug 91569 has another testcase for this involving about:plugins.

And, lots of crashes:
http://climate/reports/searchstacksignature.cfm?stacksig=MimeTypeElementImpl%3A%3AGetEnabledPlugin

Andrei, are you looking into this?  This needs to get on someone's radar...
Keywords: nsbeta1, nsBranch
Hardware: PC → All
Ahh! Andrei, it does crash with about:plugins, see bug 91569.

Steps to reproduce:
1. Launch the browser.
2. Go help menu by following steps:
   a. Feedback center.
   b. About Plug-ins
   c. Feedback center again.

The feedback url is: http://home.netscape.com/browsers/6/feedback/index61pr.html

It also has to be that order. When I replaced "Feedback center" with another
link, this did not happen.
Priority: -- → P1
On what platform do you experience this? Looks like this will be my first action 
item tomorrow...
Happens at least on Mac and Windows (W2k). I suspect all.

My guess is that the mime-type array doesn't get cleared out all the way and it
could be on the DOM side, but that's just a guess.
Summary: crash on reloading this page that does a plugins.refresh() → crash in MimeTypeElementImpl::GetEnabledPlugin after plugin.refresh()
Is this seen on the branch, or only the trunk?
I Just Created an attachment (id=42986) to illustrate my belief 
that the interaction between plugins.refresh() and the 
Universal Ad Call JS code is causing this bug.  This attachment
works fine in 6.0 and 6.01.  I'm using the 2001070304 build of 6.1 b1
in Win98.
Looking closer at this with Brian Nesse, it looks like in nsGlocalWindow.cpp,
NavigatorImpl caches the mime-type and plugin array. When a
navigator.plugin.refresh() happens, the weak pointers to these plugins become
stale and hence the crash when we revist a page that tried to access them. I
think we need some way of having NavigatorImpl refresh itself w.r.t plugins and
mimetypes when a navigator.plugins.refresh happens. Any ideas? I'm not familiar
with which interfaces, if any in this area, are frozen.
This has shown up as a topcrasher for the N610 branch builds (and from Blake's
last comment, this is happening on the MozillaTrunk as well).  adding crash,
topcrash keywords and [@ MimeTypeElementImpl::GetEnabledPlugin] to summary. 
Here is the latest info:

MimeTypeElementImpl::GetEnabledPlugin   8 
     First BBID :32840758
     Last BBID  :33115944
     Min Runtime :114
     Max Runtime :85536
     First Appearance Date : 2001-07-12
     Last Appearance Date : 2001-07-19
     First BuildID : 2001071106
     Last BuildID : 2001071905

Stack Trace: 

	 MimeTypeElementImpl::GetEnabledPlugin()
[nsMimeTypeArray.cpp  line 217]
	 CODE.10
	 XPTC_InvokeByIndex()
[xptcinvoke_mac.cpp  line 129]
	 XPCWrappedNative::CallMethod()
[xpcwrappednative.cpp  line 1880]
	 XPC_WN_GetterSetter()
[xpcwrappednativejsops.cpp  line 1284]
	 js_Invoke()
[jsinterp.c  line 801]
	 js_InternalInvoke()
[jsinterp.c  line 895]
	 js_GetProperty()
[jsobj.c  line 2408]
	 js_Interpret()
[jsinterp.c  line 2535]
 
 	Source File : nsMimeTypeArray.cpp line : 217
     (33112037)	URL: popularcategories.com
     (33112037)	Comments: Help -> Feedback
     (33109907)	Comments: Mac crash on Help menu
     (32840758)	URL: http://www.thewb.com/
(32840758)
Comments: Went to the Bookmarks menu

I also noticed that this crash is being reported under the 0x00000000 stack
signature as well...so although there aren't "a lot" of these crashes, it is
occurring under multiple stack signatures.

Keywords: crash, topcrash
Summary: crash in MimeTypeElementImpl::GetEnabledPlugin after plugin.refresh() → Trunk & N610 crash in [@ 0x00000000 - MimeTypeElementImpl::GetEnabledPlugin] after plugin.refresh()
Okay, I think I have a fix for this. Hacking away, I made it public and called
MimeTypeArrayImpl::GetMimeTypes(). That populated the mime-type array based on
the plugin array. I did this in PluginArrayImpl::Refresh(), by way of doing an
ugly static cast of mNavigator. I did not crash on the attached testcase or URL
above.

Looking at the interfaces involved here, I think I can clean this up neatly. I
also need to take care of not leaking the old stuff as GetMimeTypes() doesn't
account for it. However, I have some questions for an expert about the
navigator.mimetype array:

Specifically, would it be okay if I completely destroyed this array before any
call to plugins.refresh() and then rebuilt it via a call to GetMimeTypes()? Does
anything else but plugis use this array? Also, can I add GetMimeTypes() to an
interface or should I use a NS_STATIC_CAST and just make it public?

This is a top crash and good PDT+ material.
Assignee: av → peterlubczynski
Whiteboard: fix-in-hand
Target Milestone: --- → mozilla0.9.3
PDT+, we'll take the thoroughly reviewed and tested fix for this.  Any extra
eyes you can get on it would be appreciated.
Whiteboard: fix-in-hand → PDT+ fix-in-hand
Here's something that's a bit neater:
1) Pass in (and store) a NavigatorImpl* instead of an nsIDOMNavigator* to the
PluginArrayImpl constructor. This will avoid the nasty (and potentially
incorrect) casting. It's fine for these implementation classes to know about
each other.
2) Add a public PluginRefresh() method to NavigatorImpl. This does not have to
be part of any public interface.
3) Add a public Refresh() method to MimeTypeArrayImpl. This is the one that can
clean up an existing mMimeTypeArray. It will be recreated lazily the next time
it's needed. This method also does not have to be part of any public interface.
4) From NavigatorImpl::PluginRefresh() call MimeTypeArrayImpl::Refresh().
Here's a patch that should fix this top crash by completely cleaning out the
mime type array and re-populating it after a navigator.plugin.refresh() happens.

I'm seeking reviews (and some testing help).
Status: NEW → ASSIGNED
Keywords: patch
Whiteboard: PDT+ fix-in-hand → PDT+ [SEEKING REVIEWS]
Oh, I got my postfix and prefix operators mixed up last night. This:

+    while (--mMimeTypeCount)

...should be...

+    while (mMimeTypeCount--)
Review comments on attachment 43095 [details] [diff] [review] as modified by previous (10:53)
comment:

    I'm hoping this will also fix bug 86642. :-)

    Since Vidur suggested this approach, I'll assume there was never any
    intent of plugging in a replacement for one of these interfaces
    without replacing the other.

    For MimeTypeArrayImpl::Refresh, why don't you do the loop releasing
    members of the array and freeing of the array itself the same way
    it's done in the destructor?  (In fact, you could even refactor that
    loop in the destructor into an additional (inline) method.)  At your
    new call site you would also have to set the array to null and the
    count to 0 after the loop since you're going to continue to use the
    object.  There are two reasons I prefer this:

     * the |while (mMimeTypeCount--)| type loop is confusing, as
       demonstrated by your confusion (and mine :-)

     * you leak the array itself, and may end up leaving a dangling
       array of some sort in some cases that confuses other code
       (non-null array, but count of 0)

    To prevent someone else from making the same mistake in the future,
    could you add something like:

      NS_PRECONDITION(!mMimeTypeArray && mMimeTypeCount==0,
                      "already initialized")

    to the very beginning of GetMimeTypes?

With those comments addressed, looks good to me, r=dbaron.
Peter, I don't see mMimeTypeArray deleted before getting new one. Also, would be 
probably safer to zero it too. So, against your patch it could be something like 
this:

nsresult MimeTypeArrayImpl::Refresh()
{
  // clear out old array
  if (mMimeTypeArray) +{
    while (mMimeTypeCount--) // DON'T FORGET THIS POSTFIX!!!
      NS_IF_RELEASE(mMimeTypeArray[mMimeTypeCount]);
+
+   delete [] mMimeTypeArray;
+   mMimeTypeArray = nsnull;
+ }

  // fill it back up
  return GetMimeTypes();
}

Other than that -- r=av

On a different note, is this also going to fix (I don't remember the bug number) 
not refreshing mime type list while refreshing plugins?
Wow, this may take care of three birds with one stone (topcrash, topcrash, and
mime refreshing)...

Cleand up patch on it's way with a new factored Clear() method. 

dbaron, can I get a final sr=? Thanks!
r=dbaron on the new version, but you'll need to find a super-reviewer for an sr.
Whiteboard: PDT+ [SEEKING REVIEWS] → PDT+ [SEEKING SUPER REVIEW]
r=av on the last patch
Patch checked into trunk and branch, marking FIXED.
Blocks: 86642
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: PDT+ [SEEKING SUPER REVIEW] → PDT+
looks ok on all 3 branch builds today 0723. marking verif. adding 'vtrunk ' 
keywd to verify on trunk later.
Status: RESOLVED → VERIFIED
spam: adding 'vtrunk'
Keywords: vtrunk
verif on all trunk 0802 blds. no longer happens.
Keywords: vtrunk
*** Bug 90435 has been marked as a duplicate of this bug. ***
Crash Signature: [@ 0x00000000 - MimeTypeElementImpl::GetEnabledPlugin]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: