Bug 116524 - Active DOM objects stopped twice
Summary: Active DOM objects stopped twice
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-21 01:39 PDT by Allan Sandfeld Jensen
Modified: 2013-05-27 02:12 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.44 KB, patch)
2013-05-21 01:41 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (1.97 KB, patch)
2013-05-23 01:45 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (1.97 KB, patch)
2013-05-23 02:45 PDT, Allan Sandfeld Jensen
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2013-05-21 01:39:47 PDT
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.
Comment 1 Allan Sandfeld Jensen 2013-05-21 01:41:16 PDT
Created attachment 202398 [details]
Patch
Comment 2 Alexey Proskuryakov 2013-05-21 09:56:39 PDT
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().
Comment 3 Allan Sandfeld Jensen 2013-05-21 11:10:15 PDT
(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.
Comment 4 Allan Sandfeld Jensen 2013-05-23 01:45:04 PDT
Created attachment 202649 [details]
Patch
Comment 5 Allan Sandfeld Jensen 2013-05-23 02:45:32 PDT
Created attachment 202653 [details]
Patch

Fix exposed bug, where in rare cases active dom objects were created after they should have been stopped
Comment 6 Alexey Proskuryakov 2013-05-23 10:43:49 PDT
The latest patch still doesn't have the assert, is that intentional?
Comment 7 Allan Sandfeld Jensen 2013-05-23 13:01:42 PDT
(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.
Comment 8 Alexey Proskuryakov 2013-05-23 13:12:48 PDT
Makes sense.

> I discovered a case where the assert was triggered.

Can you make a regression test from that?
Comment 9 Allan Sandfeld Jensen 2013-05-24 03:06:04 PDT
(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.
Comment 10 Alexey Proskuryakov 2013-05-24 09:51:41 PDT
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.
Comment 11 Allan Sandfeld Jensen 2013-05-27 02:12:59 PDT
Committed r150741: <http://trac.webkit.org/changeset/150741>