[webkit-reviews] review granted: [Bug 188426] JSRunLoopTimer may run part of a member function after it's destroyed : [Attachment 347102] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 188426: JSRunLoopTimer may run part of a member function after it's
destroyed
https://bugs.webkit.org/show_bug.cgi?id=188426

Attachment 347102: patch

https://bugs.webkit.org/attachment.cgi?id=347102&action=review




--- Comment #60 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 347102
  --> https://bugs.webkit.org/attachment.cgi?id=347102
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347102&action=review

r=me.  I agree that this patch is much easier to reason about in terms of
synchronization because it's now reduced to sync'ing on the Manager's lock, and
the Manager is immortal.

> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:194
> +    EpochTime fireEpochTime = epochTime(delay);

At first, I was concerned that delay may be negative, and that we'll have some
underflow issue with fireEpochTime.  However, I see that EpochTime is Seconds,
and therefore uses a double as storage.  I still think that it's strange to
have a negative epoch time but I suppose the math can't be messed up because
we're doing doubles math.

> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:208
> +	   if (entry.first.ptr() == &timer) {
> +	       entry.second = fireEpochTime;
> +	       found = true;
> +	   }
> +	   scheduleTime = std::min(scheduleTime, entry.second);

Shouldn't we break out of the loop if we've found the timer?  Each timer may
only be enqueued in data.timers once, right?

Shouldn't the setting of "scheduleTime = std::min(scheduleTime, entry.second);"
only be done if the timer is found?  Oh, I see.   You want to find the smallest
time of all the JSRunLoopTimers.  Not sure if it's worth it but if you just
cache this smallest time value in a PerVMData field, then you can bail out as
soon as you've found the matching timer. ... On second thought, probably not
worth it.  This is just an optimization which we can apply later if needed. 
For now, symmetry with the cancelTimer() case is probably more important to
ensure correctness.

> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:325
> +	   Manager::shared().scheduleTimer(*this, intervalInSeconds);

nit: pass the locker to document the contract that we must first lock the
JSRunLoopTimer?

> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:337
> +    Manager::shared().cancelTimer(*this);

nit: pass the locker to document the contract that we must first lock the
JSRunLoopTimer?

> Source/JavaScriptCore/runtime/JSRunLoopTimer.h:64
> +	   void willDestroyVM(VM&);

nit: name this unregisterVM for symmetry?