Bug 77370

Created attachment 124602 [details] Patch

Is this covered by existing tests?

> Is this covered by existing tests? I chatted with Jochen about ideas for how to test this patch. He's going to try writing some tests. This bug also seems related to https://bugs.webkit.org/show_bug.cgi?id=53733, which might be the right way to solve some of these problems.

(In reply to comment #3) > > Is this covered by existing tests? > > I chatted with Jochen about ideas for how to test this patch. He's going to try writing some tests. This bug also seems related to https://bugs.webkit.org/show_bug.cgi?id=53733, which might be the right way to solve some of these problems. Here's the crash stack btw Thread 0 *CRASHED* ( EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000428 ) 0x65b93127 [chrome.dll - scriptcontrollerbase.cpp:40 WebCore::ScriptController::canExecuteScripts(WebCore::ReasonForCallingCanExecuteScripts) 0x65d984f0 [chrome.dll - scheduledaction.cpp:100 WebCore::ScheduledAction::execute(WebCore::ScriptExecutionContext *) 0x65d97969 [chrome.dll - domtimer.cpp:148 WebCore::DOMTimer::fired() 0x65a84ffa [chrome.dll - threadspecific.h:251 WTF::ThreadSpecific<WebCore::ThreadGlobalData>::operator WebCore::ThreadGlobalData *() 0x65a84f3c [chrome.dll - threadglobaldata.h:104 WebCore::threadGlobalData() 0x65ae3c6a [chrome.dll - algorithm:2022] std::_Push_heap<WebCore::TimerHeapIterator,int,WebCore::TimerBase *,WebCore::TimerHeapLessThanFunction>(WebCore::TimerHeapIterator,int,int,WebCore::TimerBase *,WebCore::TimerHeapLessThanFunction) 0x65af3604 [chrome.dll - algorithm:2124] std::_Adjust_heap<WebCore::TimerHeapIterator,int,WebCore::TimerBase *,WebCore::TimerHeapLessThanFunction>(WebCore::TimerHeapIterator,int,int,WebCore::TimerBase *,WebCore::TimerHeapLessThanFunction) 0x65a84ffa [chrome.dll - threadspecific.h:251 WTF::ThreadSpecific<WebCore::ThreadGlobalData>::operator WebCore::ThreadGlobalData *() 0x74ced362 [kernel32.dll + 0x0002d362] NlsGetCacheUpdateCount 0x659f6b6a [chrome.dll - observer_list.h:167 ObserverListBase<base::MessagePumpForIO::IOObserver>::Compact() 0x65a84f3c [chrome.dll - threadglobaldata.h:104 WebCore::threadGlobalData() 0x65b70047 [chrome.dll - threadtimers.cpp:93 WebCore::ThreadTimers::sharedTimerFired() 0x65a264c0 [chrome.dll - timer.h:174 base::BaseTimer<EnumerateModulesModel,0>::TimerTask::Run() 0x659f0a5c [chrome.dll - message_loop.cc:458 MessageLoop::RunTask(base::PendingTask const &) 0x659f0734 [chrome.dll - message_loop.cc:660 MessageLoop::DoWork()

I tried to reproduce this crash, but failed :( My working assumption is that a timer fires on a Document without a frame (because it was detached). However, Document::detach invokes stops all active DOM objects (that includes not yet fired timers), before nulling the frame. Apparently there's something that manages to schedule a new timeout after the document was detached?

Created attachment 125625 [details] Patch

So there are two ways setTimeout can be invoked on a detached Document: One is during detaching: each from is completely detached before its next sibling is detached. So by keeping a reference to a javascript function in one of you siblings, and invoking it in the unonload handler, you can run javascript on a document without a frame (this is covered by the layout test). The second way is when the load event of a Document without a frame is fired. I haven't been able to reproduce this in a layout test, however.

Comment on attachment 125625 [details] Patch layout test is missing expectations.

Comment on attachment 125625 [details] Patch Thanks! This patch is much better with the test. Feel free to land it once you've got expectations.

(In reply to comment #9) > (From update of attachment 125625 [details]) > Thanks! This patch is much better with the test. Feel free to land it once you've got expectations. Problem is, the test is wrong :( So it turns out that this test indeed triggers the code path that results in the CRASH() I added (so a DOMTimer is created on a Document without a Frame). However, the FrameLoaderClient will subsequently invoke Frame::createView which ends up invoking Document::detach which kills the timer. tl;dr the actual error in ScheduledAction::execute is not triggered. So my guess is that the second code path is leading to the real crash, and I'll continue to try to find a repro

Created attachment 125702 [details] Patch

Comment on attachment 125702 [details] Patch I appreciate the work you've done to track down the source of this problem. Normally we require a test before making changes like this one, but given that effort you've put in to finding a test, I think it's ok to land this patch now and continue to work on finding a test in a subsequent patch.

Comment on attachment 125702 [details] Patch Clearing flags on attachment: 125702 Committed r106846: <http://trac.webkit.org/changeset/106846>

I leave this bug open to track this issue until I have a proper layout test.

I tried to come up with a layout test for this, but I failed. All promising paths turned out to be not desirable behavior and I filed separate bugs for them (80595, 80599, 78240). So barring any objections, I'd mark this bug as closed?

Ok. It's too bad we weren't able to find a test here.