473805 - Prevent resize event loops

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a patch

The testcase is to put "Bookmark toolbar items" to menubar and shrink the size of
the window to show only menubar items. Bookmarks are hidden then shown again.
This causes content to be resized and the places resize event handler gets called
again.
This is something which could be fixed on script side (there is the slow script
warning), but I think we should try to prevent this happening.

The idea with the patch is to limit the number of synchronous resize events
and also to fire such events later (in WillPaint).

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Note, the "testcase" works on linux only, I think. And the problem
(resize event loop) was there even before bug 457862. But since events weren't
synchronous, the UI wasn't blocked.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you explain the patch in a little more detail?  (Also, why is it safe to fire resize events from WillPaint?)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

We run scripts anyway in WillPaint - there is the flush call.

The idea with the patch is to stop synchronous resize loops at some point.
The testcase triggers the problem so that BookmarksToolbar hides all the icons.
That changes the toolbar size a bit, and that causes content window to
resize itself. Resize event is fired and BookmarksToolbar captures that event 
and does something which changes the size again and flushes layout and new
event is fired...

The patch starts a timer when first synchronous resize is about to be 
dispatched. Then some synchronous events are allowed to happen, but after the
limit only asynchronous events. And when the timer fires, that clears the 
syncronous event count and synchronous resize is allowed again.
This way the main loop gets a chance to process user/OS events.

The synchronous event limit does have the affect that in some cases
resize isn't dispatched soon enough.
Not perfect, but I don't actually know if Compositor could solve the problem
perfectly either.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This seems like it could produce weird timing-dependent effects for authors.  I would think a simpler approach would be more likely to yield behavior that authors can understand.  Could we just make any resize event caused by handling of another resize event asynchronous, or something like that?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 357203 [details] [diff] [review]
a patch

review- based on previous comment, unless I'm misunderstanding what's going on

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #4)
> Could we just make any resize event caused by handling
> of another resize event asynchronous, or something like that?
Unfortunately that doesn't quite work. The UI stays somewhat working, but
for example menus don't work. Seems like OS events aren't processed properly,
because there is always a runnable to process before that.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #6)
> The UI stays somewhat working
I mean, the UI isn't totally locked up, but the window can be resized.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Can you make the asynchronous case work a different way (e.g., using a timer) to fix that?

Comment 9

[Daniel Holbert [:dholbert]]

Any updates here?  This is preventing bug 457862 (and by extension, bug 472730) from landing on 1.9.1.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: simpler

If resize causes a new resize, the latter one will happen asynchronously, and
no other resize events will be dispatched before the asynchronous one.
This fixes the testcase and doesn't regress testcases in Bug 457862
or bug 472730.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 393508 [details] [diff] [review]
simpler

>+  PRPackedBool mInSyncResize;

Probably better to clal this mInResize since it's true for async resizes too.

>+  if (mAsyncResizeEventTimer) {

Shouldn't this just check mAsyncResizeTimerIsActive?

>+    mAsyncResizeEventTimer->Cancel();
>+  }
>+  mAsyncResizeTimerIsActive = PR_FALSE;

And maybe set mAsyncResizeEventTimer to null as well?

>+void
>+PresShell::AsyncResizeEventCallback(nsITimer* aTimer, void* aPresShell)
>+{
>+  PresShell* self = static_cast<PresShell*>(aPresShell);
>+  if (self) {
>+    self->FireResizeEvent();
>+  }
>+}

It doesn't seem like there's any point to the null-check here, so I think you should remove it.

> void
> PresShell::FireResizeEvent()
> {
>+  if (mAsyncResizeTimerIsActive && mAsyncResizeEventTimer) {
>+    mAsyncResizeTimerIsActive = PR_FALSE;
>+    mAsyncResizeEventTimer->Cancel();
>+  }

I think you only need to check *IsActive; you don't need to null-check the timer too since IsActive implies that the timer is non-null.

>+    nsCOMPtr<nsIPresShell> kungFuDeathGrip(this);

Shouldn't this be in AsyncResizeEventCallback instead?  kungFuDeathGrip(this) is a sign that somebody who should be holding a strong pointer to the object isn't doing so.

r=dbaron with those things fixed

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> (From update of attachment 393508 [details] [diff] [review])
> >+  PRPackedBool mInSyncResize;
> 
> Probably better to call this mInResize since it's true for async resizes too.
OK
 
> >+  if (mAsyncResizeEventTimer) {
> 
> Shouldn't this just check mAsyncResizeTimerIsActive?
OK

> 
> >+    mAsyncResizeEventTimer->Cancel();
> >+  }
> >+  mAsyncResizeTimerIsActive = PR_FALSE;
> 
> And maybe set mAsyncResizeEventTimer to null as well?
Why? It will be cleared when presshell is deleted.



> It doesn't seem like there's any point to the null-check here, so I think you
> should remove it.
OK

> I think you only need to check *IsActive; you don't need to null-check the
> timer too since IsActive implies that the timer is non-null.
OK.

> 
> >+    nsCOMPtr<nsIPresShell> kungFuDeathGrip(this);
> 
> Shouldn't this be in AsyncResizeEventCallback instead?
I could add nsCOMPtr/kungFuDeathGrip to the places where FireResizeEvent is called. It is the presshell which may be deleted.
But, because runnable method is used to call the method that makes it
a bit difficult to have nsCOMPtr. So in this case kungFuDeathGrip is just
easier.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/6e5826942eb2

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #12)
> > And maybe set mAsyncResizeEventTimer to null as well?
> Why? It will be cleared when presshell is deleted.

Because if anyone changes the timer to using an nsIObserver or nsITimerCallback in the future, it would be prone to leaks if it weren't disconnected.

> > >+    nsCOMPtr<nsIPresShell> kungFuDeathGrip(this);
> > 
> > Shouldn't this be in AsyncResizeEventCallback instead?
> I could add nsCOMPtr/kungFuDeathGrip to the places where FireResizeEvent is
> called. It is the presshell which may be deleted.
> But, because runnable method is used to call the method that makes it
> a bit difficult to have nsCOMPtr. So in this case kungFuDeathGrip is just
> easier.

Is there any risk of the pres shell being deleted immediately from any of the other callers?  If so, it seems like there are other problems there.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #15)
> Is there any risk of the pres shell being deleted immediately from any of the
> other callers?  If so, it seems like there are other problems there.
The other cases when the method is called are async callback (which doesn't do anything but just call the method, so it is safe) and when flushing, and 
flushing kungFuDeathGrips presshell and checks if presshell was destroyed (not deleted) after the event was dispatched.

(In reply to comment #15)
> Because if anyone changes the timer to using an nsIObserver or nsITimerCallback
> in the future, it would be prone to leaks if it weren't disconnected.
Well, if someone decides to use some other API, I'd expect s/he would actually
make sure the change works and doesn't leak.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

And btw, nsTimerImpl::Cancel() releases the callback object (nsIObserver or nsITimerCallback)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9c6103ea22f4

Comment 19

[Samuel Sidler (old account; do not CC)]

Is this something we can take on 1.9.1? (Following the chain from bug 457862 and bug 472730.)

I know it's only been landed for a day, but are there other bugs we need to consider if we take this on 1.9.1 beyond those two?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't know of any; smaug should answer too, though.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

The question is, should we take bug 457862 on 1.9.1?
That is a functionality change, so scripts may even depend on 1.9.1 to
handle resize the way it does now.

Comment 22

[Samuel Sidler (old account; do not CC)]

Olli: Since that's a behavior change in your code, I think that's your call on whether we should take a functionality change there.

I guess we can opt not to fix bug 472730 on the 1.9.1 branch.