Bug 77370 – Before accessing a frame's script controller in v8 bindings, first check that the frame actually exists

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Before accessing a frame's script controller in v8 bindings, first check that the frame actually exists

Created attachment 124602 [details]
Patch

[Alexey Proskuryakov]

Is this covered by existing tests?

[Adam Barth]

> 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.

[Adam Barth]

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

[Adam Barth]

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?

[Adam Barth]

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