Closed
Bug 617703
Opened 12 years ago
Closed 12 years ago
Add plugin support and prevent crash [@ libflashplayer.so@0x32884 ][@ libflashplayer.so@0x32800 ] in Fennec
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(fennec-)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | - | --- |
People
(Reporter: dholbert, Unassigned)
References
()
Details
(Keywords: crash, crashreportid, topcrash)
Crash Data
Attachments
(4 files, 2 obsolete files)
282 bytes,
patch
|
dougt
:
review+
dougt
:
approval2.0+
|
Details | Diff | Splinter Review |
1.15 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
1.28 KB,
patch
|
romaxa
:
review+
|
Details | Diff | Splinter Review |
1.45 KB,
patch
|
romaxa
:
review+
dougt
:
approval2.0+
|
Details | Diff | Splinter Review |
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•12 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•12 years ago
|
OS: Linux → Maemo
Hardware: x86_64 → ARM
Comment 2•12 years ago
|
||
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
Comment 3•12 years ago
|
||
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.
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•12 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.)
Comment 6•12 years ago
|
||
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•12 years ago
|
||
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•12 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.
Comment 9•12 years ago
|
||
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•12 years ago
|
||
With this patch single "plugin.disable" will works on maemo.
Attachment #502003 -
Flags: review?(doug.turner)
Comment 11•12 years ago
|
||
For making in-process plugin working we should probably apply this patch: https://bugzilla.mozilla.org/attachment.cgi?id=450582&action=edit
Comment 12•12 years ago
|
||
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•12 years ago
|
||
This fixes an error introduced in bug 576143.
Attachment #502386 -
Flags: review?(roc)
Comment 14•12 years ago
|
||
Attachment #502387 -
Flags: review?(romaxa)
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+
Comment 16•12 years ago
|
||
Corrected the comment.
Attachment #502387 -
Attachment is obsolete: true
Attachment #502388 -
Flags: review?(romaxa)
Attachment #502387 -
Flags: review?(romaxa)
Comment 17•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #502388 -
Attachment is patch: true
Attachment #502388 -
Attachment mime type: application/octet-stream → text/plain
Comment 18•12 years ago
|
||
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 19•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #502003 -
Flags: review?(doug.turner) → review+
Comment 20•12 years ago
|
||
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•12 years ago
|
||
> 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•12 years ago
|
||
Also I don't see certain guarantee that GetNativeData always return NON-NULL display in nsPluginInstanceOwner::CreateWidget
Updated•12 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
Comment 23•12 years ago
|
||
(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)
Updated•12 years ago
|
tracking-fennec: ? → 2.0-
Comment 24•12 years ago
|
||
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 25•12 years ago
|
||
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 26•12 years ago
|
||
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•12 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
Updated•12 years ago
|
Attachment #502003 -
Flags: approval2.0? → approval2.0+
Updated•12 years ago
|
Attachment #502884 -
Flags: approval2.0? → approval2.0+
Comment 27•12 years ago
|
||
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)
Comment 28•12 years ago
|
||
> 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•12 years ago
|
||
http://hg.mozilla.org/mobile-browser/rev/e00d4ad32c34 http://hg.mozilla.org/mozilla-central/rev/df7b4d535f4b No crash anymore
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 30•12 years ago
|
||
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].
Assignee | ||
Updated•12 years ago
|
Crash Signature: [@ libflashplayer.so@0x32884 ]
[@ libflashplayer.so@0x32800 ]
Updated•9 months ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•