536199 - Crash Yahoo! frontpage with 12/21 1.9.2 nightly build

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091221 Firefox/3.6b5pre Fennec/1.0b6pre

Steps to Reproduce:
1. Enable Plugins
2. Go to www.yahoo.com
3. Wait for page load and for flash title and ad to start

Actual Results:
Fennec will crash

Expected Results:
Fennec shouldn't crash

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

this won't crash if you are on m.www.yahoo.com (auto redirect), but does on www.yahoo.com

Comment 2

[Stuart Parmenter]

This is a crash in libflashplayer.so

Comment 3

[Al Billings [:abillings - ex-MoCo]]

I can't get it to not redirect me to the mobile page.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

The crash is happening in NativeImageDraw when we set the window:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#5102

But only when NativeImageDraw is called from InvalidateRect:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#2733

Doug mentioned that we only really need to call SetWindow if the size of the plugin changes. Therefore, this patch does just that. The size only changes when SetAbsoluteScreenPosition is called, which then calls NativeImageDraw.

The patch skips the SetWindow call if "invalidRect" is null and "sizeChanged"

Theremight be an upside to skipping SetWindow whenever possible. Page loading and panning both seem faster with the patch.

A test build with this patch can be found here:
http://people.mozilla.com/~mfinkle/fennec/fennec-1.0b6pre.en-US.linux-gnueabi-arm-flashfix.tar

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 418795 [details] [diff] [review]
patch

can we move the | if (!invalidRect && sizeChanged) {
| block into SetupXShm?  We probably only need to do this when size changes.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #5)
> (From update of attachment 418795 [details] [diff] [review])
> can we move the | if (!invalidRect && sizeChanged) {
> | block into SetupXShm?  We probably only need to do this when size changes.

I don't follow. The SetupXShm() call has a sizeChanged check already, right?

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 418795 [details] [diff] [review]
patch

discussed on IRC.

This is probably a fine optimization, but we do not know if this actually fixes the crash.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed m-c:
http://hg.mozilla.org/mozilla-central/rev/8edc5071211f

pushed m-192:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e2797cbdc4c1