[webkit-reviews] review granted: [Bug 116524] Active DOM objects stopped twice : [Attachment 202653] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Allan Sandfeld Jensen
<allan.jensen at digia.com>'s request for review:
Bug 116524: Active DOM objects stopped twice
https://bugs.webkit.org/show_bug.cgi?id=116524

Attachment 202653: Patch
https://bugs.webkit.org/attachment.cgi?id=202653&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=202653&action=review


r=me if you agree with the comment below, and address it.

It's very nice to get rid of unnecessary iterations in release builds, and even
better to fix a bug at the same time.

> Source/WebCore/ChangeLog:8
> +	   Only iterate over all active DOM object and stop them once.

Please mention how you also fixed a bug with this change. Also how it's covered
by existing tests.

Have you considered making a test that would demonstrate the fix even in
release builds? Just asking here, I'm not sure if that's feasible.

> Source/WebCore/dom/ScriptExecutionContext.cpp:216
> +    if (m_activeDOMObjectsAreStopped)
> +	   return;

This early return basically gets rid of assertions that we used to always run:

ASSERT((*iter)->suspendIfNeededCalled());
ASSERT((*iter)->scriptExecutionContext() == this);

I think that they are important. Please make sure that they are run in this
case, too.