Closed Bug 613376 Opened 9 years ago Closed 9 years ago

[Mac, OOPP] Firefox 4.0b8pre crash in [@ nsNPAPIPluginInstance::GetOwner ], possibly only with Silverlight plugin

Categories

(Core :: Plug-ins, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+
blocking1.9.2 --- .14+
status1.9.2 --- .14-fixed
blocking1.9.1 --- .17+
status1.9.1 --- .17-fixed

People

(Reporter: marcia, Assigned: smichaud)

References

()

Details

(Keywords: crash, Whiteboard: [sg:critical? on trunk][qa-ntd-192] [qa-ntd-191])

Crash Data

Attachments

(4 files, 1 obsolete file)

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
Adding Josh since this looks similar to something mentioned earlier today.
Regression from beta 7, blocking beta8+.
Assignee: nobody → joshmoz
blocking2.0: --- → beta8+
Group: core-security
blocking1.9.2: --- → ?
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/
(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.
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.
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.
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.
Summary: [Mac] Firefox 4.0b8pre crash in [@ nsNPAPIPluginInstance::GetOwner ] → [Mac] Firefox 4.0b8pre crash in [@ nsNPAPIPluginInstance::GetOwner ], possibly only with Silverlight plugin
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.
Summary: [Mac] Firefox 4.0b8pre crash in [@ nsNPAPIPluginInstance::GetOwner ], possibly only with Silverlight plugin → [Mac, OOPP] Firefox 4.0b8pre crash in [@ nsNPAPIPluginInstance::GetOwner ], possibly only with Silverlight plugin
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.
Attached patch Fix (obsolete) — Splinter Review
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).
Assignee: joshmoz → smichaud
Status: NEW → ASSIGNED
Attachment #492551 - Flags: review?(joshmoz)
> 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.
Attachment #492551 - Attachment is obsolete: true
Attachment #492564 - Flags: review?(joshmoz)
Attachment #492551 - Flags: review?(joshmoz)
(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 on attachment 492564 [details] [diff] [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?
Attachment #492564 - Flags: review?(joshmoz) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(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.
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.
(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
Attachment #492707 - Flags: review?(joshmoz) → review+
Comment on attachment 492707 [details] [diff] [review]
Additional trunk patch to deal with nsDummyJavaPluginOwner

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/43a10e7fbef3
Attachment #492770 - Flags: review?(joshmoz)
Attachment #492785 - Flags: review?(joshmoz)
Attachment #492770 - Flags: review?(joshmoz) → review+
Attachment #492785 - Flags: review?(joshmoz) → review+
(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.)
Did this bug cause 614812?
> 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.
Is there a branch testcase? If we take this patch on the branches how would QA verify the fix?
> 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.
>> 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.
blocking1.9.1: --- → needed
blocking1.9.2: ? → needed
Comment on attachment 492770 [details] [diff] [review]
1.9.2-branch patch

Approved for 1.9.2.14, a=dveditz for release-drivers
Attachment #492770 - Flags: approval1.9.2.14+
Comment on attachment 492785 [details] [diff] [review]
1.9.1-branch patch

Approved for 1.9.1.16, a=dveditz for release-drivers
Attachment #492785 - Flags: approval1.9.1.16+
We wouldn't block a security release on this, so marking it as wanted rather
than blocking and approving the patches.
blocking1.9.1: needed → ---
blocking1.9.2: needed → ---
Comment on attachment 492785 [details] [diff] [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?
Sorry, it should have been .17. Simple typo, everything is in the correct place. I'll fix the flag.
Attachment #492785 - Flags: approval1.9.1.16+ → approval1.9.1.17+
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?
Whiteboard: [qa-examined-191] [qa-examined-192] [qa-needs-STR]
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.
Marking this as nothing to do (NTD) for QA then since we cannot verify.
Whiteboard: [qa-examined-191] [qa-examined-192] [qa-needs-STR] → [qa-ntd-192] [qa-ntd-191]
Whiteboard: [qa-ntd-192] [qa-ntd-191] → [sg:critical? on trunk][qa-ntd-192] [qa-ntd-191]
blocking1.9.1: --- → .17+
blocking1.9.2: --- → .14+
Group: core-security
Crash Signature: [@ nsNPAPIPluginInstance::GetOwner ]
You need to log in before you can comment on or make changes to this bug.