Active DOM objects stopped twice

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

ScriptExecutionContext::stopActiveDOMObjects() is called from a few different places. At least two of these are triggered on most application close. The call is probably needed both places, but we could save stopping each dom object, since ScriptExecutionContext remembers all objects should be stopped.

This clears up some debugging, but might also help avoid surprises in new active DOM objects.

[Allan Sandfeld Jensen]

Created attachment 202398 [details]
Patch

[Alexey Proskuryakov]

Are there any protections against adding new ActiveDOMObjects when m_activeDOMObjectsAreStopped is true?

We have lots of code paths that can execute scripts and create new objects in half-closed state, so I'm worried about essentially removing the last pass of stopActiveDOMObjects().

[Allan Sandfeld Jensen]

(In reply to comment #2)
> Are there any protections against adding new ActiveDOMObjects when m_activeDOMObjectsAreStopped is true?
> 
No, not currently. I added an assert against it before running the tests, I guess it would be a good idea to include that in the patch as well.

[Allan Sandfeld Jensen]

Created attachment 202649 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 202653 [details]
Patch

Fix exposed bug, where in rare cases active dom objects were created after they should have been stopped

[Alexey Proskuryakov]

The latest patch still doesn't have the assert, is that intentional?

[Allan Sandfeld Jensen]

(In reply to comment #6)
> The latest patch still doesn't have the assert, is that intentional?

Yes. I discovered a case where the assert was triggered. This is in all likelyhood a preexisting bug, but since it was possible to handle the case, I changed the assert to a check that stops active dom elements created after the document has started to stop active dom objects.

[Alexey Proskuryakov]

Makes sense.

> I discovered a case where the assert was triggered.

Can you make a regression test from that?

[Allan Sandfeld Jensen]

(In reply to comment #8)
> Makes sense.
> 
> > I discovered a case where the assert was triggered.
> 
> Can you make a regression test from that?

It is already triggered by two regression tests fast/dom/xmlhttprequest-constructor-in-detached-document.html and fast/dom/Window/timer-null-script-execution-context.html. I somehow missed that the first time.

[Alexey Proskuryakov]

Comment on attachment 202653 [details]
Patch

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.

[Allan Sandfeld Jensen]

Committed r150741: <http://trac.webkit.org/changeset/150741>