Last Comment Bug 613376 - [Mac, OOPP] Firefox 4.0b8pre crash in [@ nsNPAPIPluginInstance::GetOwner ], possibly only with Silverlight plugin
: [Mac, OOPP] Firefox 4.0b8pre crash in [@ nsNPAPIPluginInstance::GetOwner ], p...
Status: RESOLVED FIXED
: [sg:critical? on trunk][qa-ntd-192] [...
: crash
Product: Core
Classification: Components
Component: Plug-ins
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Steven Michaud
: plugins
: http://www.bug.hr/forum/newpost/?repl...
:
: CVE-2011-0059
  Show dependency treegraph
 
Reported: 2010-11-18 16:01 PST by Marcia Knous [:marcia]
Modified: 2011-03-29 19:39 PDT (History)
9 users (show)
See Also:
Crash Signature:
[@ nsNPAPIPluginInstance::GetOwner ]
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta8+
  ---
  .14+
  .14-fixed
  .17+
  .17-fixed


Attachments
Fix (1.17 KB, patch)
2010-11-22 18:56 PST, Steven Michaud
no flags Details | Diff | Splinter Review
Fix rev1 (restore assertion and change its message) (1.26 KB, patch)
2010-11-22 19:31 PST, Steven Michaud
joshmoz: review+
Details | Diff | Splinter Review
Additional trunk patch to deal with nsDummyJavaPluginOwner (808 bytes, patch)
2010-11-23 10:50 PST, Steven Michaud
joshmoz: review+
Details | Diff | Splinter Review
1.9.2-branch patch (2.05 KB, patch)
2010-11-23 13:20 PST, Steven Michaud
joshmoz: review+
dveditz: approval1.9.2.14+
Details | Diff | Splinter Review
1.9.1-branch patch (2.49 KB, patch)
2010-11-23 13:53 PST, Steven Michaud
joshmoz: review+
clegnitto: approval1.9.1.17+
Details | Diff | Splinter Review

Summon comment box

Description Marcia Knous [:marcia] 2010-11-18 16:01:06 PST
Seen while reviewing trunk crash stats and new to the D	20101118063017 build. http://tinyurl.com/2f56b6g to the crashes which are all Mac.

Frame 	Module 	Signature [Expand] 	Source
0 	XUL 	nsNPAPIPluginInstance::GetOwner 	nsISupportsUtils.h:94
1 	XUL 	nsPluginStreamListenerPeer::GetInterfaceGlobal 	modules/plugin/base/src/nsPluginStreamListenerPeer.cpp:1259
2 	XUL 	nsHttpChannel::OnTransportStatus 	nsNetUtil.h:1287
3 	XUL 	nsTransportStatusEvent::Run 	netwerk/base/src/nsTransportUtils.cpp:112
4 	XUL 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:610
5 	XUL 	NS_ProcessPendingEvents_P 	nsThreadUtils.cpp:200
6 	XUL 	nsBaseAppShell::NativeEventCallback 	widget/src/xpwidgets/nsBaseAppShell.cpp:131
7 	XUL 	nsAppShell::ProcessGeckoEvents 	widget/src/cocoa/nsAppShell.mm:399
8 	CoreFoundation 	CoreFoundation@0x4e400 	
9 	CoreFoundation 	CoreFoundation@0x4c5f8 	
10 	XUL 	PopupAllowedForEvent 	nsTSubstring.h:113
11 	XUL 	XUL@0x12eaabf 	
12 	XUL 	nsEventListenerManager::HandleEventInternal 	dom/base/nsPIDOMWindow.h:657
13 		@0x77 	
14 	libSystem.B.dylib 	libSystem.B.dylib@0x4ee9 	
15 	XUL 	nsGenericElement::AddRef 	nsISupportsImpl.h:161
16 	XUL 	nsCOMPtr_base::assign_with_AddRef 	nsCOMPtr.cpp:88
17 	XUL 	nsDOMEvent::DuplicatePrivateData 	nsAutoPtr.h:957
18 		@0x7fff7118962f 	
19 	CoreFoundation 	CoreFoundation@0x1b1ef 	
20 	XUL 	nsGenericElement::Release 	nsISupportsImpl.h:210
21 	XUL 	nsEventDispatcher::Dispatch 	nsCOMPtr.h:492
22 		@0x13ddfc07f 	
23 	libSystem.B.dylib 	libSystem.B.dylib@0x6bf9 	
24 	libSystem.B.dylib 	libSystem.B.dylib@0x527f 	
25 	libSystem.B.dylib 	libSystem.B.dylib@0x9a03 	
26 	libSystem.B.dylib 	libSystem.B.dylib@0x3b73a 	
27 	CoreFoundation 	CoreFoundation@0x609e3 	
28 	libSystem.B.dylib 	libSystem.B.dylib@0x823b 	
29 	CarbonCore 	CarbonCore@0x22d8d 	
30 	CarbonCore 	CarbonCore@0x22c9c 	
31 	HIToolbox 	HIToolbox@0x54fe5 	
32 	HIToolbox 	HIToolbox@0x30031 	
33 	HIToolbox 	HIToolbox@0x2ff7c 	
34 	libSystem.B.dylib 	libSystem.B.dylib@0x46ae 	
35 	HIToolbox 	HIToolbox@0x2fbaf 	
36 	CoreFoundation 	CoreFoundation@0x505a2 	
37 	XUL 	nsHttpConnection::OnOutputStreamReady 	netwerk/protocol/http/nsHttpConnection.cpp:802
38 		@0x7fff5fbfe45f 	
39 	HIToolbox 	HIToolbox@0x2f9ff 	
40 	libSystem.B.dylib 	libSystem.B.dylib@0x66b3 	
41 	CoreFoundation 	CoreFoundation@0xbaaf 	
42 	CoreFoundation 	CoreFoundation@0x60b1f 	
43 	CoreFoundation 	CoreFoundation@0x70f0f 	
44 	libSystem.B.dylib 	libSystem.B.dylib@0x66b3 	
45 	CoreFoundation 	CoreFoundation@0x70c62 	
46 	CoreFoundation 	CoreFoundation@0x709c7 	
47 	CoreFoundation 	CoreFoundation@0x4bdbe 	
48 	XUL 	-[ChildView keyUp:] 	nsTSubstring.h:113
49 	CoreFoundation 	CoreFoundation@0x1052b 	
50 	CoreFoundation 	CoreFoundation@0x2232a 	
51 	CoreFoundation 	CoreFoundation@0x1052b 	
52 	libSystem.B.dylib 	libSystem.B.dylib@0x66b3 	
53 	CoreFoundation 	CoreFoundation@0x65f1 	
54 	CoreFoundation 	CoreFoundation@0xfcd6 	
55 	CoreFoundation 	CoreFoundation@0xfb2e 	
56 	CoreFoundation 	CoreFoundation@0x2b91b 	
57 	CoreFoundation 	CoreFoundation@0x6378b 	
58 	AppKit 	AppKit@0x45166 	
59 	AppKit 	AppKit@0x436f1 	
60 	CoreFoundation 	CoreFoundation@0x104c7 	
61 	AppKit 	AppKit@0x7413e7 	
62 	AppKit 	AppKit@0x72ed8 	
63 	AppKit 	AppKit@0x437a8 	
64 	libSystem.B.dylib 	libSystem.B.dylib@0xa2d8 	
65 	CoreFoundation 	CoreFoundation@0x104c7 	
66 	CoreFoundation 	CoreFoundation@0x24d5b 	
67 	XUL 	js_InitReflectClass 	js/src/jsobjinlines.h:944
68 		@0x18f 	
69 	CoreFoundation 	CoreFoundation@0x249d2 	
70 	CoreFoundation 	CoreFoundation@0x54929 	
71 	CoreFoundation 	CoreFoundation@0x24a68 	
72 	libSystem.B.dylib 	libSystem.B.dylib@0x66c7 	
73 	CoreFoundation 	CoreFoundation@0x3527b 	
74 	AppKit 	AppKit@0x77e03f 	
75 	libSystem.B.dylib 	libSystem.B.dylib@0x66c7 	
76 	Foundation 	Foundation@0x2eed 	
77 	CoreFoundation 	CoreFoundation@0x2a883 	
78 	libSystem.B.dylib 	libSystem.B.dylib@0x66c7 	
79 	AppKit 	AppKit@0x77e03f 	
80 	AppKit 	AppKit@0x948a 	
81 	XUL 	nsAppShell::Run 	widget/src/cocoa/nsAppShell.mm:746
82 	XUL 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:191
83 	XUL 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3682
84 	firefox-bin 	main 	browser/app/nsBrowserApp.cpp:158
85 	firefox-bin 	firefox-bin@0x1953
Comment 1 Marcia Knous [:marcia] 2010-11-18 16:14:01 PST
Adding Josh since this looks similar to something mentioned earlier today.
Comment 2 Josh Aas (Mozilla Corporation) 2010-11-18 16:19:20 PST
Regression from beta 7, blocking beta8+.
Comment 3 Josh Aas (Mozilla Corporation) 2010-11-19 10:26:20 PST
This is really easy to repro thanks to a comment someone put in their crash report. Just visit this URL and the browser will crash:

http://blogs.msdn.com/b/ie/
Comment 4 Josh Aas (Mozilla Corporation) 2010-11-19 10:42:11 PST
(In reply to comment #3)
> This is really easy to repro thanks to a comment someone put in their crash
> report. Just visit this URL and the browser will crash:
> 
> http://blogs.msdn.com/b/ie/

This does not reproduce on the 1.9.2 branch so hopefully it isn't a problem at all there. This crash might be caused by the async redirect API on trunk, which would explain why I can't repro on 1.9.2.
Comment 5 Josh Aas (Mozilla Corporation) 2010-11-19 10:48:50 PST
I haven't been able to reproduce the crash going to the IE blog in a debug build. Only happens in a trunk nightly (opt) build for me.
Comment 6 Steven Michaud 2010-11-22 12:41:08 PST
As Josh has already noted, these crashes started with the 2010-11-18-06-mozilla-central nightly (on OS X), and thus presumably with Josh's patch for bug 573873.
Comment 7 Steven Michaud 2010-11-22 12:48:30 PST
Judging by the comments, it's possible these crashes only happen with
the Silverlight plugin.  Unfortunately, correlation data no longer
contains data for modules/plugins that run in child processes, so it's
impossible to be sure.

I don't really expect this to turn out to be a Silverlight bug,
though.

I'm not able to reproduce any crashes with http://blogs.msdn.com/b/ie/
on OS X 10.5.8.  And on OS X 10.6.5, they only happen in 64-bit mode
(not in 32-bit mode) -- so this bug is OOPP-only.
Comment 8 Steven Michaud 2010-11-22 13:08:47 PST
Looking through all topcrashers (on all platforms) for Firefox 4.0b8pre for the two days prior to 2011-11-20, I can't find any that might be the Windows or Linux counterparts of this bug's crash (judging by build ids).

http://crash-stats.mozilla.com/query/query?product=Firefox&version=Firefox%3A4.0b8pre&range_value=2&range_unit=days&date=11%2F20%2F2010&query_search=signature&query_type=exact&query=&build_id=&process_type=any&hang_type=any&do_query=1

So this bug really does appear to be OS-X-only.
Comment 9 Steven Michaud 2010-11-22 13:29:51 PST
As best I can tell, all these crashes take place at the following line, on an attempt to AddRef() a deleted nsNPAPIPluginInstance::mOwner object:

http://hg.mozilla.org/mozilla-central/annotate/0e9ba7c029e3/modules/plugin/base/src/nsNPAPIPluginInstance.cpp#l1203

Puzzling that these crashes are sometimes listed as null-dereferences ... but I can't think of any other way to interpret the data.
Comment 10 Steven Michaud 2010-11-22 18:56:56 PST
Created attachment 492551 [details] [review]
Fix

Turns out this bug has an easy fix.

The proximate cause is as follows:

nsIPluginInstanceOwner::SetInstance is called on the following line
(from nsPluginHost::TrySetUpPluginInstance()):

http://hg.mozilla.org/mozilla-central/annotate/2f9f6cb3a0ef/modules/plugin/base/src/nsPluginHost.cpp#l1362

Then in the next line (1367) nsIPluginInstance::Initialize() is
called.  But for some reason this call fails and
nsIPluginInstanceOwner::SetInstance() gets called again with aInstance
set to nsnull:

http://hg.mozilla.org/mozilla-central/annotate/2f9f6cb3a0ef/modules/plugin/base/src/nsPluginHost.cpp#l1369

Then (inside the call to nsPluginInstanceOwner::SetInstance())
nsPluginInstanceOwner::mInstance gets nulled out:

http://hg.mozilla.org/mozilla-central/annotate/2f9f6cb3a0ef/layout/generic/nsObjectFrame.cpp#l2917

Which in turn causes nsIPluginInstance::InvalidateOwner() to fail to
be called from the following line in the nsPluginInstanceOwner
destructor:

http://hg.mozilla.org/mozilla-central/annotate/2f9f6cb3a0ef/layout/generic/nsObjectFrame.cpp#l2886

Which means that nsNPAPIPluginInstance::mOwner doesn't get nulled out
when the object to which mOwner is a weak link gets destroyed.  Which
leads directly to this bug's crash.

The fix is to also call nsIPluginInstance::InvalidateOwner() from
nsPluginInstanceOwner::SetInstance() when the value of mInstance
changes from non-null to null.

I got rid of the assertion -- since what happens in
nsPluginHost::TrySetupPluginInstance() is a case of
nsPluginInstanceOwner::SetInstance() legitimately getting called twice
on the same object.

Note that with this fix, the Silverlight plugin at
http://blogs.msdn.com/b/ie/ usually fails to load -- but that's
another bug (and the consequence of nsIPluginInstance::Initialize()
doing an error return).
Comment 11 Steven Michaud 2010-11-22 19:31:27 PST
Created attachment 492564 [details] [review]
Fix rev1 (restore assertion and change its message)

> I got rid of the assertion -- since what happens in
> nsPluginHost::TrySetupPluginInstance() is a case of
> nsPluginInstanceOwner::SetInstance() legitimately getting called
> twice on the same object.

Actually the assertion makes sense, but its message needs to be changed.
Comment 12 Josh Aas (Mozilla Corporation) 2010-11-22 21:32:41 PST
(In reply to comment #10)
> Then in the next line (1367) nsIPluginInstance::Initialize() is
> called.  But for some reason this call fails and
> nsIPluginInstanceOwner::SetInstance() gets called again with aInstance
> set to nsnull:

Silverlight is not compatible with our 64-bit builds on Mac OS X 10.6 because it can only use Carbon NPAPI. See bug 598223 about making such plugins fail to initialize. Perhaps this is the reason "nsIPluginInstance::Initialize" is failing.
Comment 13 Josh Aas (Mozilla Corporation) 2010-11-22 21:50:56 PST
Comment on attachment 492564 [details] [review]
Fix rev1 (restore assertion and change its message)

Looks good, thanks! I don't think the assertion is particularly helpful, maybe just drop it altogether.

We don't have the Silverlight cause for this crash on the branches, but do we have this same owner invalidation issue?
Comment 14 Steven Michaud 2010-11-23 08:27:46 PST
Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/832e6220498a

> Looks good, thanks!

You're welcome.

> I don't think the assertion is particularly helpful, maybe just drop
> it altogether.

I decided to keep it.  I'm not a big fan of assertions (or of debug
builds).  But if someone takes it into their head to call
nsPluginInstanceOwner::SetInstance(nsIPluginInstance *aInstance) with
aInstance and the existing mInstance both non-null, this bug's crashes
will start happening again.  The assertion at least helps to ward this
off.
Comment 15 Steven Michaud 2010-11-23 09:35:47 PST
(In reply to comment #13)

> We don't have the Silverlight cause for this crash on the branches,
> but do we have this same owner invalidation issue?

Yes, we do:

http://hg.mozilla.org/releases/mozilla-1.9.2/annotate/aa9371c6c23f/modules/plugin/base/src/nsPluginHost.cpp#l3610
http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/a0e34daf1661/modules/plugin/base/src/nsPluginHostImpl.cpp#l3869

I'll prepare patches for both branches.
Comment 16 Steven Michaud 2010-11-23 09:40:05 PST
Another problem:

nsDummyJavaPluginOwner::SetInstance() (in dom/base/nsGlobalWindow.cpp)
seems to need the same change as nsPluginInstanceOwner::SetInstance()
(in layout/generic/nsObjectFrame.cpp).

I'll prepare another trunk patch for that, and add it to my branch
patches.
Comment 17 Steven Michaud 2010-11-23 10:09:33 PST
(In reply to comment #12)

> Silverlight is not compatible with our 64-bit builds on Mac OS X
> 10.6 because it can only use Carbon NPAPI.

Oops, I'd forgotten about that.

> See bug 598223 about making such plugins fail to initialize. Perhaps
> this is the reason "nsIPluginInstance::Initialize" is failing.

Yes.  The call to nsNPAPIPluginInstance::Initialize() calls
InitializePlugin(), which fails at the following line (when
PluginModuleParent::NPP_New() makes an error return):

http://hg.mozilla.org/mozilla-central/annotate/29323301bb17/modules/plugin/base/src/nsNPAPIPluginInstance.cpp#l415

This must be because of the following code added by your patch for bug
598223:

http://hg.mozilla.org/mozilla-central/annotate/29323301bb17/dom/plugins/PluginModuleChild.cpp#l1774
Comment 18 Steven Michaud 2010-11-23 10:50:06 PST
Created attachment 492707 [details] [review]
Additional trunk patch to deal with nsDummyJavaPluginOwner
Comment 19 Steven Michaud 2010-11-23 12:47:01 PST
Comment on attachment 492707 [details] [review]
Additional trunk patch to deal with nsDummyJavaPluginOwner

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/43a10e7fbef3
Comment 20 Steven Michaud 2010-11-23 13:20:55 PST
Created attachment 492770 [details] [review]
1.9.2-branch patch
Comment 21 Steven Michaud 2010-11-23 13:53:58 PST
Created attachment 492785 [details] [review]
1.9.1-branch patch
Comment 23 Stephen Donner [:stephend] 2010-11-24 14:15:45 PST
(Another is to load http://wwwns.akamai.com/hdnetwork/demo/flash/default.html and let Flash initialize, then click on the Silverlight logo to the right of the mobile phone.)
Comment 24 Stephen Donner [:stephend] 2010-11-25 10:42:40 PST
Did this bug cause 614812?
Comment 25 Steven Michaud 2010-11-26 07:57:41 PST
> Did this bug cause 614812?

No. 

Before this bug's patch landed, the STR at bug 614812 would have
caused this bug's crash.  Now the Silverlight plugin just fails to
load, thanks to what Josh pointed out in comment #12:

> Silverlight is not compatible with our 64-bit builds on Mac OS X
> 10.6 because it can only use Carbon NPAPI. See bug 598223 about
> making such plugins fail to initialize.

Silverlight plugins should work in 32-bit mode, though.
Comment 26 Daniel Veditz 2010-11-29 10:46:34 PST
Is there a branch testcase? If we take this patch on the branches how would QA verify the fix?
Comment 27 Steven Michaud 2010-11-29 11:04:21 PST
> Is there a branch testcase?

No.

> If we take this patch on the branches how would QA verify the fix?

Not that I'm aware of.

But the patch is very simple and straightforward.  And comment #15
shows that it's clearly needed on the branches.
Comment 28 Steven Michaud 2010-11-29 11:24:06 PST
>> If we take this patch on the branches how would QA verify the fix?
>
> Not that I'm aware of.

I'm not aware of any way QA could verify the fix on the branches.

> But the patch is very simple and straightforward.  And comment #15
> shows that it's clearly needed on the branches.

Comment #15 shows that, on both branches,
nsIPluginInstanceOwner::SetInstance() can be called twice on the same
object, first with aInstance set to a non-null value, and then with
aInstance set to nsnull.

Without my patch, doing this will cause this bug's crash, even on the
branches.
Comment 29 Daniel Veditz 2010-12-01 10:16:52 PST
Comment on attachment 492770 [details] [review]
1.9.2-branch patch

Approved for 1.9.2.14, a=dveditz for release-drivers
Comment 30 Daniel Veditz 2010-12-01 10:17:34 PST
Comment on attachment 492785 [details] [review]
1.9.1-branch patch

Approved for 1.9.1.16, a=dveditz for release-drivers
Comment 31 Christian Legnitto [:LegNeato] 2010-12-01 10:18:35 PST
We wouldn't block a security release on this, so marking it as wanted rather
than blocking and approving the patches.
Comment 32 Steven Michaud 2010-12-02 09:24:24 PST
Comment on attachment 492770 [details] [review]
1.9.2-branch patch

Landed on the 1.9.2 branch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0e09c9543abe
Comment 33 Steven Michaud 2010-12-02 09:32:14 PST
Comment on attachment 492785 [details] [review]
1.9.1-branch patch

Landed on the 1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fa3bf9f20cf5

This was approved for the 1.9.1.16 branch ... but was that a mistake?  Should it have been approved for the 1.9.1.17 branch instead?  If not, do I also need to land it on the GECKO19116_20101122_RELBRANCH?
Comment 34 Christian Legnitto [:LegNeato] 2010-12-02 15:15:20 PST
Sorry, it should have been .17. Simple typo, everything is in the correct place. I'll fix the flag.
Comment 35 Al Billings [:abillings] 2011-01-04 16:11:39 PST
I can't reproduce this on OS X 10.6 using 1.9.2.13 or 1.9.1.16 with the urls in comment 22 and 23 (or the IE Blog). We've seen this reproduce on 1.9.2 and 1.9.1?
Comment 36 Steven Michaud 2011-01-04 17:35:39 PST
This bug's crash wasn't reproducible on the branches (only on the trunk).  The reason is that we don't try to run the Silverlight plugin out-of-process except on the trunk, and then only in 64-bit mode.
Comment 37 Al Billings [:abillings] 2011-01-05 10:58:36 PST
Marking this as nothing to do (NTD) for QA then since we cannot verify.

Note You need to log in before you can comment on or make changes to this bug.