[webkit-reviews] review denied: [Bug 18941] WebView fails to load if opened from a blocking JS call in a timer. : [Attachment 23580] Fix enum naming

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jeremy Moskovich
<playmobil at google.com>'s request for review:
Bug 18941: WebView fails to load if opened from a blocking JS call in a timer.
https://bugs.webkit.org/show_bug.cgi?id=18941

Attachment 23580: Fix enum naming
https://bugs.webkit.org/attachment.cgi?id=23580&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I'm glad you're tackling this problem.

This patch changes timers so they can reenter. It's great that it preserves the
property that DOM timers don't reenter. But it allows all other timers to get
called in new way. I expect that will cause problems. Did you audit any of the
42 timers used in WebCore to check if it is OK to allow them all to reenter?
What kind of testing did you do?

The language bridging layer is not a good place to call
fireTimersInNestedEventLoop. I suppose the justification is that it's a call
"out" of WebKit, but it's a blurring of responsibilities and a layering
violation for the bindings mechanism to know about timers at all. The bindings
are a language-bridging mechanism, and the event loop and timer machinery
should not be involved. Further, there is a more modern JavaScript API for
calls to and from JavaScript that doesn't involve this old bridging code. If
this is necessary we'd need to find a way to do it for that mechanism too.

I think the solution to this design quandry is that we should eliminate the
fireTimersInNestedEventLoop function entirely if we can make the disabling
mechanism work well enough to prevent reentrancy.

+// Duplicated from JSDOMWindowBase.cpp - what would be a good common place to
put this? 
+static const double cMinimumTimerInterval = 0.010;

This is wrong. This minimum timer interval is not a feature of the Timer class.
It's a feature specifically of timers created from JavaScript with the DOM
window API and should have no affect on other timers. It is incorrect to put
code knowing about this minimum into Timer.cpp.

+	 // Check if this timer is disabled by the current mask (e.g. if we're
being called from inside a 
+	 // JS Timer and this is also a JS Timer).  If so, delay execution till
later.
+	 unsigned mask_result = timer->type() & WebCore::timerMask;
+	 if (mask_result == 0) {
+	     timer->setNextFireTime(fireTime + cMinimumTimerInterval);
+	     continue;
+	 }

It's not a good design to change the firing time if a timer doesn't qualify for
firing right away. Consider that if the timer mask changes, we'd want to fire
the timer right away. And if the timer mask doesn't change, it's important to
not to have the presence of a disabled timer cause the system timer to fire
over and over again.

Instead the algorithm and data structure should skip over the timers that don't
qualify rather than removing and re-adding them to the timer heap each time
they fire erroneously.

I think a workable design would be to have a separate timer heap for each timer
type. I don't think we're going to need tons of different timer types -- the
theoretical generality of the current patch isn't all that important if we have
only two types of timer.

+	 unsigned savedTimerMask = WebCore::timerMask;
+	 WebCore::timerMask = timer->timerMask();

I believe the timer disabling should be cumulative. So if you have certain
timers disabled, firing a timer shouldn't enable them. Therefore, this should
be doing an &= rather than an assignment.

----------------

Other more-minor code style comments:

+    WebCore::TimerBase::fireTimersInNestedEventLoop();

The normal way to do this would be to put "using namespace WebCore" at the top
of the file rather than qualifying every use of TimerBase with the namespace.

 #import "FoundationExtras.h"
+#include "Timer.h"
 #import "WebScriptObject.h"

If the others are #import, then the Timer.h one should be too.

+    // Types to identify categories of timers.
+    typedef enum {
+	 TimerTypeDOMWindowTimer = 1 << 0,
+	 TimerTypeGenericTimer	 = 1 << 1,
+	 TimerTypeAllTimers	 = 0xFFFF
+    } TimerType;

This should just be "enum TimerType"; there's no need for typedef here in C++.

-    : m_nextFireTime(0), m_repeatInterval(0), m_heapIndex(-1)
+    : m_nextFireTime(0), m_repeatInterval(0), m_heapIndex(-1),
m_type(TimerTypeGenericTimer)
+    , m_timerMask(TimerTypeAllTimers)

Our usual style is to either put everything on one line, or everything on a
separate line. So since you're overflowing one line you should put these all on
separate lines.