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
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 77692
:
  Show dependency treegraph
 
Reported: 2012-01-30 14:42 PST by
Modified: 2012-04-13 09:34 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2012-01-30 14:43:57 PST -------
Created an attachment (id=124602) [details]
Patch
------- Comment #2 From 2012-01-30 23:58:22 PST -------
Is this covered by existing tests?
------- Comment #3 From 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 From 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 From 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 From 2012-02-06 05:49:14 PST -------
Created an attachment (id=125625) [details]
Patch
------- Comment #7 From 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 From 2012-02-06 06:56:53 PST -------
(From update of attachment 125625 [details])
layout test is missing expectations.
------- Comment #9 From 2012-02-06 10:56:51 PST -------
(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.
------- Comment #10 From 2012-02-06 11:28:37 PST -------
(In reply to comment #9)
> (From update of attachment 125625 [details] [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 From 2012-02-06 13:31:49 PST -------
Created an attachment (id=125702) [details]
Patch
------- Comment #12 From 2012-02-06 13:35:20 PST -------
(From update of attachment 125702 [details])
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 From 2012-02-06 13:56:00 PST -------
(From update of attachment 125702 [details])
Clearing flags on attachment: 125702

Committed r106846: <http://trac.webkit.org/changeset/106846>
------- Comment #14 From 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 From 2012-04-13 08:09:43 PST -------
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 From 2012-04-13 09:34:33 PST -------
Ok.  It's too bad we weren't able to find a test here.