Summary: | Reschedule shared CFRunLoopTimer instead of reconstructing it | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | REOPENED --- | ||||||||
Severity: | Normal | CC: | darin, kling, rniwa, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 109856, 110128 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Antti Koivisto
2013-02-13 16:13:05 PST
Created attachment 188213 [details]
patch
Comment on attachment 188213 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=188213&action=review Hilarious! r+ from me since this looks like correct use of the API to me, though you might wanna double-check with someone with more CF knowledge. > Source/WebCore/platform/mac/SharedTimerMac.mm:154 > + static CFRunLoopTimerRef timer; > + if (!timer) { You could use dispatch_once() for this block. When I first implemented SharedTimer I tried using CFRunLoopTimerSetNextFireDate and it didn’t work. Maybe I did it wrong. Or maybe there were CF bugs that have since been fixed. Someone should be sure to test this on the oldest OS X and iOS versions that WebKit currently supports in case that old bug is not fixed there. (In reply to comment #4) > When I first implemented SharedTimer I tried using CFRunLoopTimerSetNextFireDate and it didn’t work. Maybe I did it wrong. Or maybe there were CF bugs that have since been fixed. Someone should be sure to test this on the oldest OS X and iOS versions that WebKit currently supports in case that old bug is not fixed there. JSC already uses CFRunLoopTimerSetNextFireDate for HeapTimer. Re-opened since this is blocked by bug 109856 This caused failures in some inspector debugger tests http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r142826%20(5930)/results.html: Maybe nested event loops are causing problems or something? Created attachment 188716 [details]
patch2
try to fix the inspector debugger
Comment on attachment 188716 [details] patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=188716&action=review > Source/WebCore/platform/mac/SharedTimerMac.mm:177 > +void reinsertSharedTimer() > +{ > + // For some reason the timer won't fire in a nested runloop unless it has been freshly inserted. > + CFRunLoopTimerInvalidate(globalSharedTimer); > + CFRelease(globalSharedTimer); > + globalSharedTimer = 0; > +} This function's name seems to be at odds with its contents. Where does the timer actually get re-inserted? (In reply to comment #10) > This function's name seems to be at odds with its contents. > Where does the timer actually get re-inserted? From sharedTimer(), next time the timer is accessed. This will happen from ThreadTimers::fireTimersInNestedEventLoop() right after this call. Suggestions welcome. Comment on attachment 188716 [details] patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=188716&action=review r=me if you find a nicer color for the bikeshed. > Source/WebCore/platform/mac/SharedTimerMac.mm:171 > +void reinsertSharedTimer() I'd just prefer to call this removeFoo() instead of reinsertBar() since that's what it does. Re-opened since this is blocked by bug 110128 The bots still see some nested runloop related failures. Comment on attachment 188213 [details] patch Cleared Andreas Kling's review+ from obsolete attachment 188213 [details] so that this bug does not appear in http://webkit.org/pending-commit. |