155328 - nsViewManager and nsPresShell are unaware of stacked event queues

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P3)

Description

[Robert Ginda]

nsViewManager and nsPresShell do not deal with the fact that the queue that was
active when they were initialized may not be the queue they should always post
events to.

In the case where an event queue us pushed, reflow and invalidate events end up
stuck in the previous queue, and parts of the UI (the parts that intialized
before the new queue) no longer paint.

The simplest testcase is to load netscape.com in one window, and
"javascript:void setTimeout(alert, 0, 1)" in another (see the URL of this bug.)
 When the setTimeout executes, you'll notice that the netscape.com marquee stops
animating.

This problem is much more serious in the js debugger, where the debugger UI
stops repainting properly when the user stops in a timeout.

I'm not exactly sure why we only see this problem in timeouts.  I suspect, for
non-timeouts, something else comes along to force us to update.  Perhaps related
in some way to native or idle events.

Comment 1

[Robert Ginda]

Attachment: proposed patch for nsViewManager.cpp

This patch resolves the event queue each time an invalidate is posted.	If the
event queue is the same one we had last time, and mPendingInvalidateEvent is
set, we ignore the new invalidate, otherwise we post it.

During view manager teardown we revoke events regardless of the state of
mPendingInvalidateEvent because we don't know for sure if we have pending
events on a lower event queue.

Comment 2

[Robert Ginda]

Attachment: proposed patch for nsPresShell.cpp

same ideas as in the nsViewManager.cpp patch, applied to nsPresShell.cpp.

Comment 3

[Sven Krohlas]

*** Bug 155327 has been marked as a duplicate of this bug. ***

Comment 4

[Robert Ginda]

I'd like to get this on the trunk and 1.0 branch pretty quickly.  Probably just
the trunk at first, to see if the perf hit is noticable.  Comments anyone?

Comment 5

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

In the viewmanager patch, what happens if the view manager is destroyed while an
event queue is pushed? will an invalidate event be stuck on the old event queue
and later pop off, causing us to crash because the view manager has gone away?

Comment 6

[Robert Ginda]

no, for two reasons.  First and foremost, we now call RevokeEvents() in the
nsViewManager and nsPresShell destructors regardless of the state of the pending
flag.  RevokeEvents takes care to revoke the events in every event queue in the
stack.

As a backup (and probably overkill), the existing HandleEvent method of
nsInvalidateEvent actually walks the list of known view managers to verify that
the view manager in question still exists.

Comment 7

[rpotts (gone)]

Is there any reason to keep the mEventQueue member in nsViewManager ?  It seems
like we should always get the eventQ ...

-- rick

Comment 8

[rpotts (gone)]

Same with nsPresShell.cpp...  It looks like mEventQueue should be eliminated...
 Since we want to deal with the 'active' eventQ not the 'original' one...

-- rick

Comment 9

[Doug Turner (:dougt)]

should we have a special nsIEventQueue implememention which will aways points at
the topmost queue?

Comment 10

[Robert Ginda]

+  if ((!mPendingReflowEvent || eventQueue != mEventQueue) &&

We still need to keep the old event queue in order to tell whether or not it
changed.  If it hasn't changed, and we have a pending event, we don't post a new
event.  This saves quite a few reflow/repaints.

Comment 11

[rpotts (gone)]

it still seems like we could merge the two variables into a mPendingReflowForQ 
(or some equally horrid name) which is either null or indicates the eventQ where 
the reflow/repaint is pending...

just a thought...  Usually 'mEventQueue' means *the* event queue (in other parts 
of the code)...  So, getting rid of it would make the code easier to 'skim' :-)

-- rick

Comment 12

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

We need to keep a pointer to the event queue service so we don't keep getting it
over and over again.

Comment 13

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

Comment on attachment 89896 [details] [diff] [review]
proposed patch for nsViewManager.cpp

r=roc+moz

Comment 14

[Robert Ginda]

Attachment: nsViewManager.cpp patch with rpotts' suggestion

mPendingInvalidateEvent and mEventQueue are combined into mInvalidateEventQueue
in this patch.

Comment 15

[Robert Ginda]

Attachment: revised nsPresShell.cpp changes

mPendingReflowEvent and mEventQueue combined into mReflowEventQueue.

Comment 16

[Robert Ginda]

Attachment: nsViewManager changes, relative to the trunk

here are the view changes, applied to the trunk

Comment 17

[Robert Ginda]

Attachment: layout changes, relative to the trunk

layout/ changes, applied to the trunk.	ready for r/sr, hopefully for 1.1.

Comment 18

[rpotts (gone)]

The following block of cleanup code seems a little strange:

+  mReflowEventQueue = nsnull;
+  nsCOMPtr<nsIEventQueue> eventQueue;
+  mEventQueueService->GetSpecialEventQueue(kUIEventQueue,
+                                           getter_AddRefs(eventQueue));
+  eventQueue->RevokeEvents(this);
+  mReflowEventQueue = nsnull;


We seem to be clearing mReflowEventQueue twice.  I'd have expected something
more like:

  if (mReflowEventQueue) {
    mReflowEventQueue->RevokeEvents(this);
    mReflowEventQueue = nsnull;
  }

-- rick

Comment 19

[Robert Ginda]

> We seem to be clearing mReflowEventQueue twice.  I'd have expected something
> more like:
>
>  if (mReflowEventQueue) {
>    mReflowEventQueue->RevokeEvents(this);
>    mReflowEventQueue = nsnull;
>  }

Oops, I'm not sure why I nulled mReflowEventQueue twice, I'll fix that.  Your
sample code wouldn't work, however.  mReflowEventQueue may in fact be null, even
when we have a pending reflow event.

When a reflow event is posted but not handled before a new queue is pushed, it
will stay there until the queue is popped.  If new reflows are posted and
handled on the pushed queue, mReflowEventQueue will be nulled out (as it always
is, after processing a reflow event.)  If the view is destroyed before the
events in the elder queue are processed, we'll have a null mReflowEventQueue,
but some pending reflows in elder queues.

Also, if a queue is pushed on top of a queue with pending reflows, but new
events are *not* pushed, mReflowEventQueue won't point to the top queue.  I just
assumed fetch the top queue here, instead of relying on mReflowEventQueue
pointing to the right place.

Comment 20

[rpotts (gone)]

sounds reasonable :-)

Comment 21

[rpotts (gone)]

Comment on attachment 91602 [details] [diff] [review]
layout changes, relative to the trunk

sr=[e-mail]

Comment 22

[Robert Ginda]

roc, does your r= still stand?

Comment 23

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

Comment on attachment 91601 [details] [diff] [review]
nsViewManager changes, relative to the trunk

r=roc+moz

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 91601 [details] [diff] [review]
nsViewManager changes, relative to the trunk

sr=jst on these changes, but I have a few nits to pick here.

+#define kUIEventQueue	    nsIEventQueueService::UI_THREAD_EVENT_QUEUE

The names of #defines should be all uppercase, as they normally are in mozilla
code. The name kUIEve... usggest that it's a constant, but it's not, it's a
#define... And why do we need this in the first place? IMO it would be easier
to tell what this code does if you'd use
nsIEventQueueService::UI_THREAD_EVENT_QUEUE where you now use this #define...

 nsViewManager::PostInvalidateEvent()
 {
-  if (!mPendingInvalidateEvent) {
+  nsCOMPtr<nsIEventQueue> eventQueue;
+  mEventQueueService->GetSpecialEventQueue (kUIEventQueue,
+					     getter_AddRefs(eventQueue));

Loose the space before the opening paren.

 nsViewManager::~nsViewManager()
 {
...
+  mEventQueueService->GetSpecialEventQueue (kUIEventQueue,
+					     getter_AddRefs(eventQueue));

Same thing, loose the space...

-  if (nsnull == mEventQueue) {
...
+  if (nsnull == mEventQueueService)
+    mEventQueueService = do_GetService(kEventQueueServiceCID);

-    NS_ASSERTION(nsnull != mEventQueue, "event queue is null");
-  }
+  NS_ASSERTION(mEventQueueService, "couldn't get event queue service");

Why loose the braces around the if here? Asserting that mEventQueueService is
null outside the check for it not being null is just pointless, move the assert
into the if...

Comment 25

[Robert Ginda]

jst:  I picked your nits in the view manager, care to r= the pres shell too? 
(I've already removed the define and extra whitespace, as in the view manager.)

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 91602 [details] [diff] [review]
layout changes, relative to the trunk

+  mEventQueueService = do_GetService(kEventQueueServiceCID, &result);
+
+  NS_ASSERTION(mEventQueueService, "couldn't get event queue service");

Wouldn't it be good to return an error code here in stead of just asserting and
then prociding to a guaranteed crash? Not a big deal, since this should never
happen, but still...

+  mReflowEventQueue = nsnull;
+  nsCOMPtr<nsIEventQueue> eventQueue;
+  mEventQueueService->GetSpecialEventQueue(kUIEventQueue,
+					    getter_AddRefs(eventQueue));
+  eventQueue->RevokeEvents(this);
+  mReflowEventQueue = nsnull;

Remove the double nulling out of mReflowEventQueue, as discussed earlier...

r=jst

Comment 27

[Asa Dotzler [:asa]]

Comment on attachment 91601 [details] [diff] [review]
nsViewManager changes, relative to the trunk

a=asa (on behalf of drivers) for checkin to 1.1

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 91602 [details] [diff] [review]
layout changes, relative to the trunk

a=asa (on behalf of drivers) for checkin to 1.1

Comment 29

[Robert Ginda]

checked in to trunk, leaving open because I'd like to get into the branch too.

Comment 30

[cls]

FWIW, the checkin for this bug broke the beos tinderbox.  The broswer starts
with a small (64x64?) window and is completely gray.  Nothing get rendered. 
Backing out the checkin "fixed" the problem.

Comment 31

[Robert Ginda]

You could try
s/nsIEventQueueService::UI_THREAD_EVENT_QUEUE/nsIEventQueueService::CURRENT_THREAD_EVENT_QUEUE/
in both patches.  I don't think that makes sense for other platforms, but maybe
it'll do something for BeOS.

Comment 32

[Aleksey Nogin]

This checkin causes nsViewManager::Init to always rerun an uninitialized
nsresult on success!

It added a new warning on brad TBox:

+ view/src/nsViewManager.cpp:471
+  `nsresult rv' might be used uninitialized in this function

See also bug 59562 on "might be used uninitialized" warnings.

Comment 33

[Robert Ginda]

Attachment: fix nsViewManager::Init return value

D'oh.  Maybe that's the problem with BeOS.

Comment 34

[Johnny Stenback (:jst)]

Comment on attachment 92922 [details] [diff] [review]
fix nsViewManager::Init return value

sr=jst

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 92922 [details] [diff] [review]
fix nsViewManager::Init return value

r=bzbarsky

Comment 36

[Asa Dotzler [:asa]]

Comment on attachment 92922 [details] [diff] [review]
fix nsViewManager::Init return value

a=asa (on behalf of drivers) for checkin to 1.1

Comment 37

[hacker formerly known as [e-mail]]

The latest patch fixed the problem on BeOS. Thanks!

Comment 38

[Robert Ginda]

Attachment: view changes, relative to the branch

Comment 39

[Robert Ginda]

Attachment: layout changes, relative to branch

Comment 40

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 93499 [details] [diff] [review]
view changes, relative to the branch

Approval for branch granted; changek mozilla1.0.1+ to fixed1.0.1 when checked
in.  Carrying forward r/sr

I would have preferred a few comments about the issue, or a ref to the bug, but
oh well.

Comment 41

[Jaime Rodriguez, Jr.]

adding mozilla1.0.1+ to the keywords per Comment #40 From Randell Jesup, whicha
grants Mozilla Drivers' approval.

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch. pls check
this in asap, then replace "mozilla1.0.1+" with "fixed1.0.1". thanks!

Comment 42

[Robert Ginda]

checked in to 1.0 BRANCH