Add plugin support and prevent crash [@ libflashplayer.so@0x32884 ][@ libflashplayer.so@0x32800 ] in Fennec

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dholbert, Unassigned)

Tracking

({crash, crashreportid, topcrash})

Trunk
ARM
Maemo
Points:
---

Firefox Tracking Flags

(fennec-)

Details

(crash signature, )

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

9 years ago
STEPS TO REPRODUCE:
On a Nokia n900 with flashplayer installed:
 1. Set pref "plugin.disable" to false in fennec, & restart
 2. Visit http://listen.grooveshark.com

ACTUAL RESULTS: Crash [@ libflashplayer.so@0x32884 ] before the page finishes loading.
Crash reports:
 bp-b8517a21-8ed7-4840-8df9-cf8d22101207
 bp-90e3fdb8-ea25-49a0-ac20-4493c2101207
Reporter

Comment 1

9 years ago
(See also Bug 617718, which is for a different crash slightly later on the same page, when you've set one additional about:config pref.  In that bug, the page actually loads all the way before crashing, usually -- whereas here, it does not.)
Reporter

Updated

9 years ago
OS: Linux → Maemo
Hardware: x86_64 → ARM
It is #4 top crasher in Fennec 4.0b3 for the last week.
It is specific to Maemo.

Signature	libflashplayer.so@0x32884
UUID	53ed2230-47f9-4f9c-8326-a7b172110103
Time 	2011-01-03 18:43:41.282978
Uptime	0
Install Age	2755 seconds (45.9 minutes) since version was first installed.
Product	Fennec
Version	4.0b3
Build ID	20101221194929
Branch	1.9
OS	Linux
OS Version	0.0.0 Linux 2.6.28.10power40-wl1 #1 PREEMPT Thu Aug 26 15:44:10 CEST 2010 armv7l
CPU	arm
Crash Reason	SIGSEGV
Crash Address	0x84

Frame 	Module 	Signature [Expand] 	Source
0 	libflashplayer.so 	libflashplayer.so@0x32884 	
1 	libflashplayer.so 	libflashplayer.so@0x280cb 	
2 	libnspr4.so 	_PR_UNIX_GetInterval 	nsprpub/pr/src/md/unix/unix.c:3032
3 	libflashplayer.so 	libflashplayer.so@0x2c8a7 	
4 	libxul.so 	nsNPAPIPluginInstance::SetWindow 	modules/plugin/base/src/nsNPAPIPluginInstance.cpp:469
5 	libxul.so 	nsPluginNativeWindowGtk2::CallSetWindow 	modules/plugin/base/src/nsPluginNativeWindowGtk2.cpp:233
6 	libxul.so 	nsObjectFrame::CallSetWindow 	layout/generic/nsObjectFrame.cpp:1141
7 	libxul.so 	nsPluginInstanceOwner::SetWindow 	layout/generic/nsObjectFrame.cpp:3292
8 	libxul.so 	nsPluginHost::DoInstantiateEmbeddedPlugin 	modules/plugin/base/src/nsPluginHost.cpp:1105
9 	libxul.so 	nsPluginHost::InstantiateEmbeddedPlugin 	modules/plugin/base/src/nsPluginHost.cpp:966
10 	libxul.so 	nsObjectFrame::InstantiatePlugin 	layout/generic/nsObjectFrame.cpp:1041
11 	libxul.so 	nsObjectFrame::Instantiate 	layout/generic/nsObjectFrame.cpp:2348
12 	libxul.so 	nsObjectLoadingContent::Instantiate 	content/base/src/nsObjectLoadingContent.cpp:1895
13 	libxul.so 	nsObjectLoadingContent::EnsureInstantiation 	content/base/src/nsObjectLoadingContent.cpp:919
14 	libxul.so 	nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe 	dom/base/nsDOMClassInfo.cpp:9452
15 	libxul.so 	nsHTMLPluginObjElementSH::SetupProtoChain 	dom/base/nsDOMClassInfo.cpp:9532
16 	libxul.so 	nsHTMLPluginObjElementSH::PostCreate 	dom/base/nsDOMClassInfo.cpp:9645
...

More reports at:
http://crash-stats.mozilla.com/report/list?range_value=4&range_unit=weeks&signature=libflashplayer.so%400x32884
blocking2.0: --- → ?
Component: General → Plug-ins
Product: Fennec → Core
QA Contact: general → plugins
Summary: Fennec crash [@ libflashplayer.so@0x32884 ] when loading grooveshark.com with plugin.disable=false → Fennec crash [@ libflashplayer.so@0x32884 ] on Maemo

Updated

9 years ago
Keywords: topcrash
we don't support flash and our current support is buggy. I think we want to remove this pref, or just stop shipping npapi support, or mark wontfix.

Comment 4

9 years ago
in theory someone @nokia should be able to take the .dump and analyze it w/ symbols for flash. would that be of use?
Reporter

Comment 5

9 years ago
(In reply to comment #3)
> I think we want to
> remove this pref, or just stop shipping npapi support

Just to be clear -- both of those would mean that flash (youtube, pandora, flash games, etc) would no longer work in Fennec-on-maemo, right?  (I don't know how many flash-based sites work right now, but IIRC at least youtube works.)
dholbert:

yes, you are right.  flash would not work in Fennec (maemo or android).  1.0 didn't support flash, and barely supported YouTube (through an enabler addon).

timeless:

yes, it would.
Daniel in order to make flash working on maemo5 you should set:
pref("plugin.disable", false);
pref("dom.ipc.plugins.enabled", true);

otherwise plugin trying to work in the same process and there are some problem with rendering type detection or something like that...

But in general case flash should work mostly good in fennec with those prefs enabled
Reporter

Comment 8

9 years ago
(In reply to comment #7)
> Daniel in order to make flash working on maemo5 you should set:
> pref("plugin.disable", false);
> pref("dom.ipc.plugins.enabled", true);

I know -- when I reported this bug, I *also* crashed if I had both of those prefs toggled (though in a different way).  So I filed this bug on the one-pref crash, and bug 617718 on the two-pref crash.

The two-pref crash (bug 617718) is now fixed, though, and it sounds like the one-pref crash is Flash's fault / something we kinda have to live with.
We could probably take a look at flash internals and figure out why flash crashes here..., but one pref is not really nice option here, because it bring slow/unstable flash stuff into browser content process :(
With this patch single "plugin.disable" will works on maemo.
Attachment #502003 - Flags: review?(doug.turner)
For making in-process plugin working we should probably apply this patch:
https://bugzilla.mozilla.org/attachment.cgi?id=450582&action=edit
Ok, seems I've found real reason for this bug.
When dom.ipc.plugins == false we do rendering with normal X HandleEvent... and problem is that flash does not support rendering into XDrawable which has visual != default System visual.
as result it crashes in 
*********************************
#8  0x412329a4 in nsNPAPIPluginInstance::HandleEvent (this=0x48444880, event=0xbed85300, result=0x0)
    at modules/plugin/base/src/nsNPAPIPluginInstance.cpp:583
#9  0x407b68a4 in nsPluginInstanceOwner::Renderer::DrawWithXlib (this=<value optimized out>, xsurface=0x494d2bb0, offset=..., clipRects=0x0, numClipRects=0)
    at layout/generic/nsObjectFrame.cpp:5367
#10 0x4171ff14 in gfxXlibNativeRenderer::DrawOntoTempSurface (this=0xbed85644, tempXlibSurface=0x494d2bb0, offset=...)
    at gfx/thebes/gfxXlibNativeRenderer.cpp:456
#11 0x417209e8 in gfxXlibNativeRenderer::Draw (this=0x43876ae0, ctx=0x0, size=..., flags=<value optimized out>, screen=0x4383c2e0, visual=0x43855860, result=0x0)
    at gfx/thebes/gfxXlibNativeRenderer.cpp:579
#12 0x407ba7b0 in nsPluginInstanceOwner::Paint (this=0x481fa280, aContext=0x4842da80, aFrameRect=..., aDirtyRect=...)
    at layout/generic/nsObjectFrame.cpp:5243
#13 0x407bf7e4 in nsObjectFrame::PaintPlugin (this=0x494999d8, aBuilder=<value optimized out>, aRenderingContext=..., aDirtyRect=..., aPluginRect=...)
    at layout/generic/nsObjectFrame.cpp:1951
#14 0x407bf874 in nsDisplayPlugin::Paint (this=<value optimized out>, aBuilder=0xbed86038, aCtx=0x494f1240) at layout/generic/nsObjectFrame.cpp:1238
#15 0x4069ee94 in mozilla::FrameLayerBuilder::DrawThebesLayer (aLayer=<value optimized out>, aContext=0x4842da80, aRegionToDraw=..., aRegionToInvalidate=..., aCallbackData=0xbed86038)
    at layout/base/FrameLayerBuilder.cpp:1757
*********************************

Where Temp surface seems like == 24 bpp surface.

I'm suggesting now enable ipc plugins rendering path by default, and possibly fix problem with wrong temp surface visual later... also we want temp X surface, we do want layer rendering enabled and bug 596379 fixed
This fixes an error introduced in bug 576143.
Attachment #502386 - Flags: review?(roc)
Comment on attachment 502386 [details] [diff] [review]
use target visual to determine target format

Could we extend the text plugin to exercise nondefault visuals?
Attachment #502386 - Flags: review?(roc) → review+
Corrected the comment.
Attachment #502387 - Attachment is obsolete: true
Attachment #502388 - Flags: review?(romaxa)
Attachment #502387 - Flags: review?(romaxa)
(In reply to comment #15)

The test plugin does use non-default visuals, which would be used in tests that renderer to color-alpha layers, and I assume we already have such tests.

Attachment 502386 [details] [diff] is attached to this bug because it removes an incorrect use of the visual passed to Draw(), and attachment 502387 [details] [diff] [review] changes that visual on the assumption that it's main purpose is for !DRAW_SUPPORTS_ALTERNATE_VISUAL.
Attachment #502388 - Attachment is patch: true
Attachment #502388 - Attachment mime type: application/octet-stream → text/plain
Posted patch Right fix for this bug. (obsolete) — Splinter Review
Seems I found another problem, flash is expecting ws_info->display be != null, but I found that after recent refactoring flash never get any SetWindow call with valid ws_info->display value... that is why it crash for me on Qt...  need to check how it works with this patch on Gtk.
Attachment #502482 - Flags: review?(karlt)
Comment on attachment 502388 [details] [diff] [review]
use system visual for layerless windowless Flash

yep, this is more compatible with flash.
Attachment #502388 - Flags: review?(romaxa) → review+
Attachment #502003 - Flags: review?(doug.turner) → review+
Now I'm not sure whether attachment 502388 [details] [diff] [review] is an appropriate fix for this bug.
The issue reported in comment 12 caused a crash in HandleEvent.  Was this a SIGSEGV?  How would an unexpected visual cause a SIGSEGV?

The reports in comment 0 are no longer available but comment 2 describes a SEGV in SetWindow.

(In reply to comment #18)
> Created attachment 502482 [details] [diff] [review]
> Right fix for this bug.
> 
> Seems I found another problem, flash is expecting ws_info->display be != null,

Yes, display should not be NULL.

> but I found that after recent refactoring flash never get any SetWindow call
> with valid ws_info->display value... that is why it crash for me on Qt...  need
> to check how it works with this patch on Gtk.

Did it get a SetWindow call with an invalid (NULL) value?

The way it's meant to work is that ws_info->display is set here:
http://hg.mozilla.org/mozilla-central/annotate/0474f6b72e6e/layout/generic/nsObjectFrame.cpp#l6292

Can you explain why that is not sufficient?
> The way it's meant to work is that ws_info->display is set here:
> http://hg.mozilla.org/mozilla-central/annotate/0474f6b72e6e/layout/generic/nsObjectFrame.cpp#l6292
> 
> Can you explain why that is not sufficient?
Because in content process we have puppet widget and Puppet Widget don't care about NativeData
http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/PuppetWidget.h#140
Also I don't see certain guarantee that GetNativeData always return NON-NULL display in nsPluginInstanceOwner::CreateWidget

Updated

9 years ago
blocking2.0: ? → ---
tracking-fennec: --- → ?
Summary: Fennec crash [@ libflashplayer.so@0x32884 ] on Maemo → Add plugin support and prevent crash [@ libflashplayer.so@0x32884 ] in Fennec
(In reply to comment #21)
> Because in content process we have puppet widget and Puppet Widget don't care
> about NativeData
> http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/PuppetWidget.h#140

(In reply to comment #22)
> Also I don't see certain guarantee that GetNativeData always return NON-NULL
> display in nsPluginInstanceOwner::CreateWidget

Thanks.  That sounds like the bug here then.

Often the plugin receives a SetWindow before a paint, so it's best to set the display member before waiting for a paint event.
Attachment #502884 - Flags: review?(romaxa)
tracking-fennec: ? → 2.0-
Comment on attachment 502884 [details] [diff] [review]
don't rely in widgets to provide a display for ws_info

yeah, make sense.
Attachment #502884 - Flags: review?(romaxa) → review+
Comment on attachment 502884 [details] [diff] [review]
don't rely in widgets to provide a display for ws_info

I think we should land this patch, and it should not break anything.
Attachment #502884 - Flags: approval2.0?
Comment on attachment 502003 [details] [diff] [review]
Enable IPC plugins by default

Can we push it? this is the only way to get most supported plugins rendering path
Attachment #502003 - Flags: approval2.0?

Updated

9 years ago
Summary: Add plugin support and prevent crash [@ libflashplayer.so@0x32884 ] in Fennec → Add plugin support and prevent crash [@ libflashplayer.so@0x32884 ][@ libflashplayer.so@0x32800 ] in Fennec
Attachment #502003 - Flags: approval2.0? → approval2.0+
Attachment #502884 - Flags: approval2.0? → approval2.0+
Comment on attachment 502482 [details] [diff] [review]
Right fix for this bug.

I assume there is no need for this.
Attachment #502482 - Attachment is obsolete: true
Attachment #502482 - Flags: review?(karlt)
> I assume there is no need for this.
Yes, in case if we 100% sure that display setup did happen before. notsure but might be useful to have 
if (!ws_info->display) {
  ws_info->display = dpy...
}
in order to avoid future code conflicts
http://hg.mozilla.org/mobile-browser/rev/e00d4ad32c34
http://hg.mozilla.org/mozilla-central/rev/df7b4d535f4b
No crash anymore
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Filed bug 628190 for attachment 502386 [details] [diff] [review].
I don't know whether attachment 502388 [details] [diff] [review] is necessary or not.
(I failed to arrange a test desktop system setup with 16-bit default visual as well as 24-bit visuals).
I think we should be fine without attachment 502482 [details] [diff] [review].
Crash Signature: [@ libflashplayer.so@0x32884 ] [@ libflashplayer.so@0x32800 ]
You need to log in before you can comment on or make changes to this bug.