Bug 18044 – Timer issues with nested event loops on GTK

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: Platform

[Michael Emmel]

It looks like Timer is dropping timers set inside of firing timers. And that it does not correctly handle nested event loops.

[Michael Emmel]

Created attachment 20007 [details]
Patch for silently failing timers

[Brady Eidson]

Comment on attachment 20007 [details]
Patch for silently failing timers

At a high level, I don't feel qualified to really review this patch - there are a few folks who are intimately familiar with the timer code who would be much safer reviewers.

But, I can still make a review decision based on:
1 - Can you provide an example of how this bug is manifesting itself?
2 - Using that real world example/reduction, can a layout test be made?
3 - A Changelog entry is also required

[Michael Emmel]

Is someone responsible for the timer code ?

As far as a bug its pretty obvious if your firing a timer the original code silently dropped timers. 

The code also has another bug I did not fix and its if you change the timer of a timer thats in the list to fire after the one your firing then it does not fire.
Most of time this results in a timer continuously reset into the future.

I suspect the guard for the timer was and attempt to fix this. By dropping timers you hide this other bug.

The answer is the timers or at least the fire times need to be copied in the fire list and it should be local to the function.

Any chance that I can talk to a resident expert on this code since it still has a number of problems. I just fixed a obvious problem and also noted the potential advancing timer problem. Thats three with just me reviewing the code.
I'd not be surprised if there is one more.

So does the code have a resident expert ?

[Dave Hyatt]

Darin is the one who wrote the timer code.

[Michael Emmel]

Hi I emailed Darin no response

[Michael Emmel]

I don't think the future problem actually exists the ready to fire list is not used for the actual firings. So I think just zeroing the list if you remove the last timer solves the problem I'm seeing.

[Michael Emmel]

I take that back it still looks like we will potentially skip timers.
I don't think that fireTimersInNestedEventLoop is a real fix.

Probably the real answer is setNextFireTime should not remove a timer from the readyToFireList this should only be done after the Timer fires

[Michael Emmel]

Created attachment 20114 [details]
Nested event loop patch


I looked at the log and this is really the same problem that was supposedly fixed by <rdar://problem/5094895> REGRESSION (r19094)

I re factored the patch to make at cleaner and remove the case of someone resetting and removing a timer from the fire list that has not yet fired. Now a timer is only removed from the fire list right before it fires and the fire list is not mutated out side of the shared callback.

The special 
fireTimersInNestedEventLoop

is not needed anymore.

[Darin Adler]

Comment on attachment 20114 [details]
Nested event loop patch

Could you fix the formatting of this patch? The comments are not matching the style of the surrounding comments (all lowercase, no space after the //, multiple spaces in one place, no capital letters on the sentences).

Could you reword the comments to be clearer and use more straightforward technology? What's a "bool guard"?

This sequence seems obviously wrong:

+        if (timersReadyToFire)
+            timersReadyToFire->remove(timer);
+        if(!timersReadyToFire->size())
+            timersReadyToFire = 0;

If timersReadyToFire is 0, then we're going to crash dereferencing the pointer.

I don't understand the change log comment at all. In what sense does this change make anything "generic"?

I think this change to the Timer class may be an incorrect workaround for a bug elsewhere.

The logic here seems really muddled. The old change gave a special meaning to a timersReadyToFire value of 0. Just setting it to zero because you fired the last timer is not at all consistent with that design.

I believe the correct fix is to add a call to fireTimersInNestedEventLoop somewhere.

[Michael Emmel]

Created attachment 20136 [details]
Removed uneeded fireTimersInNestedEventLoop


This patch removes the uneeded and incorrect fireTimersInNestedEventLoop and cleans up the Timer patch.

The reason fireTimersInNestedEventLoop is incorrect is that nesting of event loops is done inside the GTK application WebKit cannot correctly determine if its running in a nested event loop set up by the application using the the WebKit widget.  So its wrong to not function correctly in nested event loops that may be created using the platform api outside of the scope of WebKit.

[Michael Emmel]

Created attachment 20307 [details]
This patch removes the use of the static timersReadyToFire

This patch completely removes the static variable

[Michael Emmel]

Created attachment 20321 [details]
Patch adds back static after refactoring


This patch is represents the final rewrite of the time code. It adds back a static variable but used only as a guard for stopped or deleted timers.

Timers probably should be reference counted.

[Michael Emmel]

Comment on attachment 20321 [details]
Patch adds back static after refactoring

I was hoping the previous patches could spur helpful comments before fully refactoring the code. 
In any case this is the final patch until after comments.

[Michael Emmel]

Created attachment 20322 [details]
Same as previous patch with expanded changelog entry


Expand change log entry

[Michael Emmel]

Created attachment 20325 [details]
Expanded changelog to explain what was changed.

[Aron Rosenberg]

Created attachment 26883 [details]
Updated patch for nested timers fix

This is an updated patch which takes into account the inspector changes

[Martin Robinson]

Carlos, maybe you can verify whether this can be closed?

[Martin Robinson]

Going to close this since it seems to have dried up. The timer code has changed a lot, but to fix this we need a more modern test case.