378830 - UI operation delay when page contains flash images with high refresh rate

Status: RESOLVED INVALID

(Core :: Widget, defect)

Description

[Wang Xianzhu]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: 

Please refer to bug 359573 for details.  Some fields of that bug are incorrect bug I can't modify it, so I open this new bug report.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Wang Xianzhu]

Attachment: A html page containing some flashes to reproduce this bug.

Comment 2

[Wang Xianzhu]

Attachment: A patch to partly resolve this problem, by making WM_PAINT and other windows normal messages have higher priority than WM_USER messages, to prevent messages from the flashes from blocking the Mozilla message loop.

Copy the original patch here

Comment 3

[Wayne Mery (:wsmwk)]

Wang, you need to request review and then checkin for your patch. See
 http://www.mozilla.org/hacking/code-review-faq.html
 http://www.mozilla.org/hacking/

Comment 4

[Wang Xianzhu]

Comment on attachment 262846 [details] [diff] [review]
A patch to partly resolve this problem, by making WM_PAINT and other windows
normal messages have higher priority than WM_USER messages, to prevent messages
from the flashes from blocking the Mozilla message loop.

Wayne, thank you very much for your telling me the correct path!

Hi, Ere,
I'd like you to review this patch.  Thanks!

Comment 5

[Ere Maijala (slow)]

Wang, unfortunately I can't reproduce the problem on my system. I'm not seeing any slowdown with the test case. This is also some fragile code. If you check the cvs history you can see that WM_USER - 1 has been there before but has since been removed. See <http://bonsai.mozilla.org/cvslog.cgi?file=/mozilla/widget/src/windows/nsAppShell.cpp&rev=HEAD&mark=3.61>, at least revisions 3.30, 3.31, 3.41.

Comment 6

[Wang Xianzhu]

Comment on attachment 262845 [details]
A html page containing some flashes to reproduce this bug.

Hi, Ere, I think you can try to reproduce this bug by opening two or more tabs with my testcase.  You can try to move the window and click right mouse button when the flashes are playing and see the effect.

I reproduced this bug on my home PC, my work notebook and many other friends' PC, only opening one tab with the case.  My home PC has built-in 945G graphics engine, 1G memory and a dual-core 2.8G CPU.

Many Chinese users suffer from this bug everyday because many hot web sites (sina news, soufun real-estates, etc.) in China favor using such flashes in their pages. This is an important reason that many Chinese users don't use FireFox, because IE can render these pages perfectly.

Comment 7

[Wang Xianzhu]

I noticed that PeekMessage(...WM_USER - 1...) was removed in 3.41, with log "Fix for 97805, r=sfraser, sr=waterson, also fixing event prioritization problems on Win32, r=jag, sr=brendan".  Then I searched bugzilla for any bugs related to "event prioritization problems on Win32" and found nothing.  I wonder what "event prioritization problems" were and why did hyatt make that change.

Comment 8

[Wayne Mery (:wsmwk)]

hyatt ^

Wang, 
Will this also fix bug 234016?  
Are you suggesting this may be a regression of bug 97805? (or is this a problem that has always existed on trunk?)
In bug 359573 you say this got much better with 3.0 - can you isolate a checkin date where it improved?

Comment 9

[Wang Xianzhu]

> Will this also fix bug 234016?  
I'm sorry for the confusion caused by some incorrect statements in the bug reports because of incomplete tests.  These incorrect statements include comment#4 in bug 359573 and comment#12 and comment#14 in bug 234016.  I can ensure that the statements on and after Dec. 29, 2006 are correct, and this bug is not related to bug 234016.

> Are you suggesting this may be a regression of bug 97805? (or is this a problem
that has always existed on trunk?)
I think the change from 3.40 to 3.41 had two unrelated objectives: one was to fix bug 97805, and one was to fix the unclear "event prioritization problems", so this bug is not related to bug 97805.  The attached patch in bug 97805 also did not contain the change of PeekMessage(...WM_USER...).

> In bug 359573 you say this got much better with 3.0 - can you isolate a checkin
date where it improved?
Sorry that statement is incorrect.  I made it because at that time I only focused on the delay of tab-switching, which no longer exists in 3.0.  Please see bug 359573 comment#8.  I have not investigated why tab-switching is not delayed in 3.0.  I guess this is related to some change of setTimeout(..,0) behavior.

Comment 11

[Wang Xianzhu]

Comment on attachment 262846 [details] [diff] [review]
A patch to partly resolve this problem, by making WM_PAINT and other windows
normal messages have higher priority than WM_USER messages, to prevent messages
from the flashes from blocking the Mozilla message loop.

I tried with only the following line added:        ::PeekMessageW(&msg, NULL, 0, WM_USER - 1, PM_REMOVE)
The result is almost the same with that when both lines were added.  It's much safer to add only the above line.

Comment 12

[Wang Xianzhu]

Does anyone care about this bug?  Does anyone care about the market share in China, the second biggest Internet market (and the first biggest in the near future)?

Comment 13

[li]

When will this code checkin? A lot of users suffering from this!

Comment 14

[Wayne Mery (:wsmwk)]

Wang, you might fare better with a developer who can reproduce the problem - this is a fundamental hurdle. You might also have others** test your build with the patch and see if the problems previously caused by wm_user-1 are still seen.

**Li suggests there are "a lot of users" affected ...I'm not in a position to say many are or not, but the bug cc and vote list, and number of duplicates does not suggest lots of users see this. But if there are, they could help toward getting this resolved.


Ere in comment #5:
> Wang, unfortunately I can't reproduce the problem on my system. I'm not seeing
> any slowdown with the test case. This is also some fragile code. If you check
> the cvs history you can see that WM_USER - 1 has been there before but has
> since been removed. See
> <http://bonsai.mozilla.org/cvslog.cgi?file=/mozilla/widget/src/windows/nsAppShell.cpp&rev=HEAD&mark=3.61>,
> at least revisions 3.30, 3.31, 3.41.

3.30 and 3.41 removed it:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsAppShell.cpp&branch=&root=/cvsroot&subdir=mozilla/widget/src/windows&command=DIFF_FRAMESET&rev1=3.40&rev2=3.41
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsAppShell.cpp&branch=&root=/cvsroot&subdir=mozilla/widget/src/windows&command=DIFF_FRAMESET&rev1=3.29&rev2=3.30

Ere, might wm_user-1 now play better given the threadmanager checkin?

Comment 15

[Wang Xianzhu]

Ere & Wayne, can't you reproduce this bug with my testcase (if not, please open 2 or more tabs)?  The testcase is just a simplified version of the situation of news.sina.com.cn (the most popular news website in China) that many Chinese users face everyday.  Chinese users did not report this bug because most of them are merely users, not developers.  I will spend some time in the coming months to let more Chinese users know this bug is being resolved here.

In the change history, I noticed that 3.31 reverted the change of 3.30 by adding back WM_USER-1 thing, and said "This change (3.30) is the reason for mozilla to freeze while loading pages (by not processing paint events while loading pages), this has made mozilla feel really **** for the last 5 weeks".  The final solution to bug 36849 didn't include the change of 3.30.

About change in 3.41, hyatt did not say the reason clearly, as mentioned in my previous comment #7 two months ago:
> I noticed that PeekMessage(...WM_USER - 1...) was removed in 3.41, with log
> "Fix for 97805, r=sfraser, sr=waterson, also fixing event prioritization
> problems on Win32, r=jag, sr=brendan".  Then I searched bugzilla for any bugs
> related to "event prioritization problems on Win32" and found nothing.  I
> wonder what "event prioritization problems" were and why did hyatt make that
> change.

Comment 16

[Wayne Mery (:wsmwk)]

I don't see the problem. But to be honest I'm not completely sure I understand what the problem is.  what do you mean by "high refresh rate"?

What version of flash player are you running? 
http://www.adobe.com/products/flash/about/ will show the version

and can you try a newer version, documented in Bug 341380 per Michelle Sintov <[e-mail]> ...
This bug should be fixed with Flash Player 9 Update 3. A beta version,
9.0.60.120, can be downloaded here:
http://labs.adobe.com/downloads/flashplayer9.html

Comment 17

[Wang Xianzhu]

I have described some details of this bug in previous messages (and bug 359573), but because of my poor English, they might seem not very clear.

A flash with "high refresh rate" means that the flash is designed with high number of frames-per-second, such as 20 frame/s or above.

I reproduced this bug in many different environments and I have not seen a single exception.  The steps to reproduce this bug:
1. Open 1 to 5 tabs with my test case attached;
2. Try some UI operations:
 a) on 3.0, all non-client mouse operations are delayed several to tens of seconds, such as moving or resizing the browser window by dragging the title bar or window border; switching to another window and then switching back by clicking the title bar, etc.
 b) on Firefox 2.0, much more UI operations are delayed, such as switching tabs by clicking the tab title, etc.

Thank you very much for letting me know bug 341380 which is actually almost the same problem of but different aspects of this bug.

My original flash plugin version was 9.0.45.0.  I just tried the new beta.  It performs better, but if I open more tabs (3 or above), the flashes still eat 100% CPU and the UI delay is still obvious.  However, with my patch, Firefox works much better with both 9.0.45.0 and beta versions of flash plugin.

Now I think fixing this bug for Firefox 2.0 is much more useful because it can greatly improve the user experience of existing users.

Do you have some idea about my doubt in Comment #15?  From the history, it seems more reasonable to add back WM_USER-1 than to remove it.

Comment 18

[Wayne Mery (:wsmwk)]

Mihail, Michele, do you see the high cpu usage of attachment 262845 [details] using the beta flash player?


(In reply to comment #17)
>... The steps to reproduce this bug:
> 1. Open 1 to 5 tabs with my test case attached;
> 2. Try some UI operations:
>  a) on 3.0, all non-client mouse operations are delayed several to tens of
> seconds, such as moving or resizing the browser window by dragging the title
> bar or window border; switching to another window and then switching back by
> clicking the title bar, etc.
>  b) on Firefox 2.0, much more UI operations are delayed, such as switching tabs
> by clicking the tab title, etc.
> 
> Thank you very much for letting me know bug 341380 which is actually almost the
> same problem of but different aspects of this bug.
> 
> My original flash plugin version was 9.0.45.0.  I just tried the new beta.  It
> performs better, but if I open more tabs (3 or above), the flashes still eat
> 100% CPU and the UI delay is still obvious.  However, with my patch, Firefox
> works much better with both 9.0.45.0 and beta versions of flash plugin.
> ... 
> Do you have some idea about my doubt in Comment #15?  From the history, it
> seems more reasonable to add back WM_USER-1 than to remove it.

I don't doubt the effect. But I'm not a windows guru, so I can't say whether this is the best path.  Consider these questions:
* would your patch have the effect of masking a flash and/or other problems? 
* should high cpu usage in flash (or whatever) cause UI problems**?
* in light of the above, does the patch actually fix something that should be fixed?

**some perhaps relevant bugs: bug 331520, bug 291740, bug 91351, bug 384323


Perhaps Michele, Ere or another moz developer could comment on the questions.

Comment 19

[Michelle Sintov]

Hi Wang, Thanks for the bug report. Does this bug look similar to bug 379530 to you?

I do see the high cpu and the slow UI responsiveness when I open the HTML attachment on 3 tabs in Win Firefox 2.0 and try to switch between the tabs or to a  tab without Flash content.

Comment 20

[Wang Xianzhu]

(In reply to comment #18)
> I don't doubt the effect. But I'm not a windows guru, so I can't say whether
> this is the best path.  Consider these questions:
> * would your patch have the effect of masking a flash and/or other problems? 
> * should high cpu usage in flash (or whatever) cause UI problems**?
> * in light of the above, does the patch actually fix something that should be
> fixed?
> 
> **some perhaps relevant bugs: bug 331520, bug 291740, bug 91351, bug 384323
> 
Thanks for your information.  I think the following information may be relevant to your questions:

1. The flash plugin issues high frequency WM_USER+1 messages to toplevel hidden windows of class "SWFlash_PlaceholderX" titled "DummyWindowless".  The frequency may be as high as 100+ per second.

2. These WM_USER+1 messages seem to have higher priority than other Windows messages.  Although keyboard/IME/mouse messages are peeked first, other messages like non-client mouse messages (used to move or resize the window, etc.), timer messages, are not.  Thus the message loop is stuck by these WM_USER+1 messages, and has little chance to process other normal Windows messages (other than keyboard/IME/mouse).

3. Messages after WM_USER are not used by Firefox itself, so lower the priority of them has no effect on Firefox.

Comment 21

[Wang Xianzhu]

(In reply to comment #19)
> Hi Wang, Thanks for the bug report. Does this bug look similar to bug 379530 to
> you?
> 
Yes, I think they are the same problem.

Comment 22

[Wang Xianzhu]

(In reply to comment #20)
> 3. Messages after WM_USER are not used by Firefox itself, so lower the priority
> of them has no effect on Firefox.
I mean "has no bad effect on Firefox" :)

Comment 23

[Ere Maijala (slow)]

Comment on attachment 262846 [details] [diff] [review]
A patch to partly resolve this problem, by making WM_PAINT and other windows
normal messages have higher priority than WM_USER messages, to prevent messages
from the flashes from blocking the Mozilla message loop.

Wang, sorry for the slow answer (holidays...). I'm able to reproduce with more than one tab. Please create a new patch that:

1. Adds only ::PeekMessageW(&msg, NULL, 0, WM_USER - 1, PM_REMOVE)
2. Removes ::PeekMessageW(&msg, NULL, 0, 0, PM_REMOVE) after that because it doesn't have any effect there
3. Documents this change properly (change the comment to something like "... keyboard, mouse and non-user messages" and refer to this bug)

We can then try it in trunk and see if it improves the situation. The documentation is vital so that we don't end up in the same situation as we're now not really knowing why a certain change has been made.

You'll need to ask for superreview from one of the eligible sr'ers (see http://www.mozilla.org/hacking/reviewers.html).

Comment 24

[Wang Xianzhu]

Attachment: new patch adding only the line containing WM_USER-1.

Hi, Ere, I just regeneration the patch and attached it here.
We can't remove ::PeekMessageW(&MSG, NULL, 0, 0, PM_REMOVE) because we need it to deal with messages greater than WM_USER-1.

Comment 25

[Ere Maijala (slow)]

Comment on attachment 271824 [details] [diff] [review]
new patch adding only the line containing WM_USER-1.

True, my bad. Please add a reference to this bug too. With that, r=me.

Comment 26

[Wang Xianzhu]

Thanks, Ere.  Do I need to re-generete the patch or just wait for sr and let the final committer add the reference to this bug?

Comment 27

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

Comment on attachment 271824 [details] [diff] [review]
new patch adding only the line containing WM_USER-1.

Please post the final patch with the bug number, so someone else can check it in. And add [checkin needed] to the Bugzilla status whiteboard. You'll also get the patch checked in faster if you join IRC at #developers on irc.mozilla.org.

Comment 28

[Wang Xianzhu]

Attachment: Final patch diff to latest 3.62 and bug reference added

Comment 29

[Nickolay_Ponomarev]

This hasn't actually been checked in, so reopening.

Comment 30

[Nickolay_Ponomarev]

Checking in nsAppShell.cpp;
/cvsroot/mozilla/widget/src/windows/nsAppShell.cpp,v  <--  nsAppShell.cpp
new revision: 3.63; previous revision: 3.62
done

Comment 31

[Stuart Parmenter]

Backed out due to pageload regression.

Comment 32

[Michelle Sintov]

Should this be duped to bug 379530?

Comment 33

[Wayne Mery (:wsmwk)]

(In reply to comment #32)
> Should this be duped to bug 379530?

or perhaps vice versa. additional candidates: bug 312294 and Bug 355818 

Comment 34

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

So one question here is whether the performance regression (around 10% on the talos pageload tests) is a regression in what we're testing that doesn't really affect users, or whether there's a real user-perceivable performance regression.  (Or whether it's actually an improvement to user-perceived performance.)

Anybody have any evidence for one of the above?

Comment 35

[Radek 'sysKin' Czyz]

I can reproduce it if I open testcase in two-three tabs. Which nightly build had the patch in?

Comment 36

[Radek 'sysKin' Czyz]

I mean "hourly" of course :/

Comment 37

[Wayne Mery (:wsmwk)]

(In reply to comment #35)
> I can reproduce it if I open testcase in two-three tabs. 

Radek take your pick, it should be in any of the nightlies before the patch was backed out, namely 2007-07-18, 2007-07-17, 2007-07-16, 2007-07-15. 

Comment 38

[Wang Xianzhu]

This problem also exists in Linux version. I believe The root cause is the bad performance of the flash plugin. Changing the priority of messages only makes the browser not look dead. The browser still uses much higher CPU than IE displaying the same page containing high-refresh-rate flash objects.

Comment 39

[Wang Xianzhu]

I just developed a plugin container in GGL project. I used a method that can partly resolve the problem: For windowless flash plugins, we can reduce the redraw rate at the container side by delaying some NPN_InvalidateRect() calls from the plugin and combining multiple of them into one GraphicsExpose event. An interval timer checks for delayed pending queue draws. The amount of time to delay can be adjusted at runtime based on how the plugin consumes CPU.

Comment 40

[Mats Palmgren (inactive)]

We're in the process of removing support for plugins (bug 1677160) and bug 1687239 has removed the relevant Layout code, so this bug is irrelevant now.