RFC: Deprecating the "WEBKIT" browser thread

[Jonathan Dixon]

For various reasons we'd like to make --single-process, or at least renderer-in-process, a more faithful representation of the complete chrome stack running. One thing impeding this is the webkit thread does not run at all in single-process mode, meaning code that depends on it at best follows different code paths, or just won't work.

I played around with trying to get the in-process renderer and webkit thread to share a thread, but that's fraught with deadlocks, so the preferred solution now is to deprecate the webkit thread (details in http://crbug.com/106839)

What the would mean in practice:

1/ New features should not be designed to use webkit code inside the browser process,

2/ Over coming months changes will land to rework the existing users (DOM Storage and Index DB)

Please let me know if this would adversely impact you or you know of something else that make this plan unworkable.

If I don't hear anything major, I'll land http://codereview.chromium.org/8879013/ in the next couple days (the zeroth step, renaming it to BrowserThread::WEBKIT_DEPRECATED)

Cheers

[Antoine Labour]

For my part, this is a welcome change. On Aura we are using the WebKit compositor in the browser process, which is mostly isolated from WebKit (thankfully because we need to call it from the UI thread), but requires WebKit to be initialized, which, in the current model, means forcing the WEBKIT thread to start early (because WebKit can't be initialized more than once, and it has to be initialized on its "main" thread). It works, but is very fragile.

While we're working on extracting the compositor out of WebKit to remove that dependency (among others), it will take some time, and removing the WebKit thread should help significantly.

Just keep me in the loop when you're about to remove the thread, so that we can replace it in the Aura case by and explicit WebKit initialization.

Thanks,

Antoine

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Michael Nordman]

+1 die WEBKIT_THREAD die

[Jochen Eisinger]

On what thread will we initialize webkit?

Did I mention that webkit is not thread-safe (even though it has multiple threads...)

but +1 to making webkit thread die anyway!

-jochen

[Michael Nordman]

On Thu, Dec 15, 2011 at 11:01 AM, Jochen Eisinger <joc...@chromium.org> wrote:

On what thread will we initialize webkit?

True, that we will have to call init on it in order to use things like WebSecurityOrigin (so global icu tables get initialized and such) but it shouldn't really matter what thread that's done on...once http://codereview.chromium.org/8416019/ lands, an arbitrary background thread should suffice.

[Darin Fisher]

I know there is a plan to rework the IndexedDB backend for this, but it seems like LocalStorage needs similar treatment.  Do you plan to help rework the LocalStorage backend?

-Darin

On Thu, Dec 15, 2011 at 9:47 AM, Jonathan Dixon <jo...@chromium.org> wrote:

[Jochen Eisinger]

On Dec 15, 2011 8:18 PM, "Michael Nordman" <mich...@google.com> wrote:
>
>
>
> On Thu, Dec 15, 2011 at 11:01 AM, Jochen Eisinger <joc...@chromium.org> wrote:
>>
>> On what thread will we initialize webkit?
>
>
> True, that we will have to call init on it in order to use things like WebSecurityOrigin (so global icu tables get initialized and such) but it shouldn't really matter what thread that's done on...once http://codereview.chromium.org/8416019/ lands, an arbitrary background thread should suffice.

We could also consider doing something like in the renderer, i.e. invoking ensureWebKitInitialized every time before calling into WebKit

[Jonathan Dixon]

On 15 December 2011 19:18, Michael Nordman <mich...@google.com> wrote:

On Thu, Dec 15, 2011 at 11:01 AM, Jochen Eisinger <joc...@chromium.org> wrote:

On what thread will we initialize webkit?

True, that we will have to call init on it in order to use things like WebSecurityOrigin (so global icu tables get initialized and such) but it shouldn't really matter what thread that's done on...once http://codereview.chromium.org/8416019/ lands, an arbitrary background thread should suffice.

 

Ah, thanks.

In a completely different effort, I'd found that WebKit::initialize was taking a descent chunk of time (~60ms in my config) and currently blocks our UI thread early in startup (as it's done in InternalWebKitThread::Init() which is synchronous wrt to the caller's thread start call)

However the simple fix (http://codereview.chromium.org/7888023) to make it async would randomly fail on try bots,  because of these WebSecurityOrigin / icu table implicit dependencies I presume. We'll need to make them explicit i.e. via a thread-safe EnsureWebkitInitialized() that blocks on the background init, if it's already in progress.

(cross post with jochen: same idea)

[Jonathan Dixon]

On 15 December 2011 19:27, Darin Fisher <da...@chromium.org> wrote:

I know there is a plan to rework the IndexedDB backend for this, but it seems like LocalStorage needs similar treatment.  Do you plan to help rework the LocalStorage backend?

Yes, Ben is lined up for this in the new year, and Michael is on board to help out as our consultant :-)

Regarding deprecating the thread, at this point renaming it is mostly to give a reminder to anyone working on new stuff.... the code will in practice probably need to hang around for some time, as I guess we'll need to use it to migrate existing databases over to the new backend. At least that should become a lazily thread startup, and only if there is something to migrate. (And never needed in environments where we don't want to migrate any data)

[Antoine Labour]

On Thu, Dec 15, 2011 at 11:40 AM, Jonathan Dixon <jo...@google.com> wrote:

On 15 December 2011 19:18, Michael Nordman <mich...@google.com> wrote:

On Thu, Dec 15, 2011 at 11:01 AM, Jochen Eisinger <joc...@chromium.org> wrote:

On what thread will we initialize webkit?

True, that we will have to call init on it in order to use things like WebSecurityOrigin (so global icu tables get initialized and such) but it shouldn't really matter what thread that's done on...once http://codereview.chromium.org/8416019/ lands, an arbitrary background thread should suffice.

 

Ah, thanks.

In a completely different effort, I'd found that WebKit::initialize was taking a descent chunk of time (~60ms in my config) and currently blocks our UI thread early in startup (as it's done in InternalWebKitThread::Init() which is synchronous wrt to the caller's thread start call)

However the simple fix (http://codereview.chromium.org/7888023) to make it async would randomly fail on try bots,  because of these WebSecurityOrigin / icu table implicit dependencies I presume.

Aura needs it sync for the same reason. But an EnsureWebkitInitialized() would work too.

Antoine

[Michael Nordman]

On Thu, Dec 15, 2011 at 11:27 AM, Darin Fisher <da...@chromium.org> wrote:

I know there is a plan to rework the IndexedDB backend for this, but it seems like LocalStorage needs similar treatment.  Do you plan to help rework the LocalStorage backend?

We've been trading mail about that, andrei volunteered ben to help with some of this, i'd be willing to spend time on it too.

There is no concrete plan of what to do by when just yet. The important thing for clank is to nuke the dependency on webcore/webkit_thread, other things (like switching the leveldb and adding a decent caching layer) are secondary. I'm thinking the first round of removing those webkit dependencies can be done w/o changing webkit APIs or data formats on disk.

[Darin Fisher]

On Thu, Dec 15, 2011 at 12:38 PM, Michael Nordman <mich...@google.com> wrote:

On Thu, Dec 15, 2011 at 11:27 AM, Darin Fisher <da...@chromium.org> wrote:

I know there is a plan to rework the IndexedDB backend for this, but it seems like LocalStorage needs similar treatment.  Do you plan to help rework the LocalStorage backend?

We've been trading mail about that, andrei volunteered ben to help with some of this, i'd be willing to spend time on it too.

There is no concrete plan of what to do by when just yet. The important thing for clank is to nuke the dependency on webcore/webkit_thread, other things (like switching the leveldb and adding a decent caching layer) are secondary. I'm thinking the first round of removing those webkit dependencies can be done w/o changing webkit APIs or data formats on disk.

Makes sense.  I'm of course interested in the other changes too :-)

-Darin

[David Levin]

On Thu, Dec 15, 2011 at 11:18 AM, Michael Nordman <mich...@google.com> wrote:

On Thu, Dec 15, 2011 at 11:01 AM, Jochen Eisinger <joc...@chromium.org> wrote:

On what thread will we initialize webkit?

True, that we will have to call init on it in order to use things like WebSecurityOrigin (so global icu tables get initialized and such) but it shouldn't really matter what thread that's done on...once http://codereview.chromium.org/8416019/ lands, an arbitrary background thread should suffice.

If you are talking about WebKit::initializeWithoutV8 (or these two calls  RenderThreadImpl::EnsureWebKitInitialized, WebKit::initialize), currently it initializes the main webkit thread to be the current thread (http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/src/WebKit.cpp&l=115), so it matters what thread it is done on currently.

Is this being fixed?

dave

[Michael Nordman]

We might want a new initializer that doesn't initialize the 'main' webkit thread or accept a WebKitPlatformSupport* parameter at all.

  WebKit::initializePrimitivesOnly()

I  see render_sandbox_host_linux.cc also inits webkit in order to use the  WebKit::WebFontInfo static methods. Maybe the very few primitive classes we have a use for outside of renderer/worker processes should have their own initializers so they can be utilized w/o initting all of webkit?

  WebSecurityOrigin::initializeClass();

  WebFontInfo::initializeClass();

 

dave

[David Levin]

Sure. fwiw, WebKit threading has to be initialized (not the main thread to my memory) before calling almost anything in WebKit now. (This just allocates a tls.) This is what the webkit init in render_sandbox_host_linux.cc  is for.

dave 

 

 

dave