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+.

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.

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.

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.

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).

> 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.

(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?

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.

(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

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

(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.

Comment on attachment 492770 [details] [diff] [review] 1.9.2-branch patch Approved for 1.9.2.14, a=dveditz for release-drivers

Comment on attachment 492785 [details] [diff] [review] 1.9.1-branch patch Approved for 1.9.1.16, a=dveditz for release-drivers

We wouldn't block a security release on this, so marking it as wanted rather than blocking and approving the patches.

Comment on attachment 492770 [details] [diff] [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 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.

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?

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.