459530 - Drawing page with drawWindow duplicates flash content

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Dave Townsend [:mossop]]

+++ This bug was initially created as a clone of Bug #408898 +++

I'm still seeing the same issue as bug 408898, quite frequently now. I'm not on flash 9.0.124. It doesn't seem to be every drawWindow call that duplicates the flash display, but I can reasonably reliably see it after using pandora for a short time with my add-on Tab Sidebar installed and enabled for a short time (http://www.oxymoronical.com/files/extensions/TabSidebar/archive/TabSidebar-2.5a1.rev395.xpi).

Given that we are starting to use drawWindow in the product with ctrl-tab and other planned ideas I think we should consider blocking on this as it is pretty ugly when it happens.

Comment 1

[Dave Townsend [:mossop]]

Attachment: screenshot

Comment 2

[Steven Michaud [:smichaud] (Retired)]

We can't really block on this until we have steps to reproduce.

Does it also happen on Windows and/or Linux?

Comment 3

[Dave Townsend [:mossop]]

STR:

1. Install the add-on in comment 0.
2. Restart Firefox and go to view - sidebar - Tab Sidebar.
3. Go to www.pandora.com.
4. If the problem isn't immediately evident click play/pause a few times or resize the sidebar.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #3)

I can't reproduce your STR.  I'm using Flash 9.0 r124 (which is the
current version from Apple).  I tested on OS X 10.5.5.

> I'm not on flash 9.0.124.

What version are you using.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Should have mentioned that I tested with today's Minefield nightly.

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #4)
> (In reply to comment #3)
> 
> I can't reproduce your STR.  I'm using Flash 9.0 r124 (which is the
> current version from Apple).  I tested on OS X 10.5.5.
> 
> > I'm not on flash 9.0.124.
> 
> What version are you using.

Should have read "I'm now on flash 9.0.124"

Comment 7

[Dave Townsend [:mossop]]

Try it with extensions.tabsidebar.selecteddelay at -1 and extensions.tabsidebar.position as 3

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Ok, with these settings (which I assume are integer settings) I saw the upside down display once (in the Tab Sidebar).  But that doesn't a good STR make (and even I wasn't able to figure out exactly what I'd done to trigger the problem).

Please give us an STR that works at least 50% of the time.  A reduced testcase would also be really nice :-)

Comment 9

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

Matthew Gregan probably has the best clue here.

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: flash media for testcase

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: testcase

This testcase reproduces the issue reliably for me.  Open the testcase, then hold the cursor down key to move the fixed position div containing the flash and canvas.  Fairly regularly, the canvas will paint at the very bottom left of the browser window.  When this happens, it does not paint in expected location, so you will see the canvas blink each time the invalid paint in the bottom left of the window occurs.

I don't have any insight into the problem yet, but this testcase should help someone work on this.  I'm probably busy all this week, then away next week.  If nobody else has picked this up once I've got some time, I'll take a look at it.

Comment 12

[Matthew Gregan [:kinetik]]

Note that it's a standalone test case using drawWindow, so you don't need Tab Sidebar, but you will need to grant UniversalXPConnect privileges.

Comment 13

[Dave Townsend [:mossop]]

This website appears to do something that triggers it for what most be most of the drawWindow calls, and it doesn't look like it it changing the DOM I think either:

http://blip.tv/file/1229589

Comment 14

[Dave Townsend [:mossop]]

Forgot to add, that website makes it reproducible without my extension. Just have it and a couple of other tabs open then press ctrl-tab.

Comment 15

[Dave Townsend [:mossop]]

Something maybe related. At the site http://thinkprogress.org/2008/10/18/real-virginia/ The flash that is getting duplicated is in the middle left. It is getting duplicated upside down at the bottom left (same as every example). Tab Sidebar is receiving MozAfterPaint events indicating that the top left of the page has been painted though.

Comment 16

[Dave Townsend [:mossop]]

Ok the website in comment 13 is so bad not because of the site but because I was looking since bug 459244 landed. Looks like that has made this problem even worse. With a local backout that site doesn't exhibit the duplication at all.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #16)

Tha patch for bug 459244 probably just uncovered another bug.  Or to
put this another way, fixing the incorrect behavior described in bug
459244 probably just uncovered (or further uncovered) the true
cause(s) of this bug (bug 459530).

By the way, does adding in the patch for bug 459319 (on top of current
trunk code) make any difference?

Comment 18

[Dave Townsend [:mossop]]

(In reply to comment #17)
> By the way, does adding in the patch for bug 459319 (on top of current
> trunk code) make any difference?

No obvious difference

Comment 19

[Steven Michaud [:smichaud] (Retired)]

OK, the patch for bug 459244 has also made this bug very easy for me
to reproduce.  Now all I need do is install TabSidebar (using the URL
from comment #0), choose View : Sidebar : Tab Sidebar, and load any
Flash plugin into a tab (only a single tab need be open).  A mirror
image of the plugin (upside down and backwards) appears in the lower
left.  If the Flash "movie" is animated, the mirror image also
flickers annoyingly.

Here are the Flash "movies" I tested with (in addition to the one from
comment #13).  The first is static.  The second two are animated.

http://www.playercore.com/bugfiles/146162/AddReturnHTML.html
http://www.rogerdean.com
http://www.youtube.com/watch?v=TZ860P4iTaM&NR=1

This is fortunate -- it should make it easier to find the true cause
of this bug.

Sadly, though, I still can't reproduce the problem using kinetik's
testcase from comment #11.

Comment 20

[Tony Chung [:tchung]]

i can repro this easily on a clean profile (no extensions), on the mac.   Simply have one tab running a flash banner, and open a second tab and start typing.   Try http://www.nfl.com/scores another testcase.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

> Simply have one tab running a flash banner, and open a second tab
> and start typing.  Try http://www.nfl.com/scores another testcase.

This doesn't work 100% of the time for me.

But I've found that I can do the following:

1) Open http://www.nfl.com/scores.

2) Cmd-t to open another (blank) tab.

3) Wait.  Within 5 minutes (probably less) you'll see the problem.

   So you don't need to type anything.

Someone please provide an example of a Flash banner that updates
continuously.

Comment 23

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #22)
> Someone please provide an example of a Flash banner that updates
> continuously.

Would a Flash-based video count? If so, perhaps this 90-minute video from the last presidential debate would work? http://www.yagoo.ro/play.php?vid=783

Comment 24

[Steven Michaud [:smichaud] (Retired)]

> Would a Flash-based video count?

Apparently not.  Neither your example nor the YouTube video from
comment #19 (http://www.youtube.com/watch?v=TZ860P4iTaM&NR=1) "work".
Nor do the other examples from comment #19.

Interestingly, I've found a non-banner ad that *does* "work" very
well.  Go to http://www.vg.no/ and scroll down until the "Stopp
lekkasjen!" ad is displayed.  Then open another	(blank) tab and wait
less than a minute.

Unfortunately I can't find where this ad is loaded (it's not loaded in
the "View Source" page).

Even more interesting:  If I right click on the ad and choose
Settings, then disable (uncheck) hardware acceleration, the problem
stops happening.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

The "Stopp lekkansjen!" ad is a dripping faucet.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Attachment: Dripping faucet Flash ad

The dripping faucet Flash ad is at
http://flash.vg.no/annonser/2008/varmeogbad/2010/vb_lekasje_vg.swf.
Here's a copy of it.

I still haven't managed to figure out how http://www.vg.no/ is loading
it (what kind of tag it uses).

The problem I describe in comment #22 doesn't happen if you load this
ad as an *.swf file.

Comment 27

[Chris Lawson (gone)]

FWIW, I can't repro with the latest Minefield nightly and Flash 10 on PPC (with harware acceleration enabled, as it is by default). Maybe this is Flash 9- or Intel-only.

Comment 28

[Dave Townsend [:mossop]]

No I updated to flash today and still see it. Though I am on Intel.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

Attachment: Testcase with dripping faucet ad

Mstange found out how http://www.vg.no/ loads the dripping faucet
Flash ad.  Here's a semi-reduced testcase that, for me, reliably
reproduces this problem in recent mozilla-central nightlies (those
containing my patch for bug 459244).

1) Start Minefield and load the testcase.

2) Cmd-tab to open a new, blank, tab.

3) Wait 5 seconds.

   If nothing happens, cmd-w to close the blank tab and then cmd-tab
   again to open another blank tab.

   But often something *will* happen -- you'll see a mirrored image of
   the plugin (upside down and backwards) in the lower left of the
   browser page.

Whoever's listening here, please tell me if you can reproduce my STR.
I'm particularly interested in hearing from mossop, tchung and cl.

Comment 30

[Dave Townsend [:mossop]]

If you mean cmd-t to open a new tab then yes I can reproduce. I suspect that whatever code gets the thumbnails for the ctrl-tab display is probably doing so in response to a timer or something and that is what is causing the 5 second delay.

Comment 31

[Dave Townsend [:mossop]]

And boy air mozilla is really irritating because of it :(

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Yes, cmd-tab should be cmd-t.  Sorry :-(

Here's my STR from comment #29 over again:

1) Start Minefield and load the testcase from comment #29 (attachment
   343955).

2) Cmd-t to open a new (blank) tab.

3) Wait 5 seconds.

   If nothing happens, cmd-w to close the blank tab and then cmd-t
   again to open another blank tab.

   But often something *will* happen -- you'll see a mirrored image of
   the plugin (upside down and backwards) in the lower left of the
   browser page.

Comment 33

[Dave Townsend [:mossop]]

putting back blocking+

Comment 34

[Thomas K. (:tom)]

(In reply to comment #32)
> Yes, cmd-tab should be cmd-t.  Sorry :-(

If this is a Macintosh bug, why did you change the platform back to PC with OS Mac OS X? Is this a bug that only affects hackintoshes or something? I notice it on my Apple Mac, thus it's a Macintosh platform bug.

Comment 35

[Dave Townsend [:mossop]]

Although it isn't all that important, the hardware is used to differentiate between intel and ppc for Mac's. In this case it is seen on both so All is more relevant really.

Comment 36

[Chris Lawson (gone)]

(In reply to comment #35)
> In this case it is seen on both so All is more
> relevant really.

Dave, has someone actually seen this bug on PPC? I certainly can't reproduce it on PPC, and it seems everyone who's commenting here is on Intel.

Comment 37

[Dave Townsend [:mossop]]

(In reply to comment #36)
> (In reply to comment #35)
> > In this case it is seen on both so All is more
> > relevant really.
> 
> Dave, has someone actually seen this bug on PPC? I certainly can't reproduce it
> on PPC, and it seems everyone who's commenting here is on Intel.

Sorry I misread comment 27 to mean that you had seen it on ppc with Flash 9.

Comment 38

[Tony Chung [:tchung]]

(In reply to comment #29)
> Created an attachment (id=343955) [details]
> Testcase with dripping faucet ad
> 
> Mstange found out how http://www.vg.no/ loads the dripping faucet
> Flash ad.  Here's a semi-reduced testcase that, for me, reliably
> reproduces this problem in recent mozilla-central nightlies (those
> containing my patch for bug 459244).
> 
> 1) Start Minefield and load the testcase.
> 
> 2) Cmd-tab to open a new, blank, tab.
> 
> 3) Wait 5 seconds.
> 
>    If nothing happens, cmd-w to close the blank tab and then cmd-tab
>    again to open another blank tab.
> 
>    But often something *will* happen -- you'll see a mirrored image of
>    the plugin (upside down and backwards) in the lower left of the
>    browser page.
> 
> Whoever's listening here, please tell me if you can reproduce my STR.
> I'm particularly interested in hearing from mossop, tchung and cl.

I cant' repro the drawWindow issue with your testcase.  But i can repro it pretty easily with the nfl.com/scores one.  

My system:  Intel-based Mac OSX 10.5.2, running a new profile, on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081020 Minefield/3.1b2pre.

I have a PPC mac here in the lab.  i'll try that now.

Comment 39

[Tony Chung [:tchung]]

okay i take that back.  i *can* repro your upside-down dripping faucet testcase on an Intel Mac after closing/opening a new tab.

However, i *couldn't* repro this on a PPC mac osx 10.4.  Hope that helps you narrow things down.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.1b2pre) Gecko/20081020 Minefield/3.1b2pre

Comment 40

[Steven Michaud [:smichaud] (Retired)]

Using my (corrected) STR from comment #32 (which seems to be 100%
effective if followed exactly):

This bug doesn't happen on PPC 10.4.11.

It does happen on Intel 10.5.5 and 10.4.11.

It doesn't happen using a special build in which support for the
CoreGraphics drawing model in plugins has been disabled.

It happens whether or not hardware acceleration (a Flash setting) is
disabled (so what I said at the end of comment #24 is incorrect).

All my tests have (so far) been done with Apple's current Flash
version (9.0 r124).

Comment 41

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #40)
> All my tests have (so far) been done with Apple's current Flash
> version (9.0 r124).

Is not 10.0 r12 the current Flash version? 
http://www.adobe.com/products/flashplayer/

Comment 42

[Steven Michaud [:smichaud] (Retired)]

> Is not 10.0 r12 the current Flash version?

It's not *Apple's* current Flash version (the one Apple distributes via Software Update).

Comment 43

[Steven Michaud [:smichaud] (Retired)]

Actually, this is really a plugins bug (though it's OS-X-specific).

Comment 44

[Alex]

I can reproduce it quite often on PPC (10.5, Flash 10), most of the test cases posted here reproduce it.

Comment 45

[Tony Chung [:tchung]]

In addition to duplicate flash content, it's also being drawn all over the wrong place, with flash controls misplaced everywhere.

See http://video.yahoo.com

Comment 46

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #45)

Sigh, yes.  That video draws out of place (higher than it should), and
overlays the ticker (also a Flash object?).

That problem also predates my patch for bug 459244.

This is one complex bug (or rather, it's several bugs disguised as
one).  I'm slowly making progress (in fact I've got the STR I reported
in comment #32 fixed).  But over the next couple of days I'm going to
try to fix all the Flash problems at one go.

Comment 47

[Steven Michaud [:smichaud] (Retired)]

Tony, could you try to find the regression range for the problems with
http://video.yahoo.com/?  They don't happen with Firefox 3.0.3.

Comment 48

[Tony Chung [:tchung]]

Regression falls between builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081011 Minefield/3.1b2pre and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081012 Minefield/3.1b2pre

Hg: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-10-11+02%3A00%3A00&enddate=2008-10-12+03%3A00%3A00

Comment 49

[Steven Michaud [:smichaud] (Retired)]

Thanks!

The only patch in that range that seems possible (as a trigger for the problem with http://video.yahoo.com/) is bug 458111 ("Remove Mac-specific tabbrowser-tab binding").

Comment 50

[Tony Chung [:tchung]]

dao, if this is really a regression from your patch of bug 458111, please set the product/component field to what it should be.  Thanks.

Comment 51

[Steven Michaud [:smichaud] (Retired)]

> dao, if this is really a regression from your patch of bug 458111 ...

"This" is just the problem described in comment #46, not the rest of the bug (which is definitely a plugins bug).

We should eventually spin off the comment #46 problem into a new bug.

But before we do that, it'd be nice to know if I'm right (and it was your patch for bug 458111 that triggered the comment #46 problem).

Comment 52

[Dão Gottwald [:dao]]

(In reply to comment #51)
> But before we do that, it'd be nice to know if I'm right (and it was your patch
> for bug 458111 that triggered the comment #46 problem).

I don't think it could. In what way does it seem possible to you?

Comment 53

[Steven Michaud [:smichaud] (Retired)]

> In what way does it seem possible to you?

Your patch is the only Mac-specific one in the regression range from comment #48.

Comment 55

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #53)
> > In what way does it seem possible to you?
> 
> Your patch is the only Mac-specific one in the regression range from comment
> #48.

video.yahoo.com looks just as broken in the latest builds on Windows and Linux, so it's certainly not a Mac-specific issue.

Comment 56

[Steven Michaud [:smichaud] (Retired)]

> video.yahoo.com looks just as broken in the latest builds on Windows and Linux,
> so it's certainly not a Mac-specific issue.

Broken in the same way, with the same regression range?  (See comment #48.)

Comment 57

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #56)
> > video.yahoo.com looks just as broken in the latest builds on Windows and Linux,
> > so it's certainly not a Mac-specific issue.
> 
> Broken in the same way, with the same regression range?  (See comment #48.)

Yup.

Comment 58

[Steven Michaud [:smichaud] (Retired)]

> Broken in the same way, with the same regression range?

I also say yes to both questions ... at least on Linux.

Comment 59

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's a patch that fixes this bug as originally reported (and my STRs
from comment #19 and comment #32).  It turns out to be caused by a
Flash bug (CoreGraphics-specific), and my patch works around that.  A
tryserver build will follow in an hour or two.

This patch *doesn't* fix the problem discussed in comment #45 through
comment #53 and comment #55 through comment #58.  That's a completely
different/unrelated problem, which effects OS X, Windows and Linux.
In the next hour or so I'll open a new bug on it.

Comment 60

[Steven Michaud [:smichaud] (Retired)]

> This patch *doesn't* fix the problem discussed in comment #45
> through comment #53 and comment #55 through comment #58.  That's a
> completely different/unrelated problem, which effects OS X, Windows
> and Linux.  In the next hour or so I'll open a new bug on it.

I've spun this off into bug 461266.

Comment 61

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my patch:

https://build.mozilla.org/tryserver-builds/2008-10-22_15:[e-mail]-bugzilla459530/[e-mail]

I've tested it (fairly lightly) on OS X 10.5.5 (with Flash 10.0 r12)
and 10.4.11 (with Flash 9.0r124):  First I confirmed that it fixes my
STR from comment #32.  Then I loaded the dripping faucet testcase
(attachment 343955 [details]) and all the URLs from comment #19 into different
tabs, turned on View : Sidebar : TabSidebar, and played around for a
while.

I didn't see this bug even once.

Comment 62

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (clean up)

I fixed some potential leaks in
nsPluginInstanceOwner::StartPluginPaint().

I discovered that the Flash plugin can sometimes also paint (and paint
outside its frame) from an idle event.

This last caused the following STR to "work" even with my original
patch:

1) Start Minefield and load the dripping faucet "movie" from comment
   #26 (attachment 343939 [details]).

2) Cmd-t to open a new (blank) tab.

3) Click on the first tab to switch back to it.

4) Wait 2-3 seconds.

   Often the dripping faucet will be (partially) redrawn just below
   its proper location (so that it overwrites the status bar).

This STR no longer "works" with my rev1 patch.

A new tryserver build will follow soon.

Comment 63

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 344414 [details] [diff] [review]
Fix rev1 (clean up)

This patch causes SetWindow to be called *much* too often.

I'll need to revise it.

Comment 64

[Josh Aas]

"It turns out to be caused by a Flash bug (CoreGraphics-specific)"

If this is true, we should attempt to get Adobe to fix the problem before working around it in our own code. I don't think the current situation is so pressing that we couldn't wait for a fix from them. We have to live with workarounds in our code for a *long* time and we should be very careful about taking on such burdens.

Comment 65

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #64)
> "It turns out to be caused by a Flash bug (CoreGraphics-specific)"
> 
> If this is true, we should attempt to get Adobe to fix the problem before
> working around it in our own code. I don't think the current situation is so

On a potentially related note, Steven, is this CoreGraphics-specific bug present in both Flash 9 and Flash 10? 

(Supposing for a moment that it's only present in Flash 9, would it be worth targeting our workaround just for that version and subsequently not engaging the workaround if the user is running a Flash version >= 10 ?)

Comment 66

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #65)

> On a potentially related note, Steven, is this CoreGraphics-specific
> bug present in both Flash 9 and Flash 10?

It's present in both.

I think we should fix this (i.e. work around the bug).  It's a pretty
bad problem, and (now) quite easy to reproduce.  There are lots of
other product-specific workarounds in our plugin code, without which
the user experience would be considerably worse.  And a workaround
needs to stay around for a while to be useful -- even if the vendor
fixes the bug right away (very unusual), it takes a while for the fix
to appear in a release, and older releases can stay in use for years
(particularly if, as with the Flash plugin, the plugin is supplied by
someone else than the plugin vendor (in this case by Apple)).

As for my patch for bug 459244, that just unmasked the true bug.
Backing it out would just be a clumsier and stupider workaround.

I'm still working on this, and hope to have a new patch in a couple of
hours.

Comment 67

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2 (new design)

Here's a new patch, with a substantially different design.  I've
tested it against the STRs from comment #19, comment #32 and comment
#62, on OS X 10.5.5 and 10.4.11.  I didn't see any problems.

Here's a tryserver build made with my rev2 patch:

https://build.mozilla.org/tryserver-builds/2008-10-23_11:[e-mail]-bugzilla459530-rev2/[e-mail]

Please try it out and let us know your results.  I'm particularly
interested in hearing from mossop.  I won't ask for review until we
get some results from testing (besides those I've already done
myself).

Comment 68

[Tony Chung [:tchung]]

(In reply to comment #67)
> 
> Please try it out and let us know your results.  I'm particularly
> interested in hearing from mossop.  I won't ask for review until we
> get some results from testing (besides those I've already done
> myself).

At first glance, its looking good.  I can't seem to repro the issue anymore on Flash 10, mac osx 10.5.2.  I'll comment more if i see issues.

Comment 69

[Dave Townsend [:mossop]]

With the new build I am still able to reproduce using the video in comment 13 with Tab Sidebar. I can however no longer reproduce in a couple of other places. Unfortunately it looks like flash is no longer getting included with the window content painted with drawWindow calls.

Comment 70

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #69)

> Unfortunately it looks like flash is no longer getting included with
> the window content painted with drawWindow calls.

I saw that, but didn't know what to make of it.

But now I see that it works with the 2008-10-10-02 Minefield nightly
(using a static Flash plugin like
http://www.playercore.com/bugfiles/146162/AddReturnHTML.html from
comment #19).

Comment 71

[Matthew Gregan [:kinetik]]

Not sure if this adds any additional information, but I can still reproduce the problem consistently with a current trunk build and the most recent patch applied using my attached testcase (saved to a local file so it can be given the appropriate privs).

Comment 73

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch for logging changes to nsPluginWindow->window->cgPort.context (not a fix)

Comment 74

[Steven Michaud [:smichaud] (Retired)]

The plot thickens.

I've discovered that the Flash plugin (when using the CoreGraphics
drawing model) actually changes the value of a member of the
nsPluginWindow structure that gets passed to it in calls to
NPP_SetWindow() -- the window->cgPort.context member, which is a
CGContextRef.

These changes tend to happen at the same time as the problems
described in my STRs from comment #32 and comment #62 ... on Intel
boxes.  But they don't happen at all on my PPC G4 running OS X 10.4.11
(where I never see the problems that I've been able to reproduce on my
Intel boxes).

To see the changes, run the tryserver build corresponding to this
patch, and observe what gets logged to the system console.

https://build.mozilla.org/tryserver-builds/2008-10-23_21:[e-mail]-bugzilla459530-testing-rev1/

Michelle and cliss, can you tell me what's going on?

And do this bug's problems go away if you stop the Flash plugin from
changing nsPluginWindow->window->cgPort.context?

Comment 75

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #71, comment #11 and comment #12)

I didn't realize I needed to download your testcase and run it locally
-- so I wasn't able to reproduce any problems with it.

But when I do run it locally (and permanently grant it
UniversalXPConnect privileges at the prompt), I now *can* see the
problem -- though only on Intel boxes.  I also see that my rev2 patch
doesn't fix it.  And my testing build (from comment #74) shows a
constant stream of changes (by the Flash plugin) to
nsPluginWindow->window->cgPort.context while it's running.

Comment 76

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #69)

> With the new build I am still able to reproduce using the video in
> comment 13 with Tab Sidebar.

I can confirm that my rev2 patch doesn't fix this problem (when I load
that video into a tab and turn on Tab Sidebar).  And (while it's
running in Tab Sidebar) I also (using my testing build from comment
#74) see a constant stream of changes to
nsPluginWindow->window->cgPort.context.

But this is only on Intel boxes.  I don't see any problems on my PPC
G4 running OS X 10.4.11.

Comment 77

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #74)

I was wrong :-(

The Flash plugin *isn't* changing the value of
nsPluginWindow->window->cgPort.context.  Instead it's calls to
childView->GetNativeData(NS_NATIVE_PLUGIN_PORT_CG) that do this --
they always return a pointer to the same internal (private) object,
but may change the values in that object.  This object (the pointer to
it) is the "window" in nsPluginWindow->window->cgPort.context.

So this *isn't* a Flash bug, and *is* a Firefox box.

Sorry for the confusion.  I hope to have a new patch before too long.

Comment 78

[Steven Michaud [:smichaud] (Retired)]

I'm still puzzled why this problem doesn't happen on my PPC box.  It may be some kind of obscure endian issue.

Comment 79

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev3 (another new design)

This is it, finally ... at least I hope so :-)

This patch fixes my STRs from comment #19, comment #32 and comment
#62.  It also fixes the problems running the video from comment #13 in
Tab Sidebar, and those seen running kinetik's testcase locally
(attachment 343159 [details], as described in comment #75).

The only remaining problem is probably insoluble:

When you load multiple dynamic Flash plugins into multiple tabs and
view them using Tab Sidebar, those in pages that aren't currently
"active" don't update.  I believe this is because only plugins in
foreground tabs get sent idle events (aka null events) -- which (if
I'm right) is as it should be.

A tryserver build will follow shortly.

Comment 80

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my rev3 patch:

https://build.mozilla.org/tryserver-builds/2008-10-24_15:[e-mail]-bugzilla459530-rev3/[e-mail]

Comment 81

[Steven Michaud [:smichaud] (Retired)]

+        window->window->cgPort.context = cgContext;
+        mInstanceOwner->SetPluginPortChanged(PR_TRUE);
+      }
+      *pluginPortCopy = *window->window;

+#ifndef NP_NO_QUICKDRAW
+  if (drawingModel == NPDrawingModelQuickDraw) {
+    if (mPluginWindow->window->qdPort.port != mPluginPortCopy.qdPort.port)
+      mPluginPortChanged = PR_TRUE;
+  } else if (drawingModel == NPDrawingModelCoreGraphics)
+#endif
+  {
+    if ((mPluginWindow->window->cgPort.context !=
+         mPluginPortCopy.cgPort.context) ||
+        (mPluginWindow->window->cgPort.window !=
+         mPluginPortCopy.cgPort.window))
+      mPluginPortChanged = PR_TRUE;
+  }
+  mPluginPortCopy = *mPluginWindow->window;

To avoid ambiguities in how the compiler might interpret lines that
copy an entire nsPluginPort struct (which is a union), I'll replace
these chunks in my next revision with the following:

+        window->window->cgPort.context = cgContext;
+        pluginPortCopy->cgPort.context = cgContext;
+        mInstanceOwner->SetPluginPortChanged(PR_TRUE);
+      }

+#ifndef NP_NO_QUICKDRAW
+  if (drawingModel == NPDrawingModelQuickDraw) {
+    if (mPluginWindow->window->qdPort.port != mPluginPortCopy.qdPort.port) {
+      mPluginPortCopy.qdPort.port = mPluginWindow->window->qdPort.port;
+      mPluginPortChanged = PR_TRUE;
+    }
+  } else if (drawingModel == NPDrawingModelCoreGraphics)
+#endif
+  {
+    if ((mPluginWindow->window->cgPort.context !=
+         mPluginPortCopy.cgPort.context) ||
+        (mPluginWindow->window->cgPort.window !=
+         mPluginPortCopy.cgPort.window)) {
+      mPluginPortCopy.cgPort.context = mPluginWindow->window->cgPort.context;
+      mPluginPortCopy.cgPort.window = mPluginWindow->window->cgPort.window;
+      mPluginPortChanged = PR_TRUE;
+    }
+  }

Comment 82

[Dave Townsend [:mossop]]

(In reply to comment #80)
> Here's a tryserver build made with my rev3 patch:
> 
> https://build.mozilla.org/tryserver-builds/2008-10-24_15:[e-mail]-bugzilla459530-rev3/[e-mail]

Just tested this and it looks perfect. Couldn't reproduce the issue any longer in any of my tests.

Comment 83

[Tony Chung [:tchung]]

(In reply to comment #82)
> 
> Just tested this and it looks perfect. Couldn't reproduce the issue any longer
> in any of my tests.

I concur... the tryserver build seems to have fixed these drawWindow problems i saw earlier also.  

As expected, bug 461266 still exists with video.yahoo.com, but we'll track that issue over in that bug.

Nice work, smichaud.

Comment 86

[Matthew Gregan [:kinetik]]

Comment on attachment 344695 [details] [diff] [review]
Fix rev3 (another new design)

Looks great, thanks Steven!

With respect to comment #81, as far as I can tell from the standard, union-to-union assignments are legal and will do the right thing, but it may be true that the intention of the code is clearer with the proposed change.

Comment 88

[mjh]

I have been running the build from comment #80 for a day now, it appears to fix the flipped flash ghost content.

Thanks Steven!

Comment 90

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

Comment on attachment 344695 [details] [diff] [review]
Fix rev3 (another new design)

+  void SetPluginPortChanged(PRBool aState) { mPluginPortChanged = aState; }
+  nsPluginPort* GetPluginPortCopy() { return &mPluginPortCopy; }
+  nsPluginPort* SetPluginPortAndDetectChange();
+  void BeginCGPaint();
+  void EndCGPaint();

Please add comments to document these.

Comment 92

[Steven Michaud [:smichaud] (Retired)]

Landed on mozilla-central (changeset 9b4084a61cd9).

Comment 93

[Steven Michaud [:smichaud] (Retired)]

Attachment: What I landed

This is	what I actually landed.

I incorporated the changes from comment #81, added the comments roc
requested in comment #90, and got rid of the "doesn't happen in
current code" comments on Josh's recommendation.

Comment 94

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081031 Minefield/3.1b2pre.