Move FrameLoader completion check timer to loading task runner. (issue 2172153002 by dcheng@chromium.org)

[dch...@chromium.org]

Reviewers: alex clarke
CL: https://codereview.chromium.org/2172153002/

Description:
Move FrameLoader completion check timer to loading task runner.

BUG=624694

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+9, -6 lines):
M third_party/WebKit/Source/core/loader/FrameLoader.cpp
M third_party/WebKit/Source/platform/Timer.h


Index: third_party/WebKit/Source/core/loader/FrameLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index 3137c89265b684f69598e0e809d913f3903c6e5a..d33df9db829233465d09fee716158635e555190d 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -40,6 +40,7 @@
#include "core/HTMLNames.h"
#include "core/dom/Document.h"
#include "core/dom/Element.h"
+#include "core/dom/TaskRunnerHelper.h"
#include "core/dom/ViewportDescription.h"
#include "core/editing/Editor.h"
#include "core/events/GestureEvent.h"
@@ -168,7 +169,7 @@ FrameLoader::FrameLoader(LocalFrame* frame)
, m_progressTracker(ProgressTracker::create(frame))
, m_loadType(FrameLoadTypeStandard)
, m_inStopAllLoaders(false)
- , m_checkTimer(this, &FrameLoader::checkTimerFired)
+ , m_checkTimer(this, &FrameLoader::checkTimerFired, TaskRunnerHelper::getLoadingTaskRunner(frame))
, m_didAccessInitialDocument(false)
, m_didAccessInitialDocumentTimer(this, &FrameLoader::didAccessInitialDocumentTimerFired)
, m_forcedSandboxFlags(SandboxNone)
Index: third_party/WebKit/Source/platform/Timer.h
diff --git a/third_party/WebKit/Source/platform/Timer.h b/third_party/WebKit/Source/platform/Timer.h
index 0a9503f9e72360f70b0496b8b135b01f46b5a61b..8245b3adbf1a499f5fbfa49ee5816444badc9fcf 100644
--- a/third_party/WebKit/Source/platform/Timer.h
+++ b/third_party/WebKit/Source/platform/Timer.h
@@ -159,11 +159,18 @@ class Timer : public TimerBase {
public:
using TimerFiredFunction = void (TimerFiredClass::*)(Timer<TimerFiredClass>*);

+ // TODO(dcheng): Consider removing this overload once all timers are using the
+ // appropriate task runner. https://crbug.com/624694
Timer(TimerFiredClass* o, TimerFiredFunction f)
: m_object(o), m_function(f)
{
}

+ Timer(TimerFiredClass* o, TimerFiredFunction f, WebTaskRunner* webTaskRunner)
+ : TimerBase(webTaskRunner), m_object(o), m_function(f)
+ {
+ }
+
~Timer() override { }

protected:
@@ -181,11 +188,6 @@ protected:
return TimerIsObjectAliveTrait<TimerFiredClass>::isHeapObjectAlive(m_object);
}

- Timer(TimerFiredClass* o, TimerFiredFunction f, WebTaskRunner* webTaskRunner)
- : TimerBase(webTaskRunner), m_object(o), m_function(f)
- {
- }
-
private:
// FIXME: Oilpan: TimerBase should be moved to the heap and m_object should be traced.
// This raw pointer is safe as long as Timer<X> is held by the X itself (That's the case

[dch...@chromium.org]

+haraken for platform changes.

https://codereview.chromium.org/2172153002/

[har...@chromium.org]

On 2016/07/22 07:04:40, dcheng wrote:
> +haraken for platform changes.

LGTM on my side.



https://codereview.chromium.org/2172153002/

[dch...@chromium.org]

Hmm, it appears this makes some tests unhappy. Investigating...

https://codereview.chromium.org/2172153002/

[alexc...@chromium.org]

Let me know when the tests are happy and I'll take a look.

https://codereview.chromium.org/2172153002/

[dch...@chromium.org]

I poked at this a bit more today, focusing on the unit tests first. The problem
is a UaF during test shutdown. I'm still working on figuring out why my CL
creates this issue though.

When the VirtualTimeTest.AllowVirtualTimeToAdvance finishes running, the first
thing that happens is the SimTest framework is shutdown. This destroys the
WebViewImpl, its WebViewScheduler, and its AutoAdvancingVirtualTimeDomain.

Later on, we shutdown Blink, which calls RendererSchedulerImpl::Shutdown(). This
ends up destroying the TaskQueueManager. However, the TaskQueueImpl holds a raw
TimeDomain* pointer to the already deleted AutoAdvancingVirtualTimeDomain, and
everything explodes when we try to call into the already-deleted object.

I'm not yet entirely sure how to fix this, and I notice //components/scheduler
doesn't have a README. Are there some docs I can read to understand how this
architecture all works together?

Also, as an aside, why is this in //components instead of //content? Do we
expect to reuse these components for iOS? They seem quite Blink-specific.

https://codereview.chromium.org/2172153002/

[alexc...@chromium.org]

On 2016/07/25 15:51:06, dcheng wrote:
> I poked at this a bit more today, focusing on the unit tests first. The
problem
> is a UaF during test shutdown. I'm still working on figuring out why my CL
> creates this issue though.
>
> When the VirtualTimeTest.AllowVirtualTimeToAdvance finishes running, the first
> thing that happens is the SimTest framework is shutdown. This destroys the
> WebViewImpl, its WebViewScheduler, and its AutoAdvancingVirtualTimeDomain.
>
> Later on, we shutdown Blink, which calls RendererSchedulerImpl::Shutdown().
This
> ends up destroying the TaskQueueManager. However, the TaskQueueImpl holds a
raw
> TimeDomain* pointer to the already deleted AutoAdvancingVirtualTimeDomain, and
> everything explodes when we try to call into the already-deleted object.

Oh I see. Sorry about that, I'll try and fix that tomorrow.

> I'm not yet entirely sure how to fix this, and I notice //components/scheduler
> doesn't have a README. Are there some docs I can read to understand how this
> architecture all works together?

There's some discussion of VirtualTime here
https://docs.google.com/document/d/1y9KDT_ZEzT7pBeY6uzVt1dgKlwc1OB_vY4NZO1zBQmo/edit

I don't think there's a current accurate overview of the Scheduler architecture.
We should probably write one.

>
> Also, as an aside, why is this in //components instead of //content? Do we
> expect to reuse these components for iOS? They seem quite Blink-specific.

That's a long story, as it happens the scheduler was in //content at one time.
Anyway we have plans to move it back into blink where it was originally.



https://codereview.chromium.org/2172153002/

[dch...@chromium.org]

Alright, (at least one of the) other issues is due to a use-after-free of the
frame's BlameContext. It's a bit hard to repro: I wasn't able to trigger it when
running only one test, but when I use an asan build, the following command:

out_asan/Release/content_browsertests --gtest_filter=SitePerProcess*
--test-launcher-bot-mode --test-launcher-retry-limit=0 |&
tools/valgrind/asan/asan_symbolize.py

The stack shows that the freed pointer being derefed is a BlameContext, which is
an unowned pointer that (optionally?) is associated with a TaskQueueImpl.

When we detach a frame, we call into the embedder
(content::RenderFrameImpl::frameDetached) to do some cleanup. This destroys the
FrameBlameContext. The corresponding Timer in FrameLoader is also cancelled, and
the queued Task is abandoned.

However, queued Tasks are still in the TaskQueueImpl: when it tries to process
the pending tasks, it calls NotifyWillProcessTask, which references the already
deleted FrameBlameContext.

This is a bit surprising, because it looks like we go to a lot of effort to try
to clean up the WebFrameSchedulerImpl when we detach: in fact, we reset the
pointer twice. In theory, this should delete the associated TaskQueues and
unregister it from the TaskQueueManager so new tasks don't get queued.

1) I'm not sure why we lazily-create the frame scheduler, shouldn't we just
eagerly initialize it when we create the frame?

2) Similarly, I think we should just reset the frame scheduler later in
LocalFrame::detach to avoid the need to double-reset.

https://codereview.chromium.org/2172153002/

[dch...@chromium.org]

Well, it looks like not lazily creating the scheduler fixes all the problems
(including the unit tests). PTAL!

https://codereview.chromium.org/2172153002/

[alexc...@chromium.org]

On 2016/07/26 11:05:31, dcheng wrote:
> Well, it looks like not lazily creating the scheduler fixes all the problems
> (including the unit tests). PTAL!

Oh interesting, that matches more closely the non-test behavior.

LGTM

https://codereview.chromium.org/2172153002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2172153002/20001


https://codereview.chromium.org/2172153002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #2 (id:20001)

https://codereview.chromium.org/2172153002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 2 (id:??) landed as
https://crrev.com/587adda57c8c304adcd080b5f1d6bad49ce588fd
Cr-Commit-Position: refs/heads/master@{#407767}

https://codereview.chromium.org/2172153002/