Bug 77370 - Before accessing a frame's script controller in v8 bindings, first check that the frame actually exists
: Before accessing a frame's script controller in v8 bindings, first check that...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: jochen
:
Depends on: 77692
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-30 14:42 PST by jochen
Modified: 2012-04-13 09:34 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.12 KB, patch)
2012-01-30 14:43 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (7.10 KB, patch)
2012-02-06 05:49 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (4.39 KB, patch)
2012-02-06 13:31 PST, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2012-01-30 14:42:09 PST
Before accessing a frame's script controller in v8 bindings, first check that the frame actually exists
Comment 1 jochen 2012-01-30 14:43:57 PST
Created attachment 124602 [details]
Patch
Comment 2 Alexey Proskuryakov 2012-01-30 23:58:22 PST
Is this covered by existing tests?
Comment 3 Adam Barth 2012-01-31 00:05:35 PST
> 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.
Comment 4 jochen 2012-01-31 00:38:17 PST
(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()
Comment 5 jochen 2012-01-31 08:25:16 PST
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?
Comment 6 jochen 2012-02-06 05:49:14 PST
Created attachment 125625 [details]
Patch
Comment 7 jochen 2012-02-06 05:53:56 PST
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 8 jochen 2012-02-06 06:56:53 PST
Comment on attachment 125625 [details]
Patch

layout test is missing expectations.
Comment 9 Adam Barth 2012-02-06 10:56:51 PST
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.
Comment 10 jochen 2012-02-06 11:28:37 PST
(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
Comment 11 jochen 2012-02-06 13:31:49 PST
Created attachment 125702 [details]
Patch
Comment 12 Adam Barth 2012-02-06 13:35:20 PST
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 13 jochen 2012-02-06 13:56:00 PST
Comment on attachment 125702 [details]
Patch

Clearing flags on attachment: 125702

Committed r106846: <http://trac.webkit.org/changeset/106846>
Comment 14 jochen 2012-02-06 13:57:07 PST
I leave this bug open to track this issue until I have a proper layout test.
Comment 15 jochen 2012-04-13 08:09:43 PDT
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?
Comment 16 Adam Barth 2012-04-13 09:34:33 PDT
Ok.  It's too bad we weren't able to find a test here.