Bug 138440 – Assertion hit DOMTimer::updateTimerIntervalIfNecessary()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

I sometimes hit the following assertion in DOMTimer::updateTimerIntervalIfNecessary():
ASSERT(repeatInterval() == previousInterval);

when visiting the following URLs:
http://lifehacker.com/the-healthiest-foods-for-one-handed-snacking-while-gami-1654728164
http://longform.org/posts/like-something-the-lord-made

Trace:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000011324634a WTFCrash + 42
1   com.apple.WebCore             	0x0000000114e93f37 WebCore::DOMTimer::updateTimerIntervalIfNecessary() + 247 (DOMTimer.cpp:324)
2   com.apple.WebCore             	0x0000000114e93e2c WebCore::DOMTimer::updateThrottlingStateIfNecessary(WebCore::DOMTimerFireState const&) + 140 (DOMTimer.cpp:203)
3   com.apple.WebCore             	0x0000000114e942b2 WebCore::DOMTimer::fired() + 706 (DOMTimer.cpp:256)
4   com.apple.WebCore             	0x00000001166b705c WebCore::ThreadTimers::sharedTimerFiredInternal() + 396 (ThreadTimers.cpp:135)
5   com.apple.WebCore             	0x00000001166b6d19 WebCore::ThreadTimers::sharedTimerFired() + 25 (ThreadTimers.cpp:108)
6   com.apple.WebCore             	0x000000011637aa2f WebCore::timerFired(__CFRunLoopTimer*, void*) + 31 (SharedTimerMac.mm:125)
7   com.apple.CoreFoundation      	0x00007fff8eb54b44 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
8   com.apple.CoreFoundation      	0x00007fff8eb547d3 __CFRunLoopDoTimer + 1059
9   com.apple.CoreFoundation      	0x00007fff8ebc7d9d __CFRunLoopDoTimers + 301
10  com.apple.CoreFoundation      	0x00007fff8eb11268 __CFRunLoopRun + 2024
11  com.apple.CoreFoundation      	0x00007fff8eb10838 CFRunLoopRunSpecific + 296
12  com.apple.HIToolbox           	0x00007fff8a0b03ff RunCurrentEventLoopInMode + 235
13  com.apple.HIToolbox           	0x00007fff8a0b017a ReceiveNextEventCommon + 431
14  com.apple.HIToolbox           	0x00007fff8a0affbb _BlockUntilNextEventMatchingListInModeWithFilter + 71
15  com.apple.AppKit              	0x00007fff85f4b85d _DPSNextEvent + 964
16  com.apple.AppKit              	0x00007fff85f4b010 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 194
17  com.apple.AppKit              	0x00007fff85f3ef93 -[NSApplication run] + 594
18  com.apple.AppKit              	0x00007fff85f2a444 NSApplicationMain + 1832
19  libxpc.dylib                  	0x00007fff9167fef2 _xpc_objc_main + 793
20  libxpc.dylib                  	0x00007fff91681a9d xpc_main + 490
21  com.apple.WebKit.WebContent.Development	0x0000000108f5a115 main + 37
22  libdyld.dylib                 	0x00007fff8436d5c9 start + 1

Investigation:
We are relying on floating point comparison in DOMTimer code and sometimes repeatInterval() == previousInterval returns false even though the values are roughly the same (due to precision). Printing those values shows "0.2 != 0.2". We should make sure the difference between the values is within std::numeric_limits<double>::epsilon() instead.

Possible cause for the following Radar:
<rdar://problem/18883681>

[Chris Dumez]

Created attachment 241069 [details]
Patch

[Chris Dumez]

Removing InRadar keyword as it is a separate issue from <rdar://problem/18883681>.

[Geoffrey Garen]

Comment on attachment 241069 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 241069 [details]
Patch

Clearing flags on attachment: 241069

Committed r175655: <http://trac.webkit.org/changeset/175655>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

The "within epsilon" check only does something useful for values that are close enough to 0. It’s not generally a correct technique to use on arbitrary floating point values.

It would be nice to rid our code of this technique if we can find a better solution to these problems in the future.

[Chris Dumez]

(In reply to comment #6)
> The "within epsilon" check only does something useful for values that are
> close enough to 0. It’s not generally a correct technique to use on
> arbitrary floating point values.
> 
> It would be nice to rid our code of this technique if we can find a better
> solution to these problems in the future.

Yes, agreed. I can update the function to use one of Knuth's formulas from:
http://www.boost.org/doc/libs/1_34_0/libs/test/doc/components/test_tools/floating_point_comparison.html#Introduction

I am not sure which formula would be best here but any of those would likely be better than the basic withinEpsilon() check we currently have. Any thought on this?