Bug 116524

Summary: Active DOM objects stopped twice
Product: WebKit Reporter: Allan Sandfeld Jensen <allan.jensen>
Component: WebCore Misc.Assignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, esprehn+autocc, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch ap: review+, ap: commit-queue-

Allan Sandfeld Jensen
Reported 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.
Attachments
Patch (1.44 KB, patch)
2013-05-21 01:41 PDT, Allan Sandfeld Jensen
no flags
Patch (1.97 KB, patch)
2013-05-23 01:45 PDT, Allan Sandfeld Jensen
no flags
Patch (1.97 KB, patch)
2013-05-23 02:45 PDT, Allan Sandfeld Jensen
ap: review+
ap: commit-queue-
Allan Sandfeld Jensen
Comment 1 2013-05-21 01:41:16 PDT
Alexey Proskuryakov
Comment 2 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().
Allan Sandfeld Jensen
Comment 3 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.
Allan Sandfeld Jensen
Comment 4 2013-05-23 01:45:04 PDT
Allan Sandfeld Jensen
Comment 5 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
Alexey Proskuryakov
Comment 6 2013-05-23 10:43:49 PDT
The latest patch still doesn't have the assert, is that intentional?
Allan Sandfeld Jensen
Comment 7 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.
Alexey Proskuryakov
Comment 8 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?
Allan Sandfeld Jensen
Comment 9 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.
Alexey Proskuryakov
Comment 10 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.
Allan Sandfeld Jensen
Comment 11 2013-05-27 02:12:59 PDT
Note You need to log in before you can comment on or make changes to this bug.