Summary: | Before accessing a frame's script controller in v8 bindings, first check that the frame actually exists | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||||||
Component: | New Bugs | Assignee: | jochen | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, ap, japhet, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 77692 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
jochen
2012-01-30 14:42:09 PST
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. |