723520 - Crash @ nsPluginHost::InstantiateEmbeddedPlugin with Flashblock

Status: RESOLVED WORKSFORME

(Core Graveyard :: Plug-ins, defect)

Description

[Scoobidiver (away)]

It's currently #26 top crasher in the first build of 13.0a1.
The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3f26b7bee352&tochange=e18c7bc2c28e
It's likely caused by bug 90268.

Signature 	nsPluginHost::InstantiateEmbeddedPlugin More Reports Search
UUID	18689483-b5ba-4562-9115-63abf2120202
Date Processed	2012-02-02 14:17:15
Uptime	189
Last Crash	10.1 weeks before submission
Install Age	3.1 minutes since version was first installed.
Install Time	2012-02-02 14:13:59
Product	Firefox
Version	13.0a1
Build ID	20120201031146
Release Channel	nightly
OS	Mac OS X
OS Version	10.7.3 11D50
Build Architecture	amd64
Build Architecture Info	family 6 model 42 stepping 7
Crash Reason	EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash Address	0xd0
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x6741GL Context? GL Context+
GL Layers? GL Layers+
Processor Notes 	This dump is too long and has triggered the automatic truncation routine
EMCheckCompatibility	False

Frame 	Module 	Signature 	Source
0 	XUL 	nsPluginHost::InstantiateEmbeddedPlugin 	dom/plugins/base/nsPluginHost.cpp:1116
1 	XUL 	nsObjectLoadingContent::InstantiatePluginInstance 	content/base/src/nsObjectLoadingContent.cpp:637
2 	XUL 	nsObjectLoadingContent::SyncStartPluginInstance 	content/base/src/nsObjectLoadingContent.cpp:1958
3 	XUL 	nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe 	dom/base/nsDOMClassInfo.cpp:9590
4 	XUL 	nsHTMLPluginObjElementSH::SetupProtoChain 	dom/base/nsDOMClassInfo.cpp:9657
5 	XUL 	nsHTMLPluginObjElementSH::PostCreate 	dom/base/nsDOMClassInfo.cpp:9757
6 	XUL 	FinishCreate 	js/xpconnect/src/XPCWrappedNative.cpp:645
7 	XUL 	XPCWrappedNative::GetNewOrUsed 	js/xpconnect/src/XPCWrappedNative.cpp:583
8 	XUL 	XPCConvert::NativeInterface2JSObject 	js/xpconnect/src/XPCConvert.cpp:1056
9 	XUL 	xpc_qsXPCOMObjectToJsval 	js/xpconnect/src/XPCQuickStubs.cpp:1086
10 	XUL 	nsIDOMDocument_GetElementById 	obj-firefox/x86_64/js/xpconnect/src/dom_quickstubs.cpp:3192
11 	XUL 	js::InvokeKernel 	js/src/jscntxtinlines.h:311
12 	XUL 	js::Interpret 	js/src/jsinterp.cpp:2801
13 	XUL 	UncachedInlineCall 	js/src/methodjit/InvokeHelpers.cpp:389
14 	XUL 	js::mjit::stubs::UncachedCallHelper 	js/src/methodjit/InvokeHelpers.cpp:472
15 	XUL 	CallCompiler::update 	js/src/methodjit/MonoIC.cpp:960
16 	XUL 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:1018
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsPluginHost%3A%3AInstantiateEmbeddedPlugin

Comment 2

[Scoobidiver (away)]

I checked manually crash reports and they all have the Flashblock extension.
It might be related to bug 723473, bug 723476 and bug 519752.

Comment 3

[Justin Dolske [:Dolske]]

I can reliably reproduce this (well, at least twice in a row)...

1) Go to https://www.amazon.com/yourdigitalitems/ref=topnav_ydi
2) Click "Your Kindle Orders"
3) boom.

(I am indeed using Flashblock)

Comment 4

[Steven Michaud [:smichaud] (Retired)]

There are a disproportionate number of these crashes on Mac trunk, where it's currently the #2 topcrasher.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #3)

Justin, could you post your Breakpad IDs?

Comment 6

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #3)

So can I!

And no, you don't need an Amazon account, or a Kindle.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #6)

And you do need to have FlashBlock installed.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Attachment: Gdb crash stack (today's trunk code on OS X 10.6.8)

Comment 9

[Sheila Mooney]

Steven, is this reproducible. If so, can I add the keyword?

Comment 10

[(not currently active) Ted Mielczarek]

I suspect this signature is also the same:
https://crash-stats.mozilla.com/report/index/bp-d8110f23-c619-4cc3-821b-2ae5e2120208
[@ nsPluginHost::InstantiateFullPagePlugin ] 

Since I've been crashing a lot with Flashblock installed lately, and it sounds virtually identical to the signature here.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Yes, this is 100% reproducible.  Here's STR (gathered from various
comments and my	own experience):

1) If you don't	already	have FlashBlock	installed, run FF 10.0 and
   install it.

   https://addons.mozilla.org/en-US/firefox/addon/flashblock/

   FlashBlock won't install in current trunk nightlies.  But it runs
   fine there, since current code now assumes that all extensions are
   compatible.

2) Run a current trunk (mozilla-central) nightly.

3) Visit https://www.amazon.com/yourdigitalitems/ref=topnav_ydi.

4) Click "Your Kindle Orders".

Comment 12

[Steven Michaud [:smichaud] (Retired)]

> bp-d8110f23-c619-4cc3-821b-2ae5e2120208

This crash stack is very fragmentary, so it's hard to tell if it's related.

But I hope to soon have a fix for this bug, and we can see if your crashes go away :-)

Comment 13

[Philip Chee]

Hi Josh, I'm the current Flashblock developer. Ted was the previous Flashblock developer so he might have some insights into this as well.

Is this OS X specific? I build SeaMonkey trunk daily on Win7 and I can't get Flashblock to crash following your STR:

> I have been reproducing with Flashblock installed on a nightly build by visiting this
> site:

> https://www.amazon.com/yourdigitalitems/ref=topnav_ydi

> Then click on "Your Kindle Orders". Here are two logging sessions that show
> Flashblock apparently having trouble resolving a method and changing the fundamental
> event chain. Can you tell me what methods Flashblock is failing to resolve?

We use Components.lookupMethod to get the native method for
1. createElementNS
https://addons.mozilla.org/en-US/firefox/files/browse/126475/file/chrome/flashblock.jar/content/flashblock/flashblock.xml#L27
2. setTimeout
https://addons.mozilla.org/en-US/firefox/files/browse/126475/file/chrome/flashblock.jar/content/flashblock/flashblock.xml#L291

This is a defensive measure because we have come across some websites redefine these methods for their own purposes perhaps in an attempt at disabling GreaseMonkey scripts.

> WARNING: Components.lookupMethod deprecated, use Components.utils.lookupMethod: file /Volumes/Mozilla/10_7_build/ff_trunk_debug_x86-64/js/xpconnect/src/XPCComponents.cpp, line 4251

As far as I can tell this is only a warning so we do actually succeed in resolving the methods.

Flashblock works by removing the object or embed from the DOM while storing a reference to it. We replace it with a simple html DIV placeholder. When the user clicks on our placeholder we swap the Flash object back in.

> Program received signal EXC_BAD_ACCESS, Could not access memory.
> Reason: 13 at address: 0x0000000000000000
> 0x0000000102d56994 in nsPluginHost::InstantiateEmbeddedPlugin (this=0x12b2aca10, aMimeType=0x12b3961c8 "text/html", aURL=0x1360614f0, aContent=0x1360df3f8, aOwner=0x1360df4b0) at /Volumes/Mozilla/10_7_build/ff_trunk_debug_x86-64/dom/plugins/base/nsPluginHost.cpp:982
> 982            aURL->GetAsciiSpec(urlSpec);

Ah shouldn't the mime type be "application/x-shockwave-flash"? Perhaps at this point Flashblock has swapped the Flash with a html DIV, causing InstantiateEmbeddedPlugin to fall over?

In Bug 723473 Comment 1:

> It's not clear to me whether anything under nsPluginStreamListenerPeer::OnStartRequest
> could end up clearing either of those two members and causing the destruction of the
> nsPluginStreamListenerPeer.

Would removing the Flash object from the DOM do this?

Comment 14

[Josh Aas]

(In reply to Philip Chee from comment #13)

> Is this OS X specific? I build SeaMonkey trunk daily on Win7 and I can't get
> Flashblock to crash following your STR:

It crashes reliably on Mac OS X. I haven't been able to see the crash on Windows. I can't say 100% for sure, but I think this is specific to Mac OS X.

> Flashblock works by removing the object or embed from the DOM while storing
> a reference to it. We replace it with a simple html DIV placeholder. When
> the user clicks on our placeholder we swap the Flash object back in.

Interesting, I didn't know that. I'll look into it.

Thanks.

Comment 15

[Andrew McCreight [:mccr8]]

I get this pretty reliably with Flashblock enabled at this URL: http://www.vg247.com/2012/01/29/racial-slur-slips-into-minecraft-translation-project/
Just go to the URL and it crashes for me.  This is on OSX.
https://crash-stats.mozilla.com/report/index/bp-8e1e9f5f-711f-4a81-8459-4f2132120208
https://crash-stats.mozilla.com/report/index/bp-a5bcac32-aa4f-4abd-9fee-fb0872120208
https://crash-stats.mozilla.com/report/index/bp-36386341-fc25-4ba7-8ff4-5c9432120208
https://crash-stats.mozilla.com/report/index/bp-45e2dd07-0f04-43b5-b965-b8a372120208
Three different top frames on the stack.  After I disabled Flashblock, I was able to visit the URL without crashing.

Comment 16

[(not currently active) Ted Mielczarek]

Interestingly, I have a reproducible crash in bug 723473 on Windows, but reproducing it in a debug build seems to crash with this stack instead.

Comment 17

[Benjamin Smedberg]

Yeah, being a memory-corruption bug we're going to see different stack traces. I think the patch from bug 723473 will have fixed this in tomorrow's nightly and perhaps a set of other bugs as well.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

What causes this bug's crashes is that, since Josh's patch for bug
90268 landed, a plugin's nsObjectLoadingContent object can be
"refetched" (in	LoadObject()) with new content before a plugin has
finished loading (in InstantiatePluginInstance()).  This invalidates
the aURI parameter in the middle of a call to
InstantiatePluginInstance(), at the following line:

http://hg.mozilla.org/mozilla-central/annotate/fb81c9a433e4/content/base/src/nsObjectLoadingContent.cpp#l607

When LoadObject() refetches content (old content or new content), the
previous value of mURI is released at the following line, and may be
deleted:

http://hg.mozilla.org/mozilla-central/annotate/fb81c9a433e4/content/base/src/nsObjectLoadingContent.cpp#l1219

My solution to this problem is to detect when an
nsObjectLoadingContent object has been refetched during a call to
InstantiatePluginInstance(), and to abort the attempt to load the
"old" content's plugin.

I figure this is an error of some sort, so I've chosen to return an
error value.  But maybe it's just as well to return NS_OK.

My patch removes Bsmedberg's bandaid patch for bug 723473.  This isn't
really necessary, but I thought it'd be best to test what happens
without that patch.

I've started tryserver builds, which should be available by tomorrow
morning.

Comment 19

[Josh Aas]

Thanks Steven! I'd like to review any final patch, after bsmedberg.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Here are the tryserver builds made with my patch from comment #18:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-292c77354647/

There were no non-spurious test failures.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #13)

> Flashblock works by removing the object or embed from the DOM while
> storing a reference to it. We replace it with a simple html DIV
> placeholder. When the user clicks on our placeholder we swap the
> Flash object back in.

I'd bet this is what triggered this bug's crashes.

Philip, you probably should test Flashblock with both Bsmedberg's
patch (in current trunk nightlies) and my patch (in tryserver builds
from comment #20), just to be sure there aren't any surprises.  I
suspect the "original" Flash object never finishes loading with either
patch.  I don't know if this will cause trouble for Flashblock.

Comment 22

[Philip Chee]

Steven, I could never reproduce the crashes because I'm on Windows. I understand the vast majority or crashes were on OS X and Linux.

> I suspect the "original" Flash object never finishes loading with either patch.

FWIW We have been using setTimeout since before v1.0 because Flashblock would reliably crash Firefox if we swapped the Flash object out as soon as it appears in the DOM. We may now be finally able to remove the timeout if you're correct.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

> We may now be finally able to remove the timeout if you're correct.

Please try that and let us know your results here.  Please also then (with the timeout removed) run some general tests (on several platforms, if possible), to see if my patch or Bsmedberg's has any side effects.  I don't think either patch will, but it's better to be safe than sorry.

Comment 24

[Scoobidiver (away)]

It's already fixed by bug 723473 as there have been no crashes in 13.0a1/20120209 and above for the four crash signatures.
Is the patch useful? If yes, maybe it needs to be in a separate bug.

Comment 25

[Benjamin Smedberg]

Comment on attachment 595896 [details] [diff] [review]
Fix

Steven, can you explain why this patch is preferable to the current solution? Are there other invariants you're trying to preserve between nsObjectLoadingContent::InstantiatePlugin and nsObjectLoadingContent::LoadObject? If there are important other reasons, are they visible in a way that we could test? Clearing review for now, please remark review with comments if appropriate.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #25)

The reason I prefer my patch is that it	tackles the problem at a
deeper level than your patch.

As best	I can tell, the	only way this bug's crashes can	happen is
because of the sequence	of events I describe in	comment	#18.  The same
is true of bug 723473's	crashes, and likely also of the	problems
described in several other bugs.

Aside from this one case, there's no need for your patch.  My patch
deals with it more directly.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

But, though my patch is better, it doesn't seem to be the final answer -- we still have bug 726734.

Comment 28

[Josh Aas]

Comment on attachment 595896 [details] [diff] [review]
Fix

Review of attachment 595896 [details] [diff] [review]:
-----------------------------------------------------------------

While this is more targeted, I'm not convinced it is better simply because it adds unnecessary complexity. I'd prefer not to do this.

Your patch aborts a load immediately after flushing layout changes the URL or MIME for plugin content. This requires tracking whether or not such a change happened, which is the added complexity.

There is nothing wrong with letting the load complete for a given URI and MIME type, it will get cleaned up properly and a new instance will be started if necessary. nsObjectLoadingContent::LoadObject won't cause synchronous instance loading to attempt to re-enter either, since it only does async loads.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +551,5 @@
>    , mSuppressed(false)
>    , mNetworkCreated(true)
>    // If plugins.click_to_play is false, plugins should always play
>    , mShouldPlay(!mozilla::Preferences::GetBool("plugins.click_to_play", false))
> +  , mRefetched(false)

I don't think this is a good name for the variable. This is tracking whether or not something asked the object to reload. I'd expect a name like "mReloadRequested".

Comment 29

[Josh Aas]

(In reply to Josh Aas (Mozilla Corporation) from comment #28)

> While this is more targeted, I'm not convinced it is better simply because
> it adds unnecessary complexity. I'd prefer not to do this.

Thank you, though, for the patch. At a minimum it helps us to understand the problem better.

Comment 30

[Kyle Wooten]

Thanks!

Comment 31

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm marking this bug as WORKSFORME as bug crashlog signature didn't appear from a long time (over half year) [except some obsolete <28 versions, no crashes starting since 28 version].