395983 - Duplicated Flash menus using r60(+) of Flash player

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Build identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007091204 Minefield/3.0a8pre

Summary: Duplicated Flash menus using r60(+) of Flash player

Steps to Reproduce:

1. Install r60 (or greater) of Flash Player Beta from http://labs.adobe.com/downloads/flashplayer9.html
2. Visit http://www.toyota.com, and mouse-over the "Cars" menu

Expected Results:

Flyout menu is generated

Actual Results:

We get doubled-up menus generated in Flash.

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

BTW; this does NOT happen on Windows Vista with r60.  I think it might be specific to Mac OS X (Cocoa events?)

Comment 2

[Kevin Brosnan]

Have you checked that firefox -> preferences -> content -> javascript -> advanced -> disable or replace menus is checked.

Comment 3

[Michelle Sintov]

Regarding comment #2, the actual content in Flash contains the duplicated menus, so I think that the JavaScript setting should be unrelated. And, the behavior happens whether or not that item is checked.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this a regression between certain Firefox builds?

Comment 5

[Michelle Sintov]

There is kind of a weird convergence of which Firefox and Flash Player builds have this problem. Here's what I see:

FF 1.5 and any Flash Player: no problem
FF 2.0 and any Flash Player: no problem
FF 3.0 and Flash Player 9r60+: doubled-up menus
FF 3.0 and Flash Player 9r47: no problem

We will investigate on our end.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It might be helpful to have a Firefox regression range narrower than 2 years (i.e., based on nightlies).  Was it when we switched to Cocoa widgets, or turned on cairo on Mac, or something like that?

Comment 7

[Mike Schroepfer]

Can we get some help with regression ranges?

Comment 8

[Matthew Gregan [:kinetik]]

Reproduced with Flash Player 9r98 and a locally built trunk from last week on Leopard/10.5.  In this configuration, I'm seeing huge numbers of failed allocations logged when visiting www.toyota.com.  The allocations failures seem to start as soon as the Flash plugin is loaded:

For application/x-shockwave-flash found plugin Flash Player.plugin
[loaded plugin /Library/Internet Plug-Ins/Flash Player.plugin]
++DOMWINDOW == 9
For application/x-shockwave-flash found plugin Flash Player.plugin
WARNING: Write failed (non-fatal): file /Users/kinetik/work/mozilla-cvs/mozilla/xpcom/io/nsInputStreamTee.cpp, line 84
firefox-bin(33126,0xa02a8f60) malloc: *** mmap(size=1409089536) failed (error code=12)
*** error: can't allocate region
*** set a breakpoint in malloc_error_break to debug
Mon Nov 12 10:49:04 waste.local firefox-bin[33126] <Error>: CGBitmapContextInfoCreate: unable to allocate 1409088000 bytes for bitmap data
firefox-bin(33126,0xa02a8f60) malloc: *** mmap(size=1388097536) failed (error code=12)

0  0x910eb9f1 in malloc_error_break ()
#1  0x910e69df in szone_error ()
#2  0x9100eff6 in allocate_pages ()
#3  0x9100f7e2 in large_and_huge_malloc ()
#4  0x9100df15 in malloc_zone_calloc ()
#5  0x9100dea2 in calloc ()
#6  0x94c04cd5 in CGBitmapAllocateData ()
#7  0x94c0501c in CGBitmapContextInfoCreate ()
#8  0x94c04e2c in CGBitmapContextCreateWithDictionary ()
#9  0x94c04d9f in CGBitmapContextCreate ()
#10 0x1f5b347f in Flash_EnforceLocalSecurity () at NSGeometry.h:83
#11 0x1f4186f3 in dyld_stub_strstr () at NSGeometry.h:83
#12 0x1f5aad28 in Flash_EnforceLocalSecurity () at NSGeometry.h:83
#13 0x1f5967c2 in Flash_EnforceLocalSecurity () at NSGeometry.h:83
#14 0x1f10af4a in ns4xPluginInstance::HandleEvent (this=0x1ede7330, event=0xbfffbd90, handled=0xbfffbd8c) at /Users/kinetik/work/mozilla-cvs/mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp:1230
#15 0x1276f7bb in nsPluginInstanceOwner::Paint (this=0x1edb8660, aDirtyRect=@0xbfffbea0) at /Users/kinetik/work/mozilla-cvs/mozilla/layout/generic/nsObjectFrame.cpp:3601
#16 0x1276f807 in nsObjectFrame::PaintPlugin (this=0xc93ab4, aRenderingContext=@0x1f0086e0, aDirtyRect=@0xbfffbea0) at /Users/kinetik/work/mozilla-cvs/mozilla/layout/generic/nsObjectFrame.cpp:1205
#17 0x1276f84e in PaintPlugin (aFrame=0xc93ab4, aCtx=0x1f0086e0, aDirtyRect=@0xbfffbea0, aPt=@0xbfffbe28) at /Users/kinetik/work/mozilla-cvs/mozilla/layout/generic/nsObjectFrame.cpp:1034
#18 0x12730ea2 in nsDisplayGeneric::Paint (this=0xdd5264, aBuilder=0xbfffbf28, aCtx=0x1f0086e0, aDirtyRect=@0xbfffbea0) at ../base/nsDisplayList.h:818
#19 0x126a36eb in nsDisplayList::Paint (this=0xdd52bc, aBuilder=0xbfffbf28, aCtx=0x1f0086e0, aDirtyRect=@0xbfffbea0) at /Users/kinetik/work/mozilla-cvs/mozilla/layout/base/nsDisplayList.cpp:293
#20 0x126a4945 in nsDisplayWrapList::Paint (this=0xdd52b0, aBuilder=0xbfffbf28, aCtx=0x1f0086e0, aDirtyRect=@0xbfffbea0) at /Users/kinetik/work/mozilla-cvs/mozilla/layout/base/nsDisplayList.cpp:672
#21 0x126a54f2 in nsDisplayClip::Paint (this=0xdd52b0, aBuilder=0xbfffbf28, aCtx=0x1f0086e0, aDirtyRect=@0xbfffc06c) at /Users/kinetik/work/mozilla-cvs/mozilla/layout/base/nsDisplayList.cpp:864
#22 0x126a36eb in nsDisplayList::Paint (this=0xbfffbfa4, aBuilder=0xbfffbf28, aCtx=0x1f0086e0, aDirtyRect=@0xbfffc06c) at /Users/kinetik/work/mozilla-cvs/mozilla/layout/base/nsDisplayList.cpp:293
#23 0x126c7a71 in nsLayoutUtils::PaintFrame (aRenderingContext=0x1f0086e0, aFrame=0xb908a0, aDirtyRegion=@0xbfffc04c, aBackground=4294967295) at /Users/kinetik/work/mozilla-cvs/mozilla/layout/base/nsLayoutUtils.cpp:851
#24 0x126d6760 in PresShell::Paint (this=0xac1c00, aView=0x1e4edc40, aRenderingContext=0x1f0086e0, aDirtyRegion=@0xbfffc04c) at /Users/kinetik/work/mozilla-cvs/mozilla/layout/base/nsPresShell.cpp:5262
#25 0x12b73d9b in nsViewManager::RenderViews (this=0x1e4edd90, aView=0x1ed891b0, aRC=@0x1f0086e0, aRegion=@0xbfffc104) at /Users/kinetik/work/mozilla-cvs/mozilla/view/src/nsViewManager.cpp:604
#26 0x12b77fb0 in nsViewManager::Refresh (this=0x1e4edd90, aView=0x1ed891b0, aContext=0x1f0086e0, aRegion=0x1f008910, aUpdateFlags=1) at /Users/kinetik/work/mozilla-cvs/mozilla/view/src/nsViewManager.cpp:493
#27 0x12b7878b in nsViewManager::DispatchEvent (this=0x1e4edd90, aEvent=0xbfffc3c8, aStatus=0xbfffc2e8) at /Users/kinetik/work/mozilla-cvs/mozilla/view/src/nsViewManager.cpp:1067
#28 0x12b6ec25 in HandleEvent (aEvent=0xbfffc3c8) at /Users/kinetik/work/mozilla-cvs/mozilla/view/src/nsView.cpp:168
#29 0x11605d39 in nsChildView::DispatchEvent (this=0x1edeab60, event=0xbfffc3c8, aStatus=@0xbfffc35c) at /Users/kinetik/work/mozilla-cvs/mozilla/widget/src/cocoa/nsChildView.mm:1339
#30 0x116006a6 in nsChildView::DispatchWindowEvent (this=0x1edeab60, event=@0xbfffc3c8) at /Users/kinetik/work/mozilla-cvs/mozilla/widget/src/cocoa/nsChildView.mm:1352
#31 0x11609060 in -[ChildView drawRect:] (self=0x1edeac00, _cmd=0x92576630, aRect={origin = {x = 0, y = 0}, size = {width = 760, height = 340}}) at /Users/kinetik/work/mozilla-cvs/mozilla/widget/src/cocoa/nsChildView.mm:2226
#32 0x95c84eb0 in -[NSView _drawRect:clip:] ()
#33 0x95c83a44 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#34 0x95c823a3 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#35 0x95c831f4 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#36 0x95c831f4 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#37 0x95c831f4 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#38 0x95c831f4 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#39 0x95c831f4 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#40 0x95c831f4 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#41 0x95c831f4 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#42 0x95c831f4 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#43 0x95c81ce3 in -[NSThemeFrame _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#44 0x95c7e814 in -[NSView _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] ()
#45 0x95bbf37d in -[NSView displayIfNeeded] ()
#46 0x95bbef2d in -[NSWindow displayIfNeeded] ()
#47 0x95bbed50 in _handleWindowNeedsDisplay ()
#48 0x9129b9e2 in __CFRunLoopDoObservers ()
#49 0x9129cd45 in CFRunLoopRunSpecific ()
#50 0x9129dd38 in CFRunLoopRunInMode ()
#51 0x9414b8a4 in RunCurrentEventLoopInMode ()
#52 0x9414b5f6 in ReceiveNextEventCommon ()
#53 0x9414b531 in BlockUntilNextEventMatchingListInMode ()
#54 0x95bbcd5b in _DPSNextEvent ()
#55 0x95bbc6a0 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#56 0x95bb56d1 in -[NSApplication run] ()
#57 0x115f6d2d in nsAppShell::Run (this=0x637c10) at /Users/kinetik/work/mozilla-cvs/mozilla/widget/src/cocoa/nsAppShell.mm:546
#58 0x12228662 in nsAppStartup::Run (this=0x651470) at /Users/kinetik/work/mozilla-cvs/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:170
#59 0x000f37cf in XRE_main (argc=4, argv=0xbffff680, aAppData=0x60ebc0) at /Users/kinetik/work/mozilla-cvs/mozilla/toolkit/xre/nsAppRunner.cpp:3142
#60 0x000026d3 in main (argc=4, argv=0xbffff680) at /Users/kinetik/work/mozilla-cvs/mozilla/browser/app/nsBrowserApp.cpp:153

The website (including Flash content) continues to work, but is very slow.  The doubled menu bar appears as soon as I hover over a menu item.

In earlier builds, rather than seeing a doubled menu I am just seeing the allocation failures and the Flash content is unresponsive (firefox-bin process and WindowServer process are using around 100% CPU together).

Using the same setup (9r98, Leopard/10.5):
- 20061101 OK
- 20061231 OK
- 20070515 FAIL
- 20070315 OK
- 20070415 FAIL
- 20070330 FAIL
- 20070322 OK
- 20070326 OK
- 20070328 FAIL
- 20070327 FAIL

The trunk nightly from 2007-03-26 works, the trunk nightly from 2007-03-27 fails.

Comment 9

[Matthew Gregan [:kinetik]]

The behaviour changed when the patch for bug #344427 landed at 2007-03-26 18:07:

> Implement CoreGraphics NPAPI Drawing Model. NPAPI plugins now have the option to > render using CoreGraphics instead of Quickdraw. b=344427 r=sfraser sr=jst

Confirmed by building trunk with MOZ_CO_DATE="2007-03-26 17:00 PST" (works) and "2007-03-26 18:07 PST" (fails).

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: testcase 1

This testcase is sufficient to reproduce at least part of the problem--the high CPU use and logging of allocation failures in CGBitmapContextInfoCreate.  I'm still not sure if the allocation failures are a separate problem to the menu doubling.  Working on a simplified reproducer for the menu doubling and also trying to determining if the allocation failures are related or a separate problem.

Comment 11

[Matthew Gregan [:kinetik]]

Forgot to mention that the testcase in comment #10 can use any Flash file--I was using a simple animation from a collection of example files.

Watching plugin initialization, nsPluginInstanceOwner::GetDrawingModel() is returning NPDrawingModelCoreGraphics when using Flash 9.0 >= r60 and NPDrawingModelQuickDraw when using r47.  This explains why the combination of >= r60 and the NPAPI/CoreGraphics patch is required to see the reported problems.

Comment 12

[Matthew Gregan [:kinetik]]

Attachment: testcase 1 v2

Simplified testcase 1.

Enabling logging in the plugin API suggests that the allocation failures are caused by calls to NPN_InvalidateRect with a bogus looking rect:

-1607823520[60abc0]: NPN_InvalidateRect: npp=1ec3cedc, top=0, left=0, bottom=15688, right=16385

Comment 13

[Matthew Gregan [:kinetik]]

As another data point, a current WebKit build (r27779) with Flash 9r98 using the CoreGraphics NPAPI drawing model does not exhibit either of the symptoms.

Comment 14

[Matthew Gregan [:kinetik]]

I don't know OS X drawing stuff well enough to make any real progress with this.  Assigning back to default assignee.

I'll attach the minimized testcases I refer to below after committing this comment.  Each of the testcases may be for the same root cause (it looks like it is), but I'm not entirely sure.

The first testcase is Flash content with position:fixed style on the embed.  I ended up using a simple animation from an example site--the Flash content from toyota.com is not required to reproduce.  The Flash content does not draw correctly, and the console is flooded with the allocation failures I mentioned above.  Changing the position to relative or removing the style attribute completely causes the content to draw and the allocation failure messages to stop.

The second testcase is similar to the first, but there are two pieces of Flash and the first is position:absolute.  I'm pretty sure this testcase is hitting the same bug so it may be redundant.

The third testcase reproduces the drawing problems.  This is probably also caused by the same bug as the first two, but it manifests differently and reproduces the "Flash content drawn doubled" problem originally reported.  In this case, there are two pieces of Flash content and the second overlaps the first.  The first piece of content is drawn twice, and allocation failures are logged during content redraw.  If the second piece of content is moved far enough down the page that it does not overlap, both pieces draw correctly and the allocation failures do not occur.

I added ad-hoc logging to nsPluginInstanceOwner::FixUpPluginWindow and nsChildView::GetPluginClipRect to capture all of the origin/size/clipping rects we compute, as well as enabling DEBUG_UPDATE in nsChildView.mm, and running with NSPR_LOG_MODULES=nsObjectFrame:5,Plugin:5,PluginNPP:5,PluginNPN:5.  Comparing the values from the first of the testcases mentioned in this comment to a run with the same testcase but position:relative, I can't see anything obviously wrong in the failing case (the values are the same in both cases).  The view and frame tree look as expected in both cases.  Values passed to the plugin via SetWindow also look valid in both cases.

In comparing the logs, the only difference I can see is that GetPluginClipRect/FixUpPluginWindow are being called in the context of a different nsChildView in the failing case:

Working:

-1610363040[60abf0]: NPP SetWindow called: this=1eb65e60, [x=0,y=0,w=240,h=200], clip[t=53,b=53,l=0,r=0], return=0
---- Update[0x19728080][0x19727fe0] [0.000000 0.000000 1111.000000 858.000000] cgc: 0x1974a5e0
  gecko bounds: [0 0 1112 860]
  xform in: [1.000000 0.000000 0.000000 -1.000000 0.000000 860.000000]
  window coords: [0 0 1111 858]
---- update done ----
---- Update[0x1ebbf1e0][0x1ebbe640] [0.000000 0.000000 241.000000 201.000000] cgc: 0x1974a5e0
  gecko bounds: [0 0 1112 788]
  xform in: [1.000000 0.000000 0.000000 -1.000000 0.000000 807.000000]
  window coords: [0 0 241 201]
---- update done ----
---- Update[0x1ebc4dc0][0x1ebc4d20] [0.000000 0.000000 240.000000 200.000000] cgc: 0x1974a5e0
  gecko bounds: [0 0 240 200]
  xform in: [1.000000 0.000000 0.000000 -1.000000 0.000000 807.000000]
GetPluginClipRect 0x1ebc4d20: viewOrigin x:0.000000 y:807.000000
GetPluginClipRect 0x1ebc4d20: frame x:0.000000 y:0.000000 w:1112.000000 h:860.000000
GetPluginClipRect 0x1ebc4d20: viewOrigin 0 x:0.000000 y:53.000000
GetPluginClipRect 0x1ebc4d20: visibleBounds x:0.000000 y:0.000000 w:240.000000 h:200.000000
GetPluginClipRect 0x1ebc4d20: clipOrigin x:0.000000 y:807.000000
GetPluginClipRect 0x1ebc4d20: clipOrigin 0 x:0.000000 y:53.000000
GetPluginClipRect 0x1ebc4d20: outClipRect x:0 y:53 w:0 h:0
GetPluginClipRect 0x1ebc4d20: outClipRect 0 x:0 y:53 w:240 h:200
GetPluginClipRect 0x1ebc4d20: outOrigin x:0 y:-53
FixUpPluginWindow 0x1ea2ec40: pluginOrigin: x:0 y:-53
FixUpPluginWindow 0x1ea2ec40: widgetClip: w:240 h:200 x:0 y:53
FixUpPluginWindow 0x1ea2ec40: geckoBounds: w:240 h:200 x:0 y:0
FixUpPluginWindow 0x1ea2ec40: geckoBounds 0: w:240 h:200 x:0 y:0
FixUpPluginWindow 0x1ea2ec40: geckoScreenCoords: w:240 h:200 x:15 y:109
FixUpPluginWindow 0x1ea2ec40: windowRect: t:34 l:15 b:916 r:1127
FixUpPluginWindow 0x1ea2ec40: mPluginWindow: x:0 y:75
FixUpPluginWindow 0x1ea2ec40: oldClipRect: t:53 l:0 b:53 r:0
FixUpPluginWindow 0x1ea2ec40: curClipRect: t:53 l:0 b:253 r:240
-1610363040[60abf0]: ns4xPluginInstance::SetWindow (about to call it) this=1eb65e60
-1610363040[60abf0]: NPP SetWindow called: this=1eb65e60, [x=0,y=75,w=240,h=200], clip[t=53,b=253,l=0,r=240], return=0
FixUpPluginWindow 0x1ea2ec40: called SetWindow, normal delay set
FixUpPluginWindow 0x1ea2ec40: ret cgPort 0x1eb67750


Failing:

-1610363040[60abf0]: NPP SetWindow called: this=1ecd6710, [x=0,y=0,w=240,h=200], clip[t=53,b=53,l=0,r=0], return=0
---- Update[0x633150][0x6330b0] [0.000000 0.000000 1112.000000 53.000000] cgc: 0x197c0220
  gecko bounds: [0 0 1112 860]
  xform in: [1.000000 0.000000 0.000000 -1.000000 0.000000 860.000000]
  window coords: [0 0 1112 53]
---- update done ----
---- Update[0x1ecc1a80][0x1ecc0ee0] [0.000000 0.000000 1112.000000 788.000000] cgc: 0x197c0220
  gecko bounds: [0 0 1112 788]
  xform in: [1.000000 0.000000 0.000000 -1.000000 0.000000 807.000000]
GetPluginClipRect 0x147ef2c0: viewOrigin x:0.000000 y:807.000000
GetPluginClipRect 0x147ef2c0: frame x:0.000000 y:0.000000 w:1112.000000 h:860.000000
GetPluginClipRect 0x147ef2c0: viewOrigin 0 x:0.000000 y:53.000000
GetPluginClipRect 0x147ef2c0: visibleBounds x:0.000000 y:0.000000 w:240.000000 h:200.000000
GetPluginClipRect 0x147ef2c0: clipOrigin x:0.000000 y:807.000000
GetPluginClipRect 0x147ef2c0: clipOrigin 0 x:0.000000 y:53.000000
GetPluginClipRect 0x147ef2c0: outClipRect x:0 y:53 w:0 h:0
GetPluginClipRect 0x147ef2c0: outClipRect 0 x:0 y:53 w:240 h:200
GetPluginClipRect 0x147ef2c0: outOrigin x:0 y:-53
FixUpPluginWindow 0x1ecd5a40: pluginOrigin: x:0 y:-53
FixUpPluginWindow 0x1ecd5a40: widgetClip: w:240 h:200 x:0 y:53
FixUpPluginWindow 0x1ecd5a40: geckoBounds: w:240 h:200 x:0 y:0
FixUpPluginWindow 0x1ecd5a40: geckoBounds 0: w:240 h:200 x:0 y:0
FixUpPluginWindow 0x1ecd5a40: geckoScreenCoords: w:240 h:200 x:15 y:109
FixUpPluginWindow 0x1ecd5a40: windowRect: t:34 l:15 b:916 r:1127
FixUpPluginWindow 0x1ecd5a40: mPluginWindow: x:0 y:75
FixUpPluginWindow 0x1ecd5a40: oldClipRect: t:53 l:0 b:53 r:0
FixUpPluginWindow 0x1ecd5a40: curClipRect: t:53 l:0 b:253 r:240
-1610363040[60abf0]: ns4xPluginInstance::SetWindow (about to call it) this=1ecd6710
-1610363040[60abf0]: NPP SetWindow called: this=1ecd6710, [x=0,y=75,w=240,h=200], clip[t=53,b=253,l=0,r=240], return=0
FixUpPluginWindow 0x1ecd5a40: called SetWindow, normal delay set
FixUpPluginWindow 0x1ecd5a40: ret cgPort 0x1ec69850

Note that the |---- Update| messages are different.

The |---- Update| logging from [ChildView -drawRect] logs (among other things) |self->mGeckoChild| as the second pointer and the aRect passed in as the four float values.  nsChildView::GetPluginClipRect logs |this| as the pointer along with each of the log messages.

In the working case, drawRect has been called on the ChildView associated with the plugin area.  The aRect values are correct for the plugin window's dimensions.

In the failing case, drawRect has been called on the ChildView for the browser content area.  The aRect values are much larger (and not valid for the plugin), but we end up calling GetPluginClipRect from this drawRect call and eventually dispatching an updateEvt message to the plugin.

This looks suspicious, but it might just be the result of the way the views are set up when the plugin frame is position:fixed vs position:relative.  We don't pass the values of aRect from the drawRect call into the plugin (ultimately we end up dispatching an updateEvt to the plugin as a result of this drawRect called, but the only params we pass to the plugin are the CG context and the WindowRef), but perhaps we are invalidating the wrong area at the wrong time in this case.  Adding DebugPrintWindow calls into ns4xPluginInstance::HandleEvent to dump the window we pass to the plugin via event->event->message (cast to WindowRef) doesn't reveal anything wrong in the failing case.

Sorry if this looks like a clumsy analysis of the problem so far, but as I said, I don't know the OS X drawing code or the plugin code very well yet.

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: testcase 1 for comment #14

Comment 16

[Matthew Gregan [:kinetik]]

Attachment: testcase 2 for comment #14

Comment 17

[Matthew Gregan [:kinetik]]

Attachment: testcase 3 for comment #14

Use up/down cursor keys to move the second piece of Flash content.  Once it overlaps the first (even by a single pixel), the first will begin to draw parts of both the first and second Flash content inside its own region.

Comment 18

[Matthew Gregan [:kinetik]]

Attachment: plugin content not overlapping

Comment 19

[Matthew Gregan [:kinetik]]

Attachment: plugin content with small overlap

Comment 20

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

Josh, I'm seeing this bug on many sites, it looks really bad and I suspect it might be related to the GMail crazy refresh bug I just posted a workaround for. I think this needs major love, and soon.

Comment 21

[Marcia Knous [:marcia]]

Using the latest nightly on Leopard and Tiger I see this as well, using Flash r115. What is worse is that the main flash content under the menus does not show on Mac - should I file another bug for that? Note that the toyota.com page shows fine on Win Vista with r115.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Upping priority based on previous comments. Josh, feel free to change it back if you disagree.

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #17)
> Created an attachment (id=289285) [details]
> testcase 3 for comment #14
> 
> Use up/down cursor keys to move the second piece of Flash content.  Once it
> overlaps the first (even by a single pixel), the first will begin to draw parts
> of both the first and second Flash content inside its own region.

This testcase now seems to work for me using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008020304 Minefield/3.0b3pre

Not sure what fixed this, if indeed it is; note that Toyota has redesigned its homepage.

Comment 24

[Matthew Gregan [:kinetik]]

Attachment: flash content for testcases

It's not fixed, I can still reproduce on today's trunk.  The new Toyota site design doesn't display the problems of the old site, but the testcases still show the problem.

Unfortunately, I think the original testcases I attached are somewhat confusing because they require you to save them and ensure some animated flash content named test1.swf is available in the same directory.  Sorry about that.

Attaching some freely distributable animated content (from Adobe's Flash example page) now, and then I'll attach a new testcase that uses the attached flash directly.

Comment 25

[Matthew Gregan [:kinetik]]

Attachment: flash inside position:fixed div, does not draw when flash is in CG mode

Comment 26

[Matthew Gregan [:kinetik]]

Attachment: two flash embeds, one with position:absolute, mem alloc errors logged to console

Comment 27

[Matthew Gregan [:kinetik]]

Attachment: two flash embeds, second does not draw initially, drawn corrupt when overlapped

This one demonstrates multiple related issues.  When loaded initially, the animated flash content should be displayed twice.  Instead, the second animated is not shown and the parent div's background colour shines through.

Moving the second flash content down with the down cursor key far enough will cause it to begin drawing.  Once it does, hit space to reset its position.  It will now overlap the first flash content--watch the right edge of the first flash and yo will see the animation leave a trail behind.  Moving the second flash content down so it no longer overlaps the first causes the first to draw correctly again.

Comment 28

[Matthew Gregan [:kinetik]]

Attachment: gross hack: tell plugin we do not support CoreGraphics mode

If we change the plugin API to say that we don't support CoreGraphics mode, the testcases work without major problems (attachment #301420 [details] still has an issue, it looks like the two flash areas are z-fighting).

I'm not seriously proposing this patch, but if it gets late in the release cycle we might be better off disabling CG mode than shipping with these bugs on OS X... It would buy us additional time until someone has enough cycles to debug this properly.

Comment 29

[Matthew Gregan [:kinetik]]

Comparing the behaviour of the position: fixed testcase in attachment #301417 [details] with the same test using position: relative, it turns out that we are asking the plugin to draw twice as often in the position: fixed case.  One of the draw requests is valid (it's invoked as the result of a ChildView::drawRect call on the view associated with the plugin).  The other is invalid (it is invoked as a result of a ChildView::drawRect call on a different view and we get confused somewhere (in the view manager?) and ask the plugin to paint).  In the second case the plugin will draw with the wrong CGContext.

As an experiment, I added a check to the plugin paint code in nsObjectFrame to return early (before sending an updateEvt asking the plugin to paint) if GetPluginPort hands back a plugin port containing a NULL CGContext (indicating that it was called without the associated view having focus).  This hack is sufficient to allow the three testcases (attachment #301417 [details], attachment #301420 [details], and attachment #301423 [details]) to behave as expected (draw correctly and not log allocation errors to the console) and site linked in the bug URL (parkerpen.com) is less messed up.  parkerpen.com still has some problems with overlapped plugin content--the menus on the left are only visible while a submenu is sliding out, and the left-of-center menu is not visible at all, but we no longer draw content doubled up/in the wrong place, and the street view feature of Google Maps still does not render correctly.

I'll see if I can work out why and where we get confused and ask the plugin to paint in the invalid cases tomorrow.

Comment 30

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

Aha!!! That's a big clue. We should be able to polish this off tomorrow.

Comment 31

[Matthew Gregan [:kinetik]]

When we receive paint requests (via ChildView::drawRect) for areas that include a plugin area, we usually avoid attempting to paint the plugin because nsViewManager::AddCoveringWidgetsToOpaqueRegion adds the plugin widget to the opaque region, which eventually results in the plugin being removed from the display list when nsLayoutUtils::PaintFrame calls OptimizeVisibility on the display list.

In the position:fixed case, the plugin widget is not added to the opaque region in nsViewManager::AddCoveringWidgetsToOpaqueRegion because the nearest widget to aRootView (which is the same in both cases) does not have the plugin widget as a child, so the plugin ends up on the display list in nsLayoutUtils::PaintFrame and is not removed by the optimization pass.

In this case, we will end up in nsPluginInstanceOwner::Paint, which will send an updateEvt to the plugin to tell it to paint.  The plugin will use the CGContextRef we gave it last time ns4xPluginInstance::SetWindow was called.

#0  nsPluginInstanceOwner::Paint
#1  nsObjectFrame::PaintPlugin
#2  PaintPlugin
#3  nsDisplayGeneric::Paint
#4  nsDisplayList::Paint
#5  nsDisplayWrapList::Paint
#6  nsDisplayClip::Paint
#7  nsDisplayList::Paint
#8  nsLayoutUtils::PaintFrame
#9  PresShell::Paint
#10 nsViewManager::RenderViews
#11 nsViewManager::Refresh
#12 nsViewManager::DispatchEvent
#13 HandleEvent
#14 nsChildView::DispatchEvent
#15 nsChildView::DispatchWindowEvent
#16 -[ChildView drawRect:]

In frame #16, the view is 0x24a8adc0, the CGContext is 0x2238c0f0, and this view has focus locked.

In frame #0, mWidget->mView is 0x231b1d00, and the CGContext we last passed to the plugin is 0x2238c0f0, and mView does not have focus locked.

So the CGContext is still valid, but since the plugin's view is not locked, the context will be set up for view 0x24a8adc0 and thus will have inappropriate window coords, etc. set up.  If we tell the plugin to draw at this point, Bad Things happen.

There are a couple of solutions we can use.  The simplest is to alter nsPluginInstanceOwner::Paint so that it does not tell the plugin to paint if the plugin widget's view is not locked (i.e. if we're attempting to draw as a result of a ChildView::drawRect on some surrounding area).  This is what my hack mentioned in comment #29 does.  This is a simple change, but it will ensure that some functionality does not work--roc pointed out that it should currently be possible to render plugin content offscreen on Mac, but if we only allow the plugin to paint when it's view has focus it will result in offscreen paint calls being suppressed.

Another solution would be to allocate a CGContext for the bitmap (using CGBitmapContextCreate or CGLayerCreateWithContext) which we can let the plugin draw to at any time, and then draw on screen only when the plugin widget's view has focus locked.  This is also actually a pretty simple change and I have a prototype patch for this that seems to work well (but is likely such a naïve  attempt that it will need further revision).

Note that neither of the proposed fixes above solve all of the problems.  They do solve the memory allocation errors and general erratic behaviour caused by the plugin attempting to use invalid/misconfigured CGContexts, and fix all of the drawing problems demonstrated by my testcases.  It also improves the situation with parkerpen.com, but there are still issues to do with overlapping content there and with the street view feature of Google Maps.

It's worth noting that without one of the suggested fixes applied, I can't get street view to display at all (except for the tiny bit of plugin content visible in the lower right corner of where it should be).  With the fixes applied, it is not visible by default, but removing the z-order stylefrom one of the embed's parent divs using DOMi will reveal the correctly drawn flash content.  It looks like the parkerpen.com issue is similar, since the left-hand menu will draw when hovered over, but the rest of the time one of the other pieces of flash draws over the top of it.

I think it'd be good to get a partial fix in so that we make some progress, and I can continue looking for a fix for the remaining problems with parkerpen.com and gmaps street view.

Comment 32

[Matthew Gregan [:kinetik]]

I talked to roc a bit about this, and he pointed out a bunch of things I had missed.  I think he's going to post a comment explaining things soon, but in the meantime I'll note down what I remember or worked out based on that conversation.

ChildView::drawRect sets up a rendering context (based off of the current CGContext) each time it's called.  This will eventually be passed into PaintPlugin when we ask the plugin to paint.  So another solution (and probably the correct one) would be to save the current rendering state and set ourselves up appropriately to paint the plugin using this rendering context.  This would set the CGContext up to paint the plugin properly even (or, if necessary for perf reasons, only) when painting was initiated as a result of a drawRect call to a surrounding view.  Looking at the existing Windows/Linux code, they do something similar already, and I guess it wasn't necessary on Mac with the QuickDraw plugin drawing model.

Comment 33

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

Cocoa theme drawing has code to set up a CGContext to reflect the current cairo state:
http://mxr.mozilla.org/seamonkey/source/widget/src/cocoa/nsNativeThemeCocoa.mm#838
I think we need to extract this code into an interface similar to gfxWindowsNativeDrawing or gfxXlibNativeRenderer, and use it from both nsNativeThemeCocoa and nsObjectFrame. The code actually needs to be extended to handle non-Quartz surface types by rendering to an offscreen RGBA buffer that we can then composite onto any surface. Using it from nsObjectFrame is going to be a little tricky since a) we can't pass the CGContext to the plugin, so we may have to call SetWindow or somehow check that the CGContext we have is the same one we gave to the plugin before and b) it's not clear what state the plugin expects CGContext to be in when it draws into it. Josh, what docs are there covering the CGContext plugin drawing model?

We really need gfxWindowsNativeThemeDrawing used in nsObjectFrame too, BTW. Vlad mentioned he was looking at this.

Comment 34

[Josh Aas]

http://wiki.mozilla.org/Mac:NPAPI_Drawing_Models

Comment 35

[Josh Aas]

Is 416860 a dupe of this?

Comment 36

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

Quite possibly, but I'm not sure. Thanks for those docs.

Comment 38

[Matthew Gregan [:kinetik]]

Attachment: native drawing v1

First pass at implementing a Quartz version of the native drawing utility class.  Based on the API of the Windows implementation (gfxWindowsNativeDrawing) and code from nsNativeThemeCocoa::DrawWidgetBackground.

Replaces the existing code in nsNativeThemeCocoa::DrawWidgetBackground with use of gfxQuartzNativeDrawing, and introduces use of a gfxQuartzNativeDrawing in the CoreGraphics plugin paint path in nsObjectFrame::PaintPlugin.

This fixes all of the known issues with plugin rendering caused by this bug.  The attached testcases all work correctly, reported broken sites (maps.google.com street view, parkerpen.com, nike.com, nypost.com, drinkarizona.com) all work, and native theme rendering seems to work correctly (used testcases from bug #382049 and bug #397792, plus browsing sites with many native widgets).

As a bonus, rotated/scaled plugin content now almost works--the plugin content is drawn correctly rotated/scaled but doesn't redraw properly after invalidation.

Comment 39

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

+        /* Whether the native drawing can draw to a surface of content COLOR_ALPHA */
+        CAN_DRAW_TO_COLOR_ALPHA    = 1 << 0,
+        CANNOT_DRAW_TO_COLOR_ALPHA = 0 << 0,
+
+        /* Whether the native drawing can be scaled using CGContextConcatCTM */
+        CAN_AXIS_ALIGNED_SCALE     = 1 << 1,
+        CANNOT_AXIS_ALIGNED_SCALE  = 0 << 1,
+
+        /* Whether the native drawing can be both scaled and rotated arbitrarily using CGContextConcatCTM */
+        CAN_COMPLEX_TRANSFORM      = 1 << 2,
+        CANNOT_COMPLEX_TRANSFORM   = 0 << 2,

I don't think we need these. Plugins or anything else that draws to a CGContext should be able to do all of these always. So we can take the flags parameter out altogether.

+     * CGContextRef.  This may or may not change CGRect, depending on whether
+     * SetWorldTransform is used or not. */

SetWorldTransform? In fact TransformToNativeRect can go because it's trivial.

Combine EndNativeDrawing and PaintToContext.

We don't really need mRenderState. Or mTransformType. Or mTranslation. We can know whether Init succeeded by whether mQuartzSurface is null or not.

mDeviceOffset can be a local variable.

Put an XXX comment in the failure path to indicate that we should create a temporary surface here, draw into that and at the end blit it back to the original gfxContext.

+        if (m.HasNonTranslationOrFlip()) {
+            // If we have a scale, I think we've already lost; so don't round
+            // anything here
+            CGContextConcatCTM(mCGContext, CGAffineTransformMake(m.xx, m.yx,
+                                                                 m.xy, m.yy,
+                                                                 m.x0, m.y0));
+        } else {
+            // Otherwise, we round the x0/y0, because otherwise things get rendered badly
+            // XXX how should we be rounding the x0/y0?
+            CGContextConcatCTM(mCGContext, CGAffineTransformMake(m.xx, m.yx,
+                                                                 m.xy, m.yy,
+                                                                 floor(m.x0 + 0.5),
+                                                                 floor(m.y0 + 0.5)));
+        }

Remove the duplicate calls and just use local variables for x0 and y0 set appropriately.

+        // we drew directly to the CGContextRef in the context; undo our changes

Be more clear we're undoing the changes to the gstate, not the actual rendering :-)

+      nsCOMPtr<nsIDeviceContext> dc;
+      aRenderingContext.GetDeviceContext(*getter_AddRefs(dc));
+      gfxFloat p2a = gfxFloat(dc->AppUnitsPerDevPixel());

PresContext()->AppUnitsPerDevPixel()

+      nsRefPtr<gfxContext> ctx = aRenderingContext.ThebesContext();

gfxContext*, no chance of ctx going away here

Can you assert that the CGContext you have here is the one the plugin is going to use?

+      // XXXkinetik two issues need to be tidied up in future:
+      // - if gfxQuartzNativeDrawing ever gave us a CGContext other than the
+      //   current one, we would need to pass that to the plugin via SetWindow
+      // - we rely on the FixUpPluginWindow call in Paint calling SetWindow
+      // - when appropriate, for now it just does so unconditionally in the
+      // - CG drawing model case

Ugh, I really think we should fix this for 1.9, if not in this patch. I realize it means some code refactoring.

The dashes make this comment hard to read :-)

Comment 40

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #39)
> +        /* Whether the native drawing can draw to a surface of content
> COLOR_ALPHA */
> +        CAN_DRAW_TO_COLOR_ALPHA    = 1 << 0,
> +        CANNOT_DRAW_TO_COLOR_ALPHA = 0 << 0,
> +
> +        /* Whether the native drawing can be scaled using CGContextConcatCTM
> */
> +        CAN_AXIS_ALIGNED_SCALE     = 1 << 1,
> +        CANNOT_AXIS_ALIGNED_SCALE  = 0 << 1,
> +
> +        /* Whether the native drawing can be both scaled and rotated
> arbitrarily using CGContextConcatCTM */
> +        CAN_COMPLEX_TRANSFORM      = 1 << 2,
> +        CANNOT_COMPLEX_TRANSFORM   = 0 << 2,
> 
> I don't think we need these. Plugins or anything else that draws to a CGContext
> should be able to do all of these always. So we can take the flags parameter
> out altogether.

Not sure if this is true -- I know some of the native theme things don't render correctly under complex transformation, at least according to colin.  Haven't tested though, scrollbars were supposedly in this category.  It's worth testing.

Comment 41

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

Yeah, I think we were thinking of using NSScroller for that reason, but the theme code currently doesn't do anything here so I think we should add these flags to the code only when we start really using them.

Comment 42

[Matthew Gregan [:kinetik]]

Attachment: native drawing v2

Address roc's comments and resync with tip.  Adding Vlad/roc for r/sr, do I need Josh or another Mac expert to look at this as well?

> Ugh, I really think we should fix this for 1.9, if not in this patch. I realize
> it means some code refactoring.

Me too.  I'll file a new bug and work on this as a separate patch to apply on top of this one.

Comment 43

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

+     * This class assumes that native drawing can take place only if the
+     * destination surface has a content type of COLOR (that is, RGB24) or
+     * COLOR_ALPHA (that is, RGBA32), and that the transformation matrix
+     * consists of only a translation or a translation and scale.

This comment isn't right? we're not making any assumptions about the transformation matrix.

+        NS_ERROR("unhandled surface type");

This should really be a warning. it's not a program error for us to reach this point.

+      CGContextRef cgContext = nativeDrawing.BeginNativeDrawing();
+      NS_ASSERTION(cgContext, "null CGContextRef");

Likewise, we shouldn't assert here. We should detect null and bail out safely.

+    mInstance->SetWindow(mPluginWindow);

What is the purpose of adding this? Isn't the window already set to mPluginWindow?

Comment 44

[Matthew Gregan [:kinetik]]

Attachment: native drawing v3

> This comment isn't right? we're not making any assumptions about the
> transformation matrix.

Removed that whole chunk, we don't make assumptions about the surface type either, it's checked and unhandled types cause an NS_WARNING.

> What is the purpose of adding this? Isn't the window already set to
> mPluginWindow?

Removed, it's no longer necessary.  Short answer: it was papering over a bug.  Long answer: tt was added because the last SetWindow call we made might have been with a null CGContextRef (nsChildView would return null if the current view was not focused), so I added an unconditional SetWindow to make sure a non-null CGContextRef was passed to the plugin as soon as we had one (not just when the clip rect changed size).  Since then, I changed nsChildView to always return the current context since the caller no longer requires the plugin view to be focused when painting the plugin.

Comment 45

[Xavez]

I think a bug I posted might be related to this... https://bugzilla.mozilla.org/show_bug.cgi?id=410248

Comment 46

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

Comment on attachment 303512 [details] [diff] [review]
native drawing v3

You might want to add a comment that mNativeRect is unused but that's OK because we'll need it later when we add an offscreen buffer fallback.

Comment 47

[Matthew Gregan [:kinetik]]

Attachment: native drawing v4

minor revision: add comment per roc's suggestion, fix build on 10.4 by using our gfxFloat rather than 10.5's CGFloat.

Comment 48

[Matthew Gregan [:kinetik]]

Tryserver builds using latest patch:

https://build.mozilla.org/tryserver-builds/2008-02-18_16:[e-mail]-1203381876/

Comment 49

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 304123 [details] [diff] [review]
native drawing v4

Looks fine here; I'm not sure if some of the XXX's are still correct (e.g. needing to set Over, say;) though they're fine for now.

In the future, if we have a non-Over operator or in some other situations, we may need to start using BeginTransparencyLayer and friends.  but looks fine for now.

Comment 50

[Matthew Gregan [:kinetik]]

Attachment: without setting Over

The source over stuff is still needed (see attachment for rendering of testcase in bug #382049 without setting over), but yeah, I'm not totally sure about some of the other XXX comments (I brought them over from the theme drawing code assuming someone knew what they meant).

Comment 51

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

oh, I agree that setting source-Over is needed, but it's kind of the wrong fix there.. but like I said, unilaterally setting it now is the right thing to do.  When we want to support other operators, then we need to use BeginTransparencyLayer; I'd leave it in now.

Comment 52

[Reed Loden [:reed]]

Checking in gfx/thebes/public/Makefile.in;
/cvsroot/mozilla/gfx/thebes/public/Makefile.in,v  <--  Makefile.in
new revision: 1.40; previous revision: 1.39
done
RCS file: /cvsroot/mozilla/gfx/thebes/public/gfxQuartzNativeDrawing.h,v
done
Checking in gfx/thebes/public/gfxQuartzNativeDrawing.h;
/cvsroot/mozilla/gfx/thebes/public/gfxQuartzNativeDrawing.h,v  <--  gfxQuartzNativeDrawing.h
initial revision: 1.1
done
Checking in gfx/thebes/src/Makefile.in;
/cvsroot/mozilla/gfx/thebes/src/Makefile.in,v  <--  Makefile.in
new revision: 1.68; previous revision: 1.67
done
RCS file: /cvsroot/mozilla/gfx/thebes/src/gfxQuartzNativeDrawing.cpp,v
done
Checking in gfx/thebes/src/gfxQuartzNativeDrawing.cpp;
/cvsroot/mozilla/gfx/thebes/src/gfxQuartzNativeDrawing.cpp,v  <--  gfxQuartzNativeDrawing.cpp
initial revision: 1.1
done
Checking in layout/generic/nsObjectFrame.cpp;
/cvsroot/mozilla/layout/generic/nsObjectFrame.cpp,v  <--  nsObjectFrame.cpp
new revision: 1.629; previous revision: 1.628
done
Checking in widget/src/cocoa/nsChildView.mm;
/cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v  <--  nsChildView.mm
new revision: 1.309; previous revision: 1.308
done
Checking in widget/src/cocoa/nsNativeThemeCocoa.mm;
/cvsroot/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm,v  <--  nsNativeThemeCocoa.mm
new revision: 1.79; previous revision: 1.78
done

Comment 53

[Stephen Donner [:stephend] Not actively reading bugmail]

http://www.drinkarizona.com/ is fixed, as is http://www.nike.com, and http://www.parkerpen.com/en/; I'll be keeping an eye out for (potential) regressions, and will file spin-off bugs if needed.

Verified FIXED using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008022004 Minefield/3.0b4pre with Shockwave Flash 9.0 r115

Thanks for taking this on, Matthew!

Comment 54

[jag (Peter Annema)]

This caused bug 419187 (SeaMonkey and Camino).