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

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Scoobidiver (away)]

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

Comment 3

[Doug Turner (:dougt)]

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

[timeless]

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

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Doug Turner (:dougt)]

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.

Comment 7

[Oleg Romashin (:romaxa)]

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

Comment 8

[Daniel Holbert [:dholbert]]

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

Comment 9

[Oleg Romashin (:romaxa)]

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

Comment 10

[Oleg Romashin (:romaxa)]

Attachment: Enable IPC plugins by default

With this patch single "plugin.disable" will works on maemo.

Comment 11

[Oleg Romashin (:romaxa)]

For making in-process plugin working we should probably apply this patch:
https://bugzilla.mozilla.org/attachment.cgi?id=450582&action=edit

Comment 12

[Oleg Romashin (:romaxa)]

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

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: use target visual to determine target format

This fixes an error introduced in bug 576143.

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: use system visual for layerless windowless Flash

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 502386 [details] [diff] [review]
use target visual to determine target format

Could we extend the text plugin to exercise nondefault visuals?

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: use system visual for layerless windowless Flash

Corrected the comment.

Comment 17

[Karl Tomlinson (:karlt)]

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

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: Right fix for this bug.

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.

Comment 19

[Oleg Romashin (:romaxa)]

Comment on attachment 502388 [details] [diff] [review]
use system visual for layerless windowless Flash

yep, this is more compatible with flash.

Comment 20

[Karl Tomlinson (:karlt)]

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?

Comment 21

[Oleg Romashin (:romaxa)]

> 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

Comment 22

[Oleg Romashin (:romaxa)]

Also I don't see certain guarantee that GetNativeData always return NON-NULL display in nsPluginInstanceOwner::CreateWidget

Comment 23

[Karl Tomlinson (:karlt)]

Attachment: don't rely in widgets to provide a display for ws_info

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

Comment 24

[Oleg Romashin (:romaxa)]

Comment on attachment 502884 [details] [diff] [review]
don't rely in widgets to provide a display for ws_info

yeah, make sense.

Comment 25

[Oleg Romashin (:romaxa)]

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.

Comment 26

[Oleg Romashin (:romaxa)]

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

Comment 27

[Karl Tomlinson (:karlt)]

Comment on attachment 502482 [details] [diff] [review]
Right fix for this bug.

I assume there is no need for this.

Comment 28

[Oleg Romashin (:romaxa)]

> 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

Comment 29

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mobile-browser/rev/e00d4ad32c34
http://hg.mozilla.org/mozilla-central/rev/df7b4d535f4b
No crash anymore

Comment 30

[Karl Tomlinson (:karlt)]

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