Bug 22755 – Next step to Timers in Workers: moving timer code from other places to DOMTimer.

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

[Dmitry Titov]

This prepares for adding means to JSScriptContext to create its own timers (except threading stuff that is separate):
- Moved more timer code from JSDOMWindowBase to DOMTimer. Removed timerFired() and simplified install/removeTimeout so when JSWorkerContext will get similar functionality, there wil be no code duplication.
- Moved TimeoutMap from Document to be a static member on DOMTimer. Also removed addTimeout/removeTimeout from Document. Same reason as above.
- Removed JSLock lock(false) local variable in ~DOMTimer. It doesn't seem to be needed there.
- Since Timers now are not recreated across modal operations, the code that updates interval to be at least 10ms is simpler now.

Next steps:
- teach ScheduledAction how to execute using WorkerContext in addition to Document.
- make DOMTimer working on a Worker thread
- expose timers off WorkerContext

This makes it easy to add create/remove timeout methods to JSWorkerContext later since they just create/delete DOMTimer object.

[Dmitry Titov]

Created attachment 25991 [details]
Proposed patch

[Darin Adler]

Comment on attachment 25991 [details]
Proposed patch

I'm guessing that a lot of these comments are about code that you just moved, and didn't write, so you can ignore those comments if you like. If it was new code I'd probably fix all these things!

> +const int cMaxTimerNestingLevel = 5;
> +const double cMinimumTimerInterval = 0.010;

Since these are local to this source file, they should be "static const". Our coding style doesn't give a specific style for these, but I normally use no prefix at all (so no "c"), but the names are arguably OK as is. It'd be nice to be consistently use either "maximum" and "minimum" or "max" and "min". This uses one of each.

> +DOMTimer::TimeoutsMap& DOMTimer::getTimeoutsMap()
> +{
> +    DEFINE_STATIC_LOCAL(TimeoutsMap, map, ());
> +    return map;
> +}

We never use the word "get" in the name of a function like this. We reserve "get" for functions that return their results in a non-standard way, such as an out parameter. We'd just call this timeoutsMap().

> +    ASSERT(!getTimeoutsMap().get(m_timeoutId));

We have "contains" for boolean uses like this. Better to use that instead of get.

> +    getTimeoutsMap().set(m_timeoutId, this);
> +
> +    // Use a minimum interval of 10 ms to match other browsers, but only once we've
> +    // nested enough to notice that we're repeating.
> +    // Faster timers might be "better", but they're incompatible.

The comment above describes the "interval" part of the math, but the immediately succeeding lines of code implement something else (conversion from ms to s and minimum interval of 1 ms). I find that confusing.

> +    double interval = std::max<double>(0.001, timeout * 0.001);

The 0.001 should be a named constant too. Maybe it's clear to you that they're milliseconds, but I found it unclear.

Why max<double> instead of just max? Both expressions are doubles, so it should work without the type being specified explicitly.

> +    typedef HashMap<int, DOMTimer*> TimeoutsMap;
> +    static TimeoutsMap& getTimeoutsMap();

Does this really need to be a static member function? If it was a plain old non-member function then it could be in the .cpp file and we could avoid adding new includes to the header. I'd prefer that if possible.

> +    DOMTimer* timer = new DOMTimer(scriptExecutionContext(), a, t, singleShot);

I'm not really comfortable with this interface. It's ugly that calling new gives you a pointer that's already owned by the "window" object; sets off all the alarm bells -- who owns this object? Could we change this to be a function call rather than new? The "new" part could be done inside the function so it's encapsulated in the DOMTimer class.

Is a global map really a good idea if we're going to be doing this on multiple threads?

> +    delete DOMTimer::findById(timeoutId);

It seems cleaner to have the DOMTimer function be more like "removeById".

It's a bug that now one window can cancel timers that were created in another window if it guesses the timer ID. That needs to be fixed.

review- for now given the comments above.

[Darin Adler]

Comment on attachment 25991 [details]
Proposed patch

In this patch, what deletes all the timers that are still running when a window goes away?

[Dmitry Titov]

(In reply to comment #3)
> (From update of attachment 25991 [details] [review])
> In this patch, what deletes all the timers that are still running when a window
> goes away?

It happens when a Document associated with the window is deleted and calls contextDestroyed() on all timers. There is 'delete this' in that override on a DOMTimer. The timers are stopped even before that, from FrameLoader::clear() or Document::detach(), depending which way the document is destroyed.

Updated patch is on the way.

[Dmitry Titov]

Created attachment 26084 [details]
updated patch

Changed code per feedback from Darin.

I've moved timeoutsMap back to Document (undoing the part of the change) since making it a global was not a good idea. It would have issues listed below, which would unnecessary complicate the code, so I left it on the Document with the idea to move it to ScriptExecutionContext very soon so Workers could use it too:
- It would have to become ThreadSpecific in case of using in Workers
- It would have to become a map-of-maps or something else to separate IDs for each window
- the code removing IDs from this map would be potentially a mem-leak-prone code.

[Dmitry Titov]

Created attachment 26085 [details]
updated patch

A few small formatting fixes added.

[Dmitry Titov]

I fixed all issues mentioned by Darin. The bug where different windows shared same timeout id space and could 'guess' and clear each other's timers is fixed by moving timeoutId map back to Document where it was, for the reasons mentioned above. I think I'll move it to ScriptExecutionContext in a separate patch. 
This patch seems to be ready for a second look. Thanks!

[Darin Adler]

Comment on attachment 26085 [details]
updated patch

> +    double intervalMilliseconds = std::max(oneMillisecond, timeout * oneMillisecond);

Should just be "max", with "using namespace std" at the top of the .cpp file.

>  DOMTimer::~DOMTimer()
>  {
> -    if (m_action) {
> -        JSC::JSLock lock(false);
> -        delete m_action;
> -    }
> +    delete m_action;
> +}

Given that we don't need locking around the delete now, we can move to using an OwnPtr instead of explicit deletion.

> +int DOMTimer::install(ScriptExecutionContext* context, ScheduledAction* action, int timeout, bool singleShot)
> +{
> +    DOMTimer* timer = new DOMTimer(context, action, timeout, singleShot);
> +    return timer->m_timeoutId;
> +}

This needs a comment to explain ownership. It's always confusing when someone can just call "new" and then drop the result and somehow know there won't be a storage leak. So requires a brief comment.

> +    // FIXME: move the timeout map and API to ScriptExecutionContext to be able
> +    // to create timeouts from Workers.

I don't think we need this comment in multiple places. Also, please use sentence capitalization in comments like this. Thus "Move" rather than "move".

> +    // This is static, so ok to assign after deleting the instance.
> +    m_timerNestingLevel = 0;

I don't think static data members should use the "m_" prefix; the need for this comment is a good example of why. But further, I'd rather just have global variables with internal linkage rather than static data members anyway. They are private to the .cpp file and can be added and removed without touching a header. Anyway, not new to this patch and not your fault.

r=me

[Dmitry Titov]

Thanks a lot! 

Fixed everything.  The patch is coming after tests are completed.

After switching to OwnPtr for ScheduledAction the DOMTimer destructor becomes empty. I have left it in cpp file to avoid including ScheduleAction.h in DOMTimer.h (otherwise compiler tries to generate default destructor for DOMTimer). Also makes it possible to set a breakpoint into destructor.

[Dmitry Titov]

Created attachment 26274 [details]
Addressed Darin's comments, final version.

I've addressed all the Darin's comments. Since it's r+, I think the final version of the patch is ready to be landed.

[Alexey Proskuryakov]

Committed revision 39489. Fixed one typo in a newly added comment (ActivDOMObject).