Closed Bug 473805 Opened 14 years ago Closed 13 years ago

Prevent resize event loops

Categories

(Core :: Layout, defect, P2)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch a patch (obsolete) — Splinter Review
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).
Attachment #357203 - Flags: review?(dbaron)
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.
Could you explain the patch in a little more detail?  (Also, why is it safe to fire resize events from WillPaint?)
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.
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 on attachment 357203 [details] [diff] [review]
a patch

review- based on previous comment, unless I'm misunderstanding what's going on
Attachment #357203 - Flags: review?(dbaron) → review-
(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.
(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.
Can you make the asynchronous case work a different way (e.g., using a timer) to fix that?
Flags: blocking1.9.2?
Any updates here?  This is preventing bug 457862 (and by extension, bug 472730) from landing on 1.9.1.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Assignee: nobody → Olli.Pettay
Attached patch simplerSplinter Review
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.
Attachment #357203 - Attachment is obsolete: true
Attachment #393508 - Flags: review?(dbaron)
Status: NEW → ASSIGNED
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
Attachment #393508 - Flags: review?(dbaron) → review+
(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.
Attached patch patchSplinter Review
http://hg.mozilla.org/mozilla-central/rev/6e5826942eb2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(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.
(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.
And btw, nsTimerImpl::Cancel() releases the callback object (nsIObserver or nsITimerCallback)
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?
I don't know of any; smaug should answer too, though.
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.
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.
You need to log in before you can comment on or make changes to this bug.