[webkit-reviews] review granted: [Bug 52719] ScriptExecutionContext::stopActiveDOMObjects iterates a hash map that can change during iteration (for multiple reasons, including GC) : [Attachment 229767] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 52719: ScriptExecutionContext::stopActiveDOMObjects iterates a hash map
that can change during iteration (for multiple reasons, including GC)
https://bugs.webkit.org/show_bug.cgi?id=52719

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

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


r=me. Nice patch!

> Source/WebCore/ChangeLog:61
> +	   (WebCore::ScriptExecutionContext::didCreateActiveDOMObject): Ditto.
Got rid of the
> +	   CRASH because there's no compelling value to this being a release
assert instead of a
> +	   normal assert.

This comment is now obsolete.

> Source/WebCore/ChangeLog:84
> +	   * workers/WorkerGlobalScope.h: Make hasPendingActivity function from
the base class
> +	   public instead of declaring it in this class.

Longer term, I'd like to come up with a better name for it, it's quite unclear
which "pending activity" we are talking about. The workers spec has changed a
lot, it might have a better name and/or design for this concept now.

> Source/WebCore/dom/ScriptExecutionContext.cpp:188
> +    // No protection against m_activeDOMObjects changing during iteration:
canSuspend()
> +    // functions should not add new active DOM objects, nor execute
arbitrary JavaScript.

The reworded comment doesn't seem quite accurate, as removing active DOM
objects isn't OK either (and there is a debug only assertion for that).

I thought that we had a way to assert that JS was not executed, but I can't
find it now. I only found ScriptExecutionContext::isJSExecutionForbidden(),
which is a different thing, and seems like a misnomer.

It would be very useful to catch mistakes with unexpected JS execution sooner,
not just when the script does something bad.

> Source/WebCore/dom/ScriptExecutionContext.cpp:221
> +    // No protection against m_activeDOMObjects changing during iteration:
suspend()
> +    // functions should not add new active DOM objects, nor execute
arbitrary JavaScript.

Ditto.

> Source/WebCore/dom/ScriptExecutionContext.cpp:249
> +    // No protection against m_activeDOMObjects changing during iteration:
resume()
> +    // functions should not add new active DOM objects, nor execute
arbitrary JavaScript.

Ditto.

> Source/WebCore/dom/ScriptExecutionContext.cpp:277
> +    for (auto* object : objects) {

...

> Source/WebCore/dom/ScriptExecutionContext.cpp:313
> +    // The m_activeDOMObjectAdditionForbidden check is a RELEASE_ASSERT
because of the
> +    // consequences of having an ActiveDOMObject that is not correctly
reflected in the set.
> +    // If we do have one of those, it can possibly be a security
vulnerability. So we'd
> +    // rather have a crash than continue running with the set possibly
compromised.

suspendActiveDOMObjectIfNeeded() mechanism makes this almost unnecessary,
because even objects that are created during iteration will be created
suspended or stopped.

But suspendActiveDOMObjectIfNeeded() itself is very fragile - every constructor
has to call suspendIfNeeded() for it to work - so it's best to keep this
protection I think.