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

RESOLVED FIXED in mozilla2.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: alice0775, Assigned: mats)

Tracking

({regression})

Trunk
mozilla2.0b8
x86
Windows 7
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Posted image 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
(Reporter)

Updated

9 years ago
blocking2.0: --- → ?
(Reporter)

Comment 1

9 years ago
As a matter of course, it happens since Bug 449734  was fixed.
Keywords: regression
(Reporter)

Updated

9 years ago
Summary: Repaint Issue , after detach/reattach a tab which contains Flash Video → Repaint Issue , after detach/reattach a tab which contains Flash Video[non Aero]
Mats, can you have a look at this regression?
Assignee: nobody → matspal
blocking2.0: ? → betaN+
potentially a dupe of bug 592626?
(Assignee)

Comment 4

9 years ago
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...
Blocks: 449734
(Reporter)

Comment 5

9 years ago
(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.
(Assignee)

Comment 6

9 years ago
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/mpalmgren@mozilla.com-7e24669beba2/
(Reporter)

Comment 7

9 years ago
(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.
(Assignee)

Comment 8

9 years ago
Posted patch wip1 (obsolete) — Splinter Review
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?
Attachment #486131 - Flags: review?(roc)
(Assignee)

Comment 10

9 years ago
No, the TryServer build is from Oct 23, bug 593839 landed Oct 25.
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).
(Assignee)

Comment 12

9 years ago
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.
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).
(Assignee)

Comment 14

9 years ago
Posted patch wip2Splinter Review
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/mpalmgren@mozilla.com-8368b9286696/
(Assignee)

Comment 15

9 years ago
It appears to have passed TryServer unit tests with only known oranges.
(Assignee)

Updated

9 years ago
Attachment #486131 - Attachment is obsolete: true
Attachment #486131 - Flags: review?(roc)
Summary: Repaint Issue , after detach/reattach a tab which contains Flash Video[non Aero] → Repaint issue after detaching/reattaching a tab that contains Flash video [non-Aero]
(Assignee)

Comment 16

9 years ago
roc, there are calls to ConfigureChildren where only the clip rect
has changed, do we need to Invalidate in that case?
That's a workaround and should be removed. See the comment // XXX - Workaround for Bug 587508
(Assignee)

Comment 18

9 years ago
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.
No, we do a SetWindowClipRegion in that case, that should do enough invalidation.
(Assignee)

Comment 20

9 years ago
http://hg.mozilla.org/mozilla-central/rev/37f63c626879
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla2.0 → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.