Closed Bug 158927 Opened 22 years ago Closed 22 years ago

[Quicktime] Plugin control drawing broken

Categories

(Core Graveyard :: Plug-ins, defect)

PowerPC
macOS
defect
Not set
major

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)

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... :(
Blocks: 143047
Keywords: nsbeta1+, topembed
Whiteboard: [adt1 RTM] [ETA Needed]
Target Milestone: --- → mozilla1.0.1
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]
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 → --
Backing out Simon's patch does fix the problem... trying to identify the
interaction which is causing the problem.
Keywords: regression
It appears that Simon's patch causes the plugin "idle event" timer to not get
fired while the mouse is down.
-->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
Attached patch Possible patch (obsolete) — Splinter Review
This simple addition to Simon's previous patch fixes the problem.
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.
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...
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.
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.
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
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.
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 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 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?
> 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.
I don't believe initialization of 'lastThreadSwitch' matters much since it's set
on each thread switch but for the sake of clarity sure.
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.
New patch addresses wtc's comments, and shows whitespace changes.
Attachment #92442 - Attachment is obsolete: true
Attachment #92816 - Attachment is obsolete: true
Comment on attachment 92840 [details] [diff] [review]
Updated patch with whitespace changes, and more context

r=wtc.
Attachment #92840 - Flags: review+
Keywords: adt1.0.1
Whiteboard: [PL2:NA][adt1 RTM] [ETA Needed] → [adt1 RTM] [PL2:NA] [ETA 07/29]
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?
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.
Or at least add a comment indicating that this is the case...
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 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+
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
shrir,  can you verify this on the trunk?
fix looks good on 0729 trunk on os x. no side effects observed.
Status: RESOLVED → VERIFIED
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).
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]
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Checked in on the MOZILLA_1_0_BRANCH.
3. Mail over SSL - SSL IMAP is no slower than without SSL
4. Certificate authentication performance (bug 139802).  Works fine.

plugin functionality on trunk /object drag n drop tests worked fine.
verifido on 0823 branch on osx. Things are merry!
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: