158927 - [Quicktime] Plugin control drawing broken

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Brian Nesse (gone)]

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... :(

Comment 1

[rubydoo123]

assigning to peterl for review

Simon: can you verify that your patch is not the culprit?

Comment 2

[Simon Fraser [no longer active]]

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.

Comment 3

[Brian Nesse (gone)]

Backing out Simon's patch does fix the problem... trying to identify the
interaction which is causing the problem.

Comment 4

[Brian Nesse (gone)]

It appears that Simon's patch causes the plugin "idle event" timer to not get
fired while the mouse is down.

Comment 5

[Peter Lubczynski]

-->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?

Comment 6

[Brian Nesse (gone)]

Attachment: Possible patch

This simple addition to Simon's previous patch fixes the problem.

Comment 7

[Simon Fraser [no longer active]]

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.

Comment 8

[Brian Nesse (gone)]

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...

Comment 9

[Brian Nesse (gone)]

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.

Comment 10

[Simon Fraser [no longer active]]

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.

Comment 11

[Simon Fraser [no longer active]]

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.

Comment 12

[Simon Fraser [no longer active]]

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.

Comment 13

[Simon Fraser [no longer active]]

Attachment: Patch to avoid calling WakeUpProcess from the timer task when we're not processing NSPR threads

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

[Steve Dagley]

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 :-)

Comment 15

[Wan-Teh Chang]

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?

Comment 16

[Simon Fraser [no longer active]]

> 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

[Steve Dagley]

I don't believe initialization of 'lastThreadSwitch' matters much since it's set
on each thread switch but for the sake of clarity sure.

Comment 18

[Simon Fraser [no longer active]]

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.

Comment 19

[Simon Fraser [no longer active]]

Attachment: Updated patch with whitespace changes, and more context

New patch addresses wtc's comments, and shows whitespace changes.

Comment 20

[Wan-Teh Chang]

Comment on attachment 92840 [details] [diff] [review]
Updated patch with whitespace changes, and more context

r=wtc.

Comment 21

[Brian Nesse (gone)]

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

[Wan-Teh Chang]

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.

Comment 23

[Brian Nesse (gone)]

Or at least add a comment indicating that this is the case...

Comment 24

[Simon Fraser [no longer active]]

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

[Asa Dotzler [:asa]]

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

Comment 26

[Simon Fraser [no longer active]]

Fix checked in on the NSPRPUB_PRE_4_2_CLIENT_BRANCH branch and the NSPR tip.

Comment 27

[scottputterman]

shrir,  can you verify this on the trunk?

Comment 28

[shrirang khanzode]

fix looks good on 0729 trunk on os x. no side effects observed.

Comment 29

[Simon Fraser [no longer active]]

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

[Jaime Rodriguez, Jr.]

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".

Comment 31

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 32

[Simon Fraser [no longer active]]

Checked in on the MOZILLA_1_0_BRANCH.

Comment 33

[John Unruh]

3. Mail over SSL - SSL IMAP is no slower than without SSL
4. Certificate authentication performance (bug 139802).  Works fine.

Comment 34

[shrirang khanzode]

plugin functionality on trunk /object drag n drop tests worked fine.

Comment 35

[shrirang khanzode]

verifido on 0823 branch on osx. Things are merry!