Make the v8 Isolate used in the proxy resolver explicit.

Make the v8 Isolate used in the proxy resolver explicit.

For conceptual and performance reasons, the v8 API will require explicit passing
of Isolates in the future instead of relying on the retrieval of one out of thin
air (= either TLS or an evil global variable in v8). This CL makes the Isolate
explicit in ProxyResolverV8::Context.

The default Isolate is retrieved and remembered in the main browser thread and
then passed down to the Context where it is needed in different threads.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180134

[Sven Panne, 2013-01-18 13:13:18]

Here is my first attempt to thread down v8's default isolate to the proxy
resolver class. Perhaps a good point to start the review is proxy_resolver_v8.cc
(because there the correct isolate is actually needed) and io_thread.cc (because
there it is retrieved in the main thread and all the plumbing fun starts).

[jochen (gone - plz use gerrit), 2013-01-18 13:29:07]

can you add a BUG= line (and probably also file a bug on crbug.com)?

https://chromiumcodereview.appspot.com/11959029/diff/13014/chrome/browser/io_...
File chrome/browser/io_thread.h (right):

https://chromiumcodereview.appspot.com/11959029/diff/13014/chrome/browser/io_...
chrome/browser/io_thread.h:172: // The v8 default Isolate for the main thread.
the comment is a bit misleading, since we'll use it on the IO thread.

isn't it also the case that it's only safe to use this isolate from one thread
at a time, so whoever uses it needs to make sure to lock v8 user a V8::Locker?

[Sven Panne, 2013-01-18 14:27:33]

Good idea: We already have a v8 bug to track this (making Isolate explicit in
the API in general), I'll add this...

https://chromiumcodereview.appspot.com/11959029/diff/13014/chrome/browser/io_...
File chrome/browser/io_thread.h (right):

https://chromiumcodereview.appspot.com/11959029/diff/13014/chrome/browser/io_...
chrome/browser/io_thread.h:172: // The v8 default Isolate for the main thread.
On 2013/01/18 13:29:07, jochen wrote:
> the comment is a bit misleading, since we'll use it on the IO thread.

OK, it is v8's default Isolate, retrieved *from* the main thread. I'll change
that...

> isn't it also the case that it's only safe to use this isolate from one thread
> at a time, so whoever uses it needs to make sure to lock v8 user a V8::Locker?

It's a bit the other way round: We actually need the Isolate in the (Un)Locker
constructor to allow any usage of it in the current thread.

V8's API is a bit ugly regarding this: Compared to Java's JNI, we don't really
distinguish between JavaVM and JNIEnv, and we mix Attach/DetachCurrentThread
with the actual locking.

Isolates have been an afterthought and cleaning all this mess up is a long,
ongoing task, where this current CL is a tiny step...

[jochen (gone - plz use gerrit), 2013-01-18 15:22:45]

Ok, I'll wait for Eric's review, he has better insight into the proxy stuff

[eroman, 2013-01-18 19:21:14]

High level comments:

  * Rather than plumb a new v8::Isolate* dependency all the way from IOThread,
please internalize this as a singleton in the net/* code. My preference would be
to hide it in proxy_resolver_v8.cc so it doesn't need to be passed in as a
dependency to the constructor. Or alternately as a global in
proxy_service_v8.cc.

  * Note that I plan to remove the use of v8::Locker/Unlocker and switch to
using separate Isolates for each instance of ProxyResolverV8
(http://crbug.com/77847). This is one of the reasons why I would prefer not to
change the API to expose the Isolate dependency.

Background:

Today, ProxyResolverV8 is run by MultiThreadedProxyResolver, which causes
ProxyResolverV8 to get run concurrently on 4 different PAC threads (and
unfortunately this gets duplicated per profile and request contexts leading to
even more PAC threads). Because of this duplication, I didn't want to switch to
using separate isolates just yet for fear of the memory impact.

Finally as of this week though, I have a change to use a single-threaded
implementation instead (https://codereview.chromium.org/11885009/). Once I get
that change landed, I will be reworking things so there is at most 1 PAC thread
per Chrome, and each instance of ProxyResolverV8 will use its own Isolate.

https://codereview.chromium.org/11959029/diff/13014/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/11959029/diff/13014/net/net.gyp#newcode1677
net/net.gyp:1677: '../v8/tools/gyp/v8.gyp:v8',
why is this necessary? doesn't it already pick this up transitively from the
'net_with_v8' dependency?

https://codereview.chromium.org/11959029/diff/13014/net/net.gyp#newcode1819
net/net.gyp:1819: '../v8/tools/gyp/v8.gyp:v8',
See question above.

https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.cc:338: explicit Context(ProxyResolverJSBindings*
js_bindings,
nit: no more need for constructor to be marked explicit.

https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.h (right):

https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.h:24: // !!! Important note on threading model:
Please update this comment.

[Sven Panne, 2013-01-21 08:49:34]

On 2013/01/18 19:21:14, eroman wrote:
> High level comments:
> 
>   * Rather than plumb a new v8::Isolate* dependency all the way from IOThread,
> please internalize this as a singleton in the net/* code. My preference would
be
> to hide it in proxy_resolver_v8.cc so it doesn't need to be passed in as a
> dependency to the constructor. Or alternately as a global in
> proxy_service_v8.cc.

I am not sure if this is possible: The current proxy resolver always uses v8's
default isolate, which is created via an evil static initializer in v8 itself.
This means that you can't retrieve it in another static initializer, and
furthermore, the retrieval via Isolate::GetCurrent() has to be done on the main
thread (there is simply no "current isolate" on another thread). As far as I can
see it, there has to be some plumbing. What exactly is your proposal (in terms
of code)?

>   * Note that I plan to remove the use of v8::Locker/Unlocker and switch to
> using separate Isolates for each instance of ProxyResolverV8
> (http://crbug.com/77847). This is one of the reasons why I would prefer not to
> change the API to expose the Isolate dependency.
> 
> Background:
> 
> Today, ProxyResolverV8 is run by MultiThreadedProxyResolver, which causes
> ProxyResolverV8 to get run concurrently on 4 different PAC threads (and
> unfortunately this gets duplicated per profile and request contexts leading to
> even more PAC threads). Because of this duplication, I didn't want to switch
to
> using separate isolates just yet for fear of the memory impact.
> 
> Finally as of this week though, I have a change to use a single-threaded
> implementation instead (https://codereview.chromium.org/11885009/). Once I get
> that change landed, I will be reworking things so there is at most 1 PAC
thread
> per Chrome, and each instance of ProxyResolverV8 will use its own Isolate.

Hmmm, I have to admit that I don't fully understand your proposal: When exactly
will you create/destroy Isolates and how many of them will be alive at any
point? Could you please elaborate on this a bit?

Note that creating/destroying Isolates has a rather big performace impact. The
minimum code is basically as below:

    v8::Isolate* isolate = v8::Isolate::New();
    {
      v8::Isolate::Scope isolateScope(isolate);
      v8::Persistent<v8::Context> context = v8::Context::New();
      v8::Context::Scope contextScope(context);
      v8::HandleScope handleScope;
      [Do all your interesting stuff here]
      context.Dispose(isolate);
    }
    isolate->Dispose();

This code sequence takes 0.9ms on a fat HP620 workstation, I don't have numbers
for e.g. mobile phones with a much less powerful HW and where our snapshots
might even be compressed, so there might be more work to do (forgot the detailed
plans regarding this). The memory footprint is rather high, too: On ia32 it uses
2.2MB and an additional 16.3MB of reserved memory, on x64 it uses 2.9MB +
544MB(!) reserved memory, nothing of it shared.

In a nutshell: Isolates are not designed for rapid construction/destruction or
small memory footprint, which should not come as a surprise, they are
full-fledged, separate VM instances.

[Sven Panne, 2013-01-21 08:50:06]

https://codereview.chromium.org/11959029/diff/13014/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/11959029/diff/13014/net/net.gyp#newcode1677
net/net.gyp:1677: '../v8/tools/gyp/v8.gyp:v8',
On 2013/01/18 19:21:14, eroman wrote:
> why is this necessary? doesn't it already pick this up transitively from the
> 'net_with_v8' dependency?

Without this, 2 builder failed, see the try jobs for patch set 7:

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...
http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/77...

I don't have a GYP black belt, so I relied on Jochen who proposed these changes.
If there is a nicer way to achieve this, I'd be happy to learn it.

https://codereview.chromium.org/11959029/diff/13014/net/net.gyp#newcode1819
net/net.gyp:1819: '../v8/tools/gyp/v8.gyp:v8',
On 2013/01/18 19:21:14, eroman wrote:
> See question above.

See comment above. :)

https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.cc:338: explicit Context(ProxyResolverJSBindings*
js_bindings,
On 2013/01/18 19:21:14, eroman wrote:
> nit: no more need for constructor to be marked explicit.

Done.

https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.h (right):

https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.h:24: // !!! Important note on threading model:
On 2013/01/18 19:21:14, eroman wrote:
> Please update this comment.

This comment is still OK. The v8_default_isolate below *is* the only instance of
V8 the comment is talking about. Do you think that this should be added or what
else is missing/wrong?

[eroman, 2013-01-22 09:16:45]

What is special about the "main thread", why does the default isolate need
to come from there? Since this is the browser process, I wouldn't expect
there to be any initialization of V8 happening except for proxy resolving,
so can it not be done on a different thread?

Regarding the other question, initialization of isolates taking 1ms is not
a problem, the isolates I speak of would only be created a handful of times
during the runtime of the browser.


On Mon, Jan 21, 2013 at 12:50 AM, <svenpanne@chromium.org> wrote:

>
>
https://codereview.chromium.**org/11959029/diff/13014/net/**net.gyp<https://c...
> File net/net.gyp (right):
>
> https://codereview.chromium.**org/11959029/diff/13014/net/**
>
net.gyp#newcode1677<https://codereview.chromium.org/11959029/diff/13014/net/net.gyp#newcode1677>
> net/net.gyp:1677: '../v8/tools/gyp/v8.gyp:v8',
> On 2013/01/18 19:21:14, eroman wrote:
>
>> why is this necessary? doesn't it already pick this up transitively
>>
> from the
>
>> 'net_with_v8' dependency?
>>
>
> Without this, 2 builder failed, see the try jobs for patch set 7:
>
> http://build.chromium.org/p/**tryserver.chromium/builders/**
>
linux_chromeos/builds/75540/**steps/compile/logs/stdio<http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds/75540/steps/compile/logs/stdio>
> http://build.chromium.org/p/**tryserver.chromium/builders/**
>
linux_clang/builds/77053/**steps/compile/logs/stdio<http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/77053/steps/compile/logs/stdio>
>
> I don't have a GYP black belt, so I relied on Jochen who proposed these
> changes. If there is a nicer way to achieve this, I'd be happy to learn
> it.
>
>
> https://codereview.chromium.**org/11959029/diff/13014/net/**
>
net.gyp#newcode1819<https://codereview.chromium.org/11959029/diff/13014/net/net.gyp#newcode1819>
> net/net.gyp:1819: '../v8/tools/gyp/v8.gyp:v8',
> On 2013/01/18 19:21:14, eroman wrote:
>
>> See question above.
>>
>
> See comment above. :)
>
>
> https://codereview.chromium.**org/11959029/diff/13014/net/**
>
proxy/proxy_resolver_v8.cc<https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_v8.cc>
> File net/proxy/proxy_resolver_v8.cc (right):
>
> https://codereview.chromium.**org/11959029/diff/13014/net/**
>
proxy/proxy_resolver_v8.cc#**newcode338<https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_v8.cc#newcode338>
> net/proxy/proxy_resolver_v8.**cc:338: explicit
> Context(**ProxyResolverJSBindings* js_bindings,
> On 2013/01/18 19:21:14, eroman wrote:
>
>> nit: no more need for constructor to be marked explicit.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/11959029/diff/13014/net/**
>
proxy/proxy_resolver_v8.h<https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_v8.h>
> File net/proxy/proxy_resolver_v8.h (right):
>
> https://codereview.chromium.**org/11959029/diff/13014/net/**
>
proxy/proxy_resolver_v8.h#**newcode24<https://codereview.chromium.org/11959029/diff/13014/net/proxy/proxy_resolver_v8.h#newcode24>
> net/proxy/proxy_resolver_v8.h:**24: // !!! Important note on threading
> model:
> On 2013/01/18 19:21:14, eroman wrote:
>
>> Please update this comment.
>>
>
> This comment is still OK. The v8_default_isolate below *is* the only
> instance of V8 the comment is talking about. Do you think that this
> should be added or what else is missing/wrong?
>
>
https://codereview.chromium.**org/11959029/<https://codereview.chromium.org/1...
>

[Sven Panne, 2013-01-22 09:51:37]

On 2013/01/22 09:16:45, eroman wrote:
> What is special about the "main thread", why does the default isolate need
> to come from there? Since this is the browser process, I wouldn't expect
> there to be any initialization of V8 happening except for proxy resolving,
> so can it not be done on a different thread?

V8 has the notion of "entered isolate" (see v8::Isolate::Enter() in v8.h),
i.e. every thread has thread-local storage to remember which Isolate is
"current", and it is exactly this one which is returned via
v8::Isolate::GetCurrent(). The bookkeeping is done via v8::Isolate::Enter()
and Exit() (or the scoped variant v8::Isolate::Scope).

V8's default isolate is special in the sense that it is created and entered
automatically via a static initializer, even before main(), and this static
initialization is done on the main thread. Unless you have other means of
retrieving this Isolate (GetIsolate() in v8::Context, v8::Arguments or
v8::AccessorInfo), you *have* to retrieve it via Isolate::GetCurrent() on
the main thread, on other threads you would get NULL. The only other option
would be to explicitly Enter() the default Isolate in another thread, but I
think this would involve plumbing, too.

Remember: An Isolate is basically a whole VM, so you probably don't want to
waste the default Isolate, which always exists, anyway.

> Regarding the other question, initialization of isolates taking 1ms is not
> a problem, the isolates I speak of would only be created a handful of times
> during the runtime of the browser.

Android phones are at least a factor of 10 slower than our workstations,
cheaper ones even more. Furthermore, they are sometimes very limited
regarding resources, so I would suggest that you check with the
Android/clank people if this is OK.

How shall we proceed? I would really like to deprecate (i.e. via compiler
deprecations) the non-Isolate versions of Locker/Unlocker soon (i.e. in the
next few days). I understand your concerns regarding the internal API
changes in net/, so I'd like to hear a concrete proposal what we can do.

[eroman, 2013-01-22 18:37:02]

What about retrieving the default isolate with
Isolate::GetDefaultIsolateForLocking() and passing that to the locker. Would
that address your refactoring need?

Interesting, so the default isolate gets created via static initializer rather
than lazyily created. So then that means Chrome browser's startup time and
memory cost are getting inflated today by virtue of linking in V8, even for
cases where the network stack does not end up needing it. I'll file a bug for
that too, is there a way to defer creation of the default isolate today?

Lastly, note the dependencies on V8 from inside of io_thread.cc will cause
compile issues -- platforms like Chrome on iOS are not compiled with Chrome. I
suspect that is the issue you were running into when adding the extra V8
dependency to the GYP file.

[Sven Panne, 2013-01-23 09:10:09]

On 2013/01/22 18:37:02, eroman wrote:
> What about retrieving the default isolate with
> Isolate::GetDefaultIsolateForLocking() and passing that to the locker. Would
> that address your refactoring need?

We had discussed such an approach internally before, and we ruled it out for
several reasons: We would have to add a new external API entry like
"v8::Isolate::GetDefault()". This would be a step in the exactly wrong
direction, because we want to remove the notion of an implicit default
Isolate. From a practical point of view we would have to deprecate the new
API entry immediately, so we would be exactly where we started. :-) And
finally from an API design point of view: It is the embedder's task to
explicitly provide the right Isolate in API call, and not V8's task to
provide storage for that.
 
> Interesting, so the default isolate gets created via static initializer rather
> than lazyily created. So then that means Chrome browser's startup time and
> memory cost are getting inflated today by virtue of linking in V8, even for
> cases where the network stack does not end up needing it. I'll file a bug for
> that too, is there a way to defer creation of the default isolate today?

Alas, no. You can't defer the creation. And believe me: This early creation
causes us endless pain internally, requiring us e.g. to do an ugly 2-phase
initialization of several data structures etc.  Isolates have really been an
afterthought in V8, and the APIs around them were not designed, they just
somehow "happened". :-P Cleaning all this mess up is exactly the reason for
this CL.

> Lastly, note the dependencies on V8 from inside of io_thread.cc will cause
> compile issues -- platforms like Chrome on iOS are not compiled with Chrome. I
> suspect that is the issue you were running into when adding the extra V8
> dependency to the GYP file.

OK, I probably don't know enough about Chrome and how its various parts are
used in other projects, so I propose the following: I'll submit a
(hopefully) non-controversial tiny subset of my CL, and you take care of the
rest of the deprecated calls? This way we could move on much faster, and I
would be happy to help if some V8 issues come up.

[eroman, 2013-01-24 01:24:08]

Thanks for the explanations Sven!

I am definitely in favor of the deprecation work for Isolates you are doing and
don't mean to stall you in this review. I just want to better understand how it
all fits together so we can make the best choices in the Chrome code!

Ok, so given our last exchange, I propose hiding this in a global with something
like this:

(1) In io_thread.cc call (the code which is actually running on the main
thread):

#ifdef XXXXX
net::ProxyResolverV8::SetDefaultIsolate(v8::Isolate::GetCurrent());
#endif

(2)

in proxy_resolver_v8.cc

//static
void ProxyResolverV8::SetDefaultIsolate(v8::Isolate* isolate) {
  CHECK(isolate) << "Must call v8::Isolate::GetCurrent() fro main thread";
  g_default_isolate_ = isolate;
}

// static
void ProxyResolverV8::GetDefaultIsolate() {
  v8::Isolate* isolate = g_default_isolate_;
  if (!isolate)
    isolate = v8::Isolate::GetCurrent();
  CHECK(isolate) << "Must first call ProxyResolverV8::SetDefaultIsolate()";
  return isolate;
}

This will only leak the detail in one place (io_thread.cc), and once the default
isolate concept is gone, the isolate initialization can be moved explicitly into
ProxyResolverV8.

What do you think?

[Sven Panne, 2013-01-24 13:11:08]

OK, your proposal makes sense is much less intrusive than mine. The only
question I have is: What is the XXXXX in '#ifdef XXXXX'? Why do we need
conditional compilation there at all?

[eroman, 2013-01-24 17:41:58]

I believe it would be:

#if !defined(OS_IOS)

Since on that platform it doesn't compile chrome with V8.


On Thu, Jan 24, 2013 at 5:11 AM, <svenpanne@chromium.org> wrote:

> OK, your proposal makes sense is much less intrusive than mine. The only
> question I have is: What is the XXXXX in '#ifdef XXXXX'? Why do we need
> conditional compilation there at all?
>
>
https://codereview.chromium.**org/11959029/<https://codereview.chromium.org/1...
>

[Sven Panne, 2013-01-31 11:10:58]

Here is my latest version, basically Eric's proposal with a slight
simplification. PTAL...

[eroman, 2013-01-31 20:11:59]

LGTM, thanks!

[commit-bot: I haz the power, 2013-01-31 20:50:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/svenpanne@chromium.org/11959029/38006

[commit-bot: I haz the power, 2013-01-31 20:50:59]

Failed to apply patch for net/proxy/proxy_resolver_v8.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file net/proxy/proxy_resolver_v8.cc
  Hunk #2 FAILED at 332.
  Hunk #3 succeeded at 348 (offset -6 lines).
  Hunk #4 succeeded at 399 (offset -6 lines).
  Hunk #5 succeeded at 478 (offset -6 lines).
  Hunk #6 succeeded at 668 (offset -6 lines).
  Hunk #7 succeeded at 729 (offset -6 lines).
  1 out of 7 hunks FAILED -- saving rejects to file
net/proxy/proxy_resolver_v8.cc.rej

Patch:       net/proxy/proxy_resolver_v8.cc
Index: net/proxy/proxy_resolver_v8.cc
diff --git a/net/proxy/proxy_resolver_v8.cc b/net/proxy/proxy_resolver_v8.cc
index
8f71af94be78524635edcda9efebac2ef4ee3b4b..1acc873007840d2e9b7e9d5df3149122201a5f0d
100644
--- a/net/proxy/proxy_resolver_v8.cc
+++ b/net/proxy/proxy_resolver_v8.cc
@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#define V8_DISABLE_DEPRECATIONS 1
-
 #include "net/proxy/proxy_resolver_v8.h"
 
 #include <algorithm>
@@ -334,15 +332,17 @@ bool IsInNetEx(const std::string& ip_address, const
std::string& ip_prefix) {
 
 class ProxyResolverV8::Context {
  public:
-  explicit Context(ProxyResolverV8* parent)
-      : parent_(parent) {
+  Context(ProxyResolverV8* parent, v8::Isolate* isolate)
+      : parent_(parent),
+        isolate_(isolate) {
+    DCHECK(isolate);
   }
 
   ~Context() {
-    v8::Locker locked;
+    v8::Locker locked(isolate_);
 
-    v8_this_.Dispose();
-    v8_context_.Dispose();
+    v8_this_.Dispose(isolate_);
+    v8_context_.Dispose(isolate_);
 
     // Run the V8 garbage collector. We do this to be sure the
     // ExternalStringResource objects we allocated get properly disposed.
@@ -356,7 +356,7 @@ class ProxyResolverV8::Context {
   }
 
   int ResolveProxy(const GURL& query_url, ProxyInfo* results) {
-    v8::Locker locked;
+    v8::Locker locked(isolate_);
     v8::HandleScope scope;
 
     v8::Context::Scope function_scope(v8_context_);
@@ -407,10 +407,11 @@ class ProxyResolverV8::Context {
   }
 
   int InitV8(const scoped_refptr<ProxyResolverScriptData>& pac_script) {
-    v8::Locker locked;
+    v8::Locker locked(isolate_);
     v8::HandleScope scope;
 
-    v8_this_ = v8::Persistent<v8::External>::New(v8::External::New(this));
+    v8_this_ = v8::Persistent<v8::External>::New(isolate_,
+                                                 v8::External::New(this));
     v8::Local<v8::ObjectTemplate> global_template = v8::ObjectTemplate::New();
 
     // Attach the javascript bindings.
@@ -485,7 +486,7 @@ class ProxyResolverV8::Context {
   }
 
   void PurgeMemory() {
-    v8::Locker locked;
+    v8::Locker locked(isolate_);
     v8::V8::LowMemoryNotification();
   }
 
@@ -675,6 +676,7 @@ class ProxyResolverV8::Context {
 
   mutable base::Lock lock_;
   ProxyResolverV8* parent_;
+  v8::Isolate* isolate_;
   v8::Persistent<v8::External> v8_this_;
   v8::Persistent<v8::Context> v8_context_;
 };
@@ -735,11 +737,30 @@ int ProxyResolverV8::SetPacScript(
     return ERR_PAC_SCRIPT_FAILED;
 
   // Try parsing the PAC script.
-  scoped_ptr<Context> context(new Context(this));
+  scoped_ptr<Context> context(new Context(this, GetDefaultIsolate()));
   int rv = context->InitV8(script_data);
   if (rv == OK)
     context_.reset(context.release());
   return rv;
 }
 
+// static
+void ProxyResolverV8::RememberDefaultIsolate() {
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  DCHECK(isolate)
+      << "ProxyResolverV8::RememberDefaultIsolate called on wrong thread";
+  DCHECK(g_default_isolate_ == NULL || g_default_isolate_ == isolate)
+      << "Default Isolate can not be changed";
+  g_default_isolate_ = isolate;
+}
+
+// static
+v8::Isolate* ProxyResolverV8::GetDefaultIsolate() {
+  DCHECK(g_default_isolate_)
+      << "Must call ProxyResolverV8::RememberDefaultIsolate() first";
+  return g_default_isolate_;
+}
+
+v8::Isolate* ProxyResolverV8::g_default_isolate_ = NULL;
+
 }  // namespace net

[commit-bot: I haz the power, 2013-02-01 08:18:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/svenpanne@chromium.org/11959029/36002

[commit-bot: I haz the power, 2013-02-01 08:18:51]

Presubmit check for 11959029-36002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.4s to calculate.

[jochen (gone - plz use gerrit), 2013-02-01 08:21:08]

chrome/ lgtm

[commit-bot: I haz the power, 2013-02-01 08:21:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/svenpanne@chromium.org/11959029/36002

[jochen (gone - plz use gerrit), 2013-02-01 12:28:54]

Committed manually as r180134 (presubmit successful).

[John Knottenbelt, 2013-02-01 15:39:33]

https://codereview.chromium.org/11959029/diff/36002/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11959029/diff/36002/chrome/browser/io_thread....
chrome/browser/io_thread.cc:377: net::ProxyResolverV8::RememberDefaultIsolate();
On downstream Chrome on Android, this function asserted because V8 hasn't been
initialised yet. Perhaps we are missing some initialisation downstream?

Where does the initialisation happen upstream?

[Sven Panne, 2013-02-01 15:52:28]

https://codereview.chromium.org/11959029/diff/36002/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11959029/diff/36002/chrome/browser/io_thread....
chrome/browser/io_thread.cc:377: net::ProxyResolverV8::RememberDefaultIsolate();
RememberDefaultIsolate() has to be called exactly once from the main thread
(i.e. the 'int main(argc, argv)' one in C-speak) before any calls to the v8
proxy resolver. Do *not* call RememberDefaultIsolate() in some kind of static
global initializer, because this would result in undefined behaviour (because v8
uses a such an initializer itself :-P ).

[digit1, 2013-02-01 16:47:09]

After analysis, we have a better understanding of the issue:

V8 initialization uses a static initializer to call its internal
EnsureDefaultIsolate() function (see line 383 in v8/src/isolate.cc).

This initializer code is run at load-time, which happens on Android in a
*temporary* thread that disappears at the end of the load (this is done to load
the library in parallel to a lot of Java initialization stuff).

The v8 code in EnsureDefaultIsolate() allocates a new Isolate, store its address
in an internal global variables, and also stores its in a TLS slot. But this
slot _disappears_ when the background loading threads stops.

The implementation of v8::Isolate::GetCurrent() only looks at the TLS slot,
which will always be empty when the main or I/O thread calls it.

In other words, v8::Isolate::GetCurrent() cannot be used to retrieve the default
Isolate on Android. Moreoever, there doesn't seem to be any v8 API to retrieve
the internal default_isolate_ value either.

I'd be tempted to revert this patch to fix the issue. Besides, I find it a bit
suspicious that PAC script would be executed in the same isolate than other
parts of Chromium, that sounds like a potential security problem (which may have
existed before though).

Still, we're still looking at the v8 sources to see if there is a simple
solution.

[Sven Panne, 2013-02-01 18:14:09]

As Eric already pointed out in another issue, there is no security issue,
because the isolate created via the initializer code is only used for the single
purpose of resolving proxies. Furthermore, there is currently no easy way to
avoid creating this initial isolate, nor is there (or even should be!) a way to
retrieve it. The goal of all this (and probably quite a few upcoming CLs) is to
remove all this implicit stuff and make everything explicit. The really tricky
part, as we currently all see ;-), is how to get there without breaking things
in the meantime or allocating unused isolates.

Is there an Android-specific way of calling RememberDefaultIsolate from that
temporary thread *after* all initializer code is guaranteed to have finished?
Doing it there and disabling the call in the IOThread constructor on Android
seems to be the right way for now. All this craziness will hopefully vanish when
everything is explicit... :-/ And last but not least: Our current DOM
performance will be much better without all the TLS accesses, too.