bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

REOPENED
Unassigned

Status

()

Core
Widget
--
major
REOPENED
11 years ago
5 years ago

People

(Reporter: Wang Xianzhu, Unassigned)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 262845 [details]
A html page containing some flashes to reproduce this bug.
(Reporter)

Comment 2

11 years ago
Created 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.

Copy the original patch here

Comment 3

11 years ago
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/
Blocks: 359573
(Reporter)

Comment 4

11 years ago
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!
Attachment #262846 - Attachment description: A patch to partly resolve this problem → 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.
Attachment #262846 - Flags: review?(emaijala)

Comment 5

11 years ago
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.
(Reporter)

Comment 6

11 years ago
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.
Attachment #262845 - Attachment description: The html to play the above flashes, to reproduce this bug (flashes are linked to the original bug) → A html page containing some flashes to reproduce this bug.
(Reporter)

Comment 7

11 years 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 8

11 years ago
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?
Assignee: win32 → nobody
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: intl
QA Contact: ian → win32
(Reporter)

Comment 9

11 years ago
> 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.
(Reporter)

Updated

11 years ago
Duplicate of this bug: 359573
(Reporter)

Comment 11

11 years ago
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.
Attachment #262846 - Flags: superreview?(emaijala)
Attachment #262846 - Flags: review?
(Reporter)

Comment 12

11 years ago
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

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

Comment 14

11 years ago
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?
(Reporter)

Comment 15

11 years ago
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

11 years ago
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 <msintov@macromedia.com> ...
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
(Reporter)

Comment 17

11 years ago
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

11 years ago
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.
Keywords: intl
Summary: UI operation delay when page contains flashes with high refresh rate → UI operation delay when page contains flash images with high refresh rate

Comment 19

11 years ago
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.
(Reporter)

Comment 20

11 years ago
(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.
(Reporter)

Comment 21

11 years ago
(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.
(Reporter)

Comment 22

11 years ago
(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

11 years ago
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).
Attachment #262846 - Attachment is obsolete: true
Attachment #262846 - Flags: superreview?(emaijala)
Attachment #262846 - Flags: review?(emaijala)
Attachment #262846 - Flags: review?
(Reporter)

Comment 24

11 years ago
Created attachment 271824 [details] [diff] [review]
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.
Attachment #271824 - Flags: superreview?(roc)
Attachment #271824 - Flags: review?(emaijala)

Comment 25

11 years ago
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.
Attachment #271824 - Flags: review?(emaijala) → review+
(Reporter)

Comment 26

11 years ago
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 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.
Attachment #271824 - Flags: superreview?(roc) → superreview+
(Reporter)

Comment 28

11 years ago
Created attachment 272011 [details] [diff] [review]
Final patch diff to latest 3.62 and bug reference added
Attachment #271824 - Attachment is obsolete: true
(Reporter)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]

Comment 29

11 years ago
This hasn't actually been checked in, so reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

11 years ago
Assignee: nobody → phnixwxz1
Status: REOPENED → NEW
Keywords: checkin-needed
Whiteboard: [checkin needed]
Version: unspecified → Trunk

Comment 30

11 years ago
Checking in nsAppShell.cpp;
/cvsroot/mozilla/widget/src/windows/nsAppShell.cpp,v  <--  nsAppShell.cpp
new revision: 3.63; previous revision: 3.62
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta1

Comment 31

11 years ago
Backed out due to pageload regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 32

11 years ago
Should this be duped to bug 379530?

Comment 33

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

or perhaps vice versa. additional candidates: bug 312294 and Bug 355818 
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

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

Comment 36

11 years ago
I mean "hourly" of course :/

Comment 37

11 years ago
(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. 
(Reporter)

Comment 38

10 years ago
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.
Assignee: phnixwxz1 → nobody
Component: Widget: Win32 → Widget
OS: Windows XP → All
Target Milestone: mozilla1.9alpha8 → psm1.3
(Reporter)

Comment 39

10 years ago
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.

Updated

10 years ago
Target Milestone: psm1.3 → ---
QA Contact: win32 → general
You need to log in before you can comment on or make changes to this bug.