Closed
Bug 158927
Opened 22 years ago
Closed 22 years ago
[Quicktime] Plugin control drawing broken
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.0.1
People
(Reporter: bnesse, Assigned: sfraser_bugs)
References
()
Details
(Keywords: regression, topembed+, Whiteboard: [adt1 RTM] [PL2:NA] [on the trunk] [ETA 07/30])
Attachments
(1 file, 2 obsolete files)
6.12 KB,
patch
|
wtc
:
review+
asa
:
approval+
|
Details | Diff | Splinter Review |
Sometime between the 2002071513 and 2002071605 branch builds we stopped giving the system time while clicking on plugin controls. Go to the link above and click on the movies volume control... notice that you get a white rectangle, but no control image. Now, click on the movie controller and drag it to the right. Notice that the movie updates, but the scrollbar doesn't until you release the mouse. There were only 2 checkins to the branch during this window: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaBranchTinderboxAll&branch=MOZILLA_1_0_0_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=07%2F15%2F2002+08%3A00%3A00&maxdate=07%2F16%2F2002+08%3A00%3A00&cvsroot=%2Fcvsroot Unfortunately, I think sfrasers WNE patch is the obvious candidate here... :(
Updated•22 years ago
|
Comment 1•22 years ago
|
||
assigning to peterl for review Simon: can you verify that your patch is not the culprit?
Assignee: beppe → peterl
Priority: -- → P2
Whiteboard: [adt1 RTM] [ETA Needed] → [PL2:NA][adt1 RTM] [ETA Needed]
Assignee | ||
Comment 2•22 years ago
|
||
bnesse said that backing out my patch fixes it, so I guess it is me. He's looking at how this might have changed the way that we feed null events to plugins.
Priority: P2 → --
Reporter | ||
Comment 3•22 years ago
|
||
Backing out Simon's patch does fix the problem... trying to identify the interaction which is causing the problem.
Keywords: regression
Reporter | ||
Comment 4•22 years ago
|
||
It appears that Simon's patch causes the plugin "idle event" timer to not get fired while the mouse is down.
Comment 5•22 years ago
|
||
-->sfraser Since plugin idle events are just using an nsITimer, I wonder if other timers like those used in setTimeout are having problems while the mouse is down?
Assignee: peterl → sfraser
Reporter | ||
Comment 6•22 years ago
|
||
This simple addition to Simon's previous patch fixes the problem.
Assignee | ||
Comment 7•22 years ago
|
||
Ick, I don't like that patch. Why does holding down the mouse button make a difference? Try hacking in nsMacMessagePump::GetEvent() to find out why.
Reporter | ||
Comment 8•22 years ago
|
||
Hmm, I was wrong about the "idle event" timer. I just realized that it doesn't fire with my patch either (but the controls do draw.) So it's not directly related to that...
Reporter | ||
Comment 9•22 years ago
|
||
nsMacMessagePump::GetEvent() gets called with the same frequency as nsIPluginInstanceOwner::Notify(), with or without my change. I don't think we're going to find the culprit there either.
Assignee | ||
Comment 10•22 years ago
|
||
As far as I can see, our event-based interaction with the plugin is exactly the same with or without my NSPR patch. I think we're going too have to work with Apple on this one.
Assignee | ||
Comment 11•22 years ago
|
||
I looked at this some more with Brian Nesse today. We used Thread Viewer to see what the QuickTime plugin is doing while clicking on its controls, and we now have a theory as to why this happens. When clicking on the volume control or slider, the plugin has a loop something like this: while (StillDown()) { // idle and/or handle events in the controller ... // redraw the controller MPDelayUntil(); // sleep for about 20ms } StillDown() calls EventAvail(), which in turn calls RunEventLoopUntilEventArrives(). On this run loop is an observer, which flushes window buffers to screen (recall that OS X windows are buffered). It's this window buffer flushing that is failing to happen in this bug. The controls are drawing, but their window buffers are not being flushed. Another point here is that this StillDown() loop blocks; we never get back to the application's main event loop during this time, nor do we do any NSPR thread switching. Thus, the NSPR timer task that does thread scheduling starts calling WakeUpProcess() every time, because we know we want to switch threads, but haven't had a chance to yet. The problem seems to occur because all those WakeUpProcess() calls do something to the StillDown() event loop that stops window buffer flushing from happening. Playing with the NSPR timer task interval shows that calling WNE more often than about every 20ms is enough to interfere with back buffer flushing.
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•22 years ago
|
||
A possible solution here is to stop the Timer task from calling WakeUpProcess() when NSPR is not apparently getting any time to switch threads (which indicates that we're blocked for an extended period of time). I'll attach a patch which tries to do this.
Assignee | ||
Comment 13•22 years ago
|
||
Index: pr/include/md/_macos.h =================================================================== +#include <DriverServices.h> Required for UpTime() call. struct _MDCPU { - PRInt8 notused; + AbsoluteTime lastThreadSwitch; + AbsoluteTime lastWakeUpProcess; + PRBool trackScheduling; }; In the CPU (of which there will only every be 1 for Mac CFM NSPR), we keep track of whether we need to do timing stuff (trackScheduling, which is TRUE only on Mac OS X), the last time we did a PR_Schedule (lastThreadSwitch), and the last time we called WakeUpProcess() from the timer task (lastWakeUpProcess). -#define _MD_INIT_RUNNING_CPU(cpu) +#define _MD_INIT_RUNNING_CPU(cpu) _MD_InitCPU(cpu) We need to have an _MD_InitCPU() so that we can initialize the above flags. PR_BEGIN_MACRO \ PR_ASSERT(_thread->no_sched); \ if (!setjmp(_thread->md.jb)) { \ _MD_SET_LAST_THREAD(_thread); \ + if (_PR_MD_CURRENT_CPU()->md.trackScheduling) \ + _PR_MD_CURRENT_CPU()->md.lastThreadSwitch = UpTime(); \ _PR_Schedule(); \ } else { \ PR_ASSERT(_MD_LAST_THREAD() !=_MD_CURRENT_THREAD()); \ _MD_LAST_THREAD()->no_sched = 0; \ This is the _MD_INIT_CONTEXT() macro. Here, we store the current time (using the very fast UpTime() call) in cpu->md.lastThreadSwitch. (I've aligned the \ in the non -w patch). -#define kMacTimerInMiliSecs 8L This was simply moved to macthr.c where it belongs. Index: pr/src/md/mac/macthr.c =================================================================== +#include <Math64.h> For UnsignedWideToUInt64(). +#define kMacTimerInMiliSecs 8L There is it. - if (gTimeManagerTaskDoesWUP) + if (gTimeManagerTaskDoesWUP) { + // We only want to call WakeUpProcess if we know that NSPR has managed to switch threads + // since the last call, otherwise we end up spewing out WakeUpProcess() calls while the + // application is blocking somewhere. This can interfere with events loops other than + // our own (see bug 158927). + if (UnsignedWideToUInt64(cpu->md.lastThreadSwitch) > UnsignedWideToUInt64(cpu->md.lastWakeUpProcess)) + { WakeUpProcess(&gApplicationProcess); + cpu->md.lastWakeUpProcess = UpTime(); + } + } Here's the meat. It basically says to only call WakeUpProcess() if we've switched threads more recently than we called WakeUpProcess(). In other words, if we haven't switched threads since the last time, do nothing. It then stores the time. + +void _MD_InitCPU(_PRCPU* cpu) +{ + if (RunningOnOSX()) { + AbsoluteTime zeroTime = {0, 0}; + cpu->md.trackScheduling = PR_TRUE; + cpu->md.lastThreadSwitch = zeroTime; + cpu->md.lastWakeUpProcess = zeroTime; } } Here we set up the CPU. We only need to do all this stuff on Mac OS X. Note that we need the 'trackScheduling' flag, because we don't want to call UpTime() on all OS versions; it will crash on non-Nubus machines that don't have DriverServicesLib (as we found with SleepQInstall in the JS code).
Comment 14•22 years ago
|
||
Comment on attachment 92816 [details] [diff] [review] Patch to avoid calling WakeUpProcess from the timer task when we're not processing NSPR threads r=sdagley If anyone ever wonders why we want to drop CFM support for Mozilla bugs like this should do it (assuming of course the native pthreads we should get in mach-o wouldn't encounter this problem :-)
Attachment #92816 -
Flags: review+
Comment 15•22 years ago
|
||
Comment on attachment 92816 [details] [diff] [review] Patch to avoid calling WakeUpProcess from the timer task when we're not processing NSPR threads >+#define _MD_INIT_RUNNING_CPU(cpu) _MD_InitCPU(cpu) Please rename it _MD_InitRunningCPU. >+void _MD_InitCPU(_PRCPU* cpu) >+{ >+ if (RunningOnOSX()) { >+ AbsoluteTime zeroTime = {0, 0}; >+ cpu->md.trackScheduling = PR_TRUE; >+ cpu->md.lastThreadSwitch = zeroTime; >+ cpu->md.lastWakeUpProcess = zeroTime; > } > } Should 'lastThreadSwitch' and 'lastWakeUpProcess' be initialized with an UpTime() call?
Assignee | ||
Comment 16•22 years ago
|
||
> Please rename it _MD_InitRunningCPU. Will do. > Should 'lastThreadSwitch' and 'lastWakeUpProcess' be > initialized with an UpTime() call? 'lastThreadSwitch', yes, since this indicates that we've been running NSPR threads reacently. 'lastWakeUpProcess' should probably be initialized to zero, so that the first time we go into the timer task, (lastWakeUpProcess < lastThreadSwitch) and we call WakeUpProcess.
Comment 17•22 years ago
|
||
I don't believe initialization of 'lastThreadSwitch' matters much since it's set on each thread switch but for the sake of clarity sure.
Assignee | ||
Comment 18•22 years ago
|
||
I tested pageload numbers with this patch. I got an ave. time of 1100ms with the patch, and 1090 with the current (broken) branch build. Recall that before the patch that regressed this (with slow cert auth), we were getting 1200ms pageload numbers. I also checked that this patch does not regress the certificate auth problem. Alas, it doesn't affect the IMAP download issue.
Assignee | ||
Comment 19•22 years ago
|
||
New patch addresses wtc's comments, and shows whitespace changes.
Attachment #92442 -
Attachment is obsolete: true
Attachment #92816 -
Attachment is obsolete: true
Comment 20•22 years ago
|
||
Comment on attachment 92840 [details] [diff] [review] Updated patch with whitespace changes, and more context r=wtc.
Attachment #92840 -
Flags: review+
Updated•22 years ago
|
Keywords: adt1.0.1
Whiteboard: [PL2:NA][adt1 RTM] [ETA Needed] → [adt1 RTM] [PL2:NA] [ETA 07/29]
Reporter | ||
Comment 21•22 years ago
|
||
One thing I don't see here... cpu->md.trackScheduling = PR_FALSE; if we aren't running under OS X. is the 'struct _MDCPU' guaranteed to be initialized to 0 at some point?
Comment 22•22 years ago
|
||
Brian, That's a good question. I checked that when I reviewed the patch. struct _MDCPU is contained in a _PRCPU structure. _PRCPU is created by the _PR_CreateCPU() function with a PR_NEWZAP call, which zeros the newly created structure. (See mozilla/nsprpub/pr/src/threads/combined/prucpu.c.) It may be a good idea to add cpu->md.trackScheduling = PR_FALSE; anyway so that the future maintainers of NSPR won't need to do this check again.
Reporter | ||
Comment 23•22 years ago
|
||
Or at least add a comment indicating that this is the case...
Assignee | ||
Comment 24•22 years ago
|
||
I was relying on the PR_NEWZAP() to zero out that field. I can add an explicit initializeation to false; something like: + cpu->md.trackScheduling = RunningOnOSX(); + if (cpu->md.trackScheduling) { + AbsoluteTime zeroTime = {0, 0}; + cpu->md.lastThreadSwitch = UpTime(); + cpu->md.lastWakeUpProcess = zeroTime; }
Comment 25•22 years ago
|
||
Comment on attachment 92840 [details] [diff] [review] Updated patch with whitespace changes, and more context a=asa (on behalf of drivers) for checkin to 1.1
Attachment #92840 -
Flags: approval+
Assignee | ||
Comment 26•22 years ago
|
||
Fix checked in on the NSPRPUB_PRE_4_2_CLIENT_BRANCH branch and the NSPR tip.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 27•22 years ago
|
||
shrir, can you verify this on the trunk?
Comment 28•22 years ago
|
||
fix looks good on 0729 trunk on os x. no side effects observed.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 29•22 years ago
|
||
In verifying this bug, please test: 1. Plugin fuctionality during click-hold operations (e.g. dragging things around) 2. https 3. Mail over SSL 4. Certificate authentication performance (bug 139802).
Comment 30•22 years ago
|
||
adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending and verification of the tests sfraser requested by QA. pls check this in asap, then chang "fixed1.0.1" to "verified1.0.1".
Whiteboard: [adt1 RTM] [PL2:NA] [ETA 07/29] → [adt1 RTM] [PL2:NA] [on the trunk] [ETA 07/30]
Comment 31•22 years ago
|
||
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Assignee | ||
Comment 32•22 years ago
|
||
Checked in on the MOZILLA_1_0_BRANCH.
Keywords: mozilla1.0.1+ → fixed1.0.1
Comment 33•22 years ago
|
||
3. Mail over SSL - SSL IMAP is no slower than without SSL 4. Certificate authentication performance (bug 139802). Works fine.
Comment 34•22 years ago
|
||
plugin functionality on trunk /object drag n drop tests worked fine.
Comment 35•22 years ago
|
||
verifido on 0823 branch on osx. Things are merry!
Keywords: fixed1.0.1 → verified1.0.1
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•