605928 - Repaint issue after detaching/reattaching a tab that contains Flash video [non-Aero]

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Alice0775 White]

Attachment: screenshot

Build Identifier: 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101020 Firefox/4.0b8pre ID:20101020042802

After having used a panorama once.
Repaint Issue happens, after detach/reattach a tab.

On/Off of "Use hardware acceleration when available" does not matter.

1. Start Minefield with new profile
2. Open more tah 2 tabs and URL ( http://www.youtube.com/watch?v=FEm8PZ_lUh8 )
3. Wait till playback is over
4. Enter Panorama and exit Panorama
5. Tear off the tab
6. Re attach the tab

Actual Results:
 Repaint Issue happens

Comment 1

[Alice0775 White]

As a matter of course, it happens since Bug 449734  was fixed.

Comment 2

[Johnny Stenback (:jst)]

Mats, can you have a look at this regression?

Comment 3

[Jim Mathies [:jimm]]

potentially a dupe of bug 592626?

Comment 4

[Mats Palmgren (inactive)]

I don't think this is related to bug 592626.  I think it's a regression
from bug 449734 as Alice said.
It's quite hard to reproduce, but I do see it on both WinXP and Win7
with different hardware.  I can only reproduce the problem with
dom.ipc.plugins.enabled = true, Alice can you confirm?

If I remove the "bounds.Size() != configuration.mBounds.Size())"
optimisation on line 6900:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#6880
so that we get an unconditional call to Resize (which calls Invalidate),
then it seems to work.
(I did work around this in one of the early patches for bug 449734 IIRC)
I'll hack something up and produce a TryServer build for testing...

Comment 5

[Alice0775 White]

(In reply to comment #4)
> It's quite hard to reproduce, but I do see it on both WinXP and Win7
> with different hardware.  I can only reproduce the problem with
> dom.ipc.plugins.enabled = true, Alice can you confirm?
> 
 On/Off of "Use hardware acceleration when available" does not matter.
 as i said in comment #0.

This easy happens after having used Panorama once.

Comment 6

[Mats Palmgren (inactive)]

There should be a TryServer build for testing on Windows in a few hours or so:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-7e24669beba2/

Comment 7

[Alice0775 White]

(In reply to comment #6)
I tried the tryserver-win32 build on Windows 7 and XP, and the problem does not seem to happen anymore.

Comment 8

[Mats Palmgren (inactive)]

Attachment: wip1

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#6880

We get several calls to nsWindow::ConfigureChildren after the swapping
of docshells is done, so the code *should* work as is...
The are 2-3 calls which triggers Resize, the last has correct position
and size, which should be enough.  Then a couple of calls with same size
but a different clip area which we currently optimise away - this seems
like a bug.  If I add code to detect this (which is easy since
SetWindowClipRegion already has code for that) and trigger Invalidate
it still does not fix the bug.  It seems the only thing that helps
is an unconditional Invalidate.

There's an XXX comment in ConfigureChildren saying there's unknown
bugs affecting this code, referring to bug 587508.  Maybe this is
why the Resize isn't enough in this bug?

Is this patch an acceptable wallpaper from a performance POV?

Comment 9

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

Does your build have the fix for bug 593839?

Comment 10

[Mats Palmgren (inactive)]

No, the TryServer build is from Oct 23, bug 593839 landed Oct 25.

Comment 11

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

OK, I suggest you update to include that fix and retest your attempts to fix this bug, since I think 593839 fixed various crazy issues with windowed plugin drawing (including the stuff referred to by bug 587508).

Comment 12

[Mats Palmgren (inactive)]

The bug still occurs with an up-to-date build.  I also re-tested various
workarounds with the same results - only an unconditional Invalidate seems
to fix it :(

(This time I also tried scheduling an Invalidate off an event, but it
didn't help.)

Fwiw, the bug occurs with or without direct2d enabled.
The bug only occurs with OOPP enabled (so we could wrap the call
to Invalidate with #ifdef MOZ_IPC).

Debugging this is hard without a replay VM.

Comment 13

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

I suggested to Mats we remove the PrintWindow code from nsObjectFrame, I suspect that will fix this. That code is simply no longer ever wanted with OOPP plugins (and there's no point in keeping it just for inprocess plugins).

Comment 14

[Mats Palmgren (inactive)]

Attachment: wip2

roc, this is what you suggested, right?  Removing this block fixed it for me.
I've submitted it to TryServer, builds should be available for test in a few
hours:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-8368b9286696/

Comment 15

[Mats Palmgren (inactive)]

It appears to have passed TryServer unit tests with only known oranges.

Comment 16

[Mats Palmgren (inactive)]

roc, there are calls to ConfigureChildren where only the clip rect
has changed, do we need to Invalidate in that case?

Comment 17

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

That's a workaround and should be removed. See the comment // XXX - Workaround for Bug 587508

Comment 18

[Mats Palmgren (inactive)]

No, I mean when the window size and position is the same, then we'll
fall through without doing anything, even though the clip is different.

Comment 19

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

No, we do a SetWindowClipRegion in that case, that should do enough invalidation.

Comment 20

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/37f63c626879