[webkit-reviews] review denied: [Bug 114577] JSC: Add LLINT and baseline JIT support for timing out scripts : [Attachment 198499] patch 5: addressed all of Geoff's feedback, fixed a few bugs, and did some additional clean up.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 114577: JSC: Add LLINT and baseline JIT support for timing out scripts
https://bugs.webkit.org/show_bug.cgi?id=114577

Attachment 198499: patch 5: addressed all of Geoff's feedback, fixed a few
bugs, and did some additional clean up.
https://bugs.webkit.org/attachment.cgi?id=198499&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=198499&action=review


Much improved, but I believe I've spotted a data race.

> Source/JavaScriptCore/API/JSContextRefPrivate.h:91
> + at param callbackData User data that you can provide to be passed back to you
> + in your callback.

This is "context" now.

> Source/JavaScriptCore/runtime/Watchdog.cpp:35
> +#define NEVER_TIMEOUT std::numeric_limits<double>::infinity()

WebKit style prefers something like "static const double noLimit = ..." vs
#define NO_LIMIT.

> Source/JavaScriptCore/runtime/Watchdog.h:82
> +    bool m_timerDidFire;

This data member should include a comment that it is set by a secondary thread.
You have a comment above next to a function, but it's clearer to think about
thread-safety in terms of the data being accessed.

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:47
> +	   dispatch_source_cancel(m_timer);

Why do we need to cancel inside the fire handler? Once the timer fires, we
wouldn't expect it to fire again.

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:48
> +	   timerDidFire();

I think this would be clearer if it just assigned directly to m_timerDidFire
instead of calling an abstract function. There's no need for abstraction here,
since we're inside the class's internal implementation functions, and this is
the only line of code that will assign to the data member.

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:56
> +    dispatch_source_cancel(m_timer);

dispatch_source_cancel is asynchronous, so this line of code does not
synchronize with any code that might fire m_timer. Therefore, it's possible to
execute this code, then return and delete the Watchdog, then observe the timer
firing, writing to the deleted Watchdog's data member.