Close leaking FDs.

Close leaking FDs.

When starting a SandboxedProcess the resource file descriptors were leaked in
the browser process, eventually causing crashers.

BUG=None
TEST=When using the Android content shell for a long time, it should not crash.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158456

[Jay Civelli, 2012-09-19 22:58:57]

https://codereview.chromium.org/10949027/diff/1/content/browser/child_process...
File content/browser/child_process_launcher.cc (left):

https://codereview.chromium.org/10949027/diff/1/content/browser/child_process...
content/browser/child_process_launcher.cc:171: kPrimaryIPCChannel, ipcfd));
Not sure why this was done.
The IPC FD is special and not passes as a file to register.

[Yaron, 2012-09-20 00:01:54]

https://codereview.chromium.org/10949027/diff/1/content/browser/child_process...
File content/browser/child_process_launcher.cc (left):

https://codereview.chromium.org/10949027/diff/1/content/browser/child_process...
content/browser/child_process_launcher.cc:171: kPrimaryIPCChannel, ipcfd));
On 2012/09/19 22:58:57, Jay Civelli wrote:
> Not sure why this was done.
> The IPC FD is special and not passes as a file to register.

Are you sure this is right? I thought we've always had this and other ports do
it as well. The same fd is also set to ipcfd_ and closed in |Notify|. Oh but
it's passed separately to |StartSandboxedProcess|?

[jam, 2012-09-20 06:50:58]

can you send this to an owner in content who's more familiar with posix? i.e.
piman

[piman, 2012-09-20 07:03:03]

https://codereview.chromium.org/10949027/diff/1/chrome/browser/chrome_content...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/10949027/diff/1/chrome/browser/chrome_content...
chrome/browser/chrome_content_browser_client.cc:1673: kCrashDumpSignal,
dup(crash_signal_fd)));
I don't get it. You're changing the semantics to add a dup here, giving out
ownership on all non-mac posix, but you're only changing android code
accordingly.
Won't that cause new fd leaks on linux?

[Jay Civelli, 2012-09-20 17:59:26]

On 2012/09/20 07:03:03, piman wrote:
>
https://codereview.chromium.org/10949027/diff/1/chrome/browser/chrome_content...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/10949027/diff/1/chrome/browser/chrome_content...
> chrome/browser/chrome_content_browser_client.cc:1673: kCrashDumpSignal,
> dup(crash_signal_fd)));
> I don't get it. You're changing the semantics to add a dup here, giving out
> ownership on all non-mac posix, but you're only changing android code
> accordingly.
> Won't that cause new fd leaks on linux?
Doh! You are right. I was wrongly thinking it was Android only for some reason
(even though there is an ifdef posix right above... I know).
I think it would be hard to keep the
ContentBrowserClient::GetAdditionalMappedFilesForChildProcess API as is and have
the calling code ifdef to realize if it needs to close or not the fd. Especially
since per Yaron suggestion, we could use this on Android to pass the IPC FD
(which should not be closed) along with other FDs that need to be closed.
So I am proposing to change the API so the GlobalDescriptor::Mapping has a new
boolean field describing whether "ownership" of the FD is passed to the caller.
What do you think?

[piman, 2012-09-20 18:07:25]

On Thu, Sep 20, 2012 at 10:59 AM, <jcivelli@chromium.org> wrote:

> On 2012/09/20 07:03:03, piman wrote:
>
> https://codereview.chromium.**org/10949027/diff/1/chrome/**
>
browser/chrome_content_**browser_client.cc<https://codereview.chromium.org/10949027/diff/1/chrome/browser/chrome_content_browser_client.cc>
>
>> File chrome/browser/chrome_content_**browser_client.cc (right):
>>
>
>
> https://codereview.chromium.**org/10949027/diff/1/chrome/**
>
browser/chrome_content_**browser_client.cc#newcode1673<https://codereview.chromium.org/10949027/diff/1/chrome/browser/chrome_content_browser_client.cc#newcode1673>
>
>> chrome/browser/chrome_content_**browser_client.cc:1673: kCrashDumpSignal,
>> dup(crash_signal_fd)));
>> I don't get it. You're changing the semantics to add a dup here, giving
>> out
>> ownership on all non-mac posix, but you're only changing android code
>> accordingly.
>> Won't that cause new fd leaks on linux?
>>
> Doh! You are right. I was wrongly thinking it was Android only for some
> reason
> (even though there is an ifdef posix right above... I know).
> I think it would be hard to keep the
> ContentBrowserClient::**GetAdditionalMappedFilesForChi**ldProcess API as
> is and have
> the calling code ifdef to realize if it needs to close or not the fd.
> Especially
> since per Yaron suggestion, we could use this on Android to pass the IPC FD
> (which should not be closed) along with other FDs that need to be closed.
> So I am proposing to change the API so the GlobalDescriptor::Mapping has a
> new
> boolean field describing whether "ownership" of the FD is passed to the
> caller.
> What do you think?
>

Use FileDescriptor and its auto_close flag? Note, auto_close isn't magic,
just understood by the IPC system when put into message, but I think it's
acceptable to overload the flag here I think.
Also note, I'm not really intimate with the code, you should probably make
sure to include a reviewer who is more knowledgeable about GlobalDescriptor.


>
>
https://codereview.chromium.**org/10949027/<https://codereview.chromium.org/1...
>

[Jay Civelli, 2012-09-21 01:42:01]

After talking with jam, I changed the ContentBrowserClient method so it uses a
FileDescriptor as suggested by piman.
Also I cleaned-up the android sandbox creation process as suggested by Yaron.

[jam, 2012-09-21 01:46:33]

new file lgtm, i defer to piman for the actual review

https://codereview.chromium.org/10949027/diff/6002/content/public/browser/fil...
File content/public/browser/file_descriptor_info.h (right):

https://codereview.chromium.org/10949027/diff/6002/content/public/browser/fil...
content/public/browser/file_descriptor_info.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: add this file to the gyp so that it shows up in IDEs

[piman, 2012-09-21 02:31:02]

https://codereview.chromium.org/10949027/diff/6002/content/browser/child_proc...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/10949027/diff/6002/content/browser/child_proc...
content/browser/child_process_launcher.cc:194:
GetAdditionalMappedFilesForChildProcess(*cmd_line, &files_to_register);
Shouldn't this call site here change too?

[Jay Civelli, 2012-09-21 22:12:06]

Adding Markus for the zygote_host part.

Thanks.

http://codereview.chromium.org/10949027/diff/6002/content/browser/child_proce...
File content/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/10949027/diff/6002/content/browser/child_proce...
content/browser/child_process_launcher.cc:194:
GetAdditionalMappedFilesForChildProcess(*cmd_line, &files_to_register);
On 2012/09/21 02:31:02, piman wrote:
> Shouldn't this call site here change too?
Sorry, in my haste I had forgotten that.

http://codereview.chromium.org/10949027/diff/6002/content/public/browser/file...
File content/public/browser/file_descriptor_info.h (right):

http://codereview.chromium.org/10949027/diff/6002/content/public/browser/file...
content/public/browser/file_descriptor_info.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/09/21 01:46:34, John Abd-El-Malek wrote:
> nit: add this file to the gyp so that it shows up in IDEs

Done.

[piman, 2012-09-21 22:30:48]

http://codereview.chromium.org/10949027/diff/10015/content/browser/child_proc...
File content/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/10949027/diff/10015/content/browser/child_proc...
content/browser/child_process_launcher.cc:199: } else
So, in the zygote case, we'll close the FD in ForkRequest.
What closes it in the other case?

[Jay Civelli, 2012-09-21 22:48:34]

https://codereview.chromium.org/10949027/diff/10015/content/browser/child_pro...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/10949027/diff/10015/content/browser/child_pro...
content/browser/child_process_launcher.cc:199: } else
On 2012/09/21 22:30:48, piman wrote:
> So, in the zygote case, we'll close the FD in ForkRequest.
> What closes it in the other case?
Good point, reverted to using the ScopedFD.

[piman, 2012-09-22 01:07:27]

lgtm

[Yaron, 2012-09-24 18:53:14]

+acleung as FYI as this conflicts with http://codereview.chromium.org/10876056/

https://codereview.chromium.org/10949027/diff/7007/content/browser/android/sa...
File content/browser/android/sandboxed_process_launcher.cc (right):

https://codereview.chromium.org/10949027/diff/7007/content/browser/android/sa...
content/browser/android/sandboxed_process_launcher.cc:47: size_t file_count =
files_to_register.size();
assert > 0 ?

https://codereview.chromium.org/10949027/diff/7007/content/browser/android/sa...
content/browser/android/sandboxed_process_launcher.cc:57:
base::android::CheckException(env);
You should probably CheckException for the GetIntArrayElement calls too, right?
Those aren't our wrappers and are straight JNI calls.

https://codereview.chromium.org/10949027/diff/7007/content/browser/android/sa...
content/browser/android/sandboxed_process_launcher.cc:64:
env->ReleaseIntArrayElements(j_file_ids.obj(), file_ids, JNI_ABORT);
So we were previously leaking these local references too?

https://codereview.chromium.org/10949027/diff/7007/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/FileDescriptorInfo.java
(right):

https://codereview.chromium.org/10949027/diff/7007/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/FileDescriptorInfo.java:13:
public boolean mAutoclose;
s/mAutoclose/mAutoClose/

https://codereview.chromium.org/10949027/diff/7007/content/shell/shell_conten...
File content/shell/shell_content_browser_client.cc (right):

https://codereview.chromium.org/10949027/diff/7007/content/shell/shell_conten...
content/shell/shell_content_browser_client.cc:92: mappings->push_back(
You'll have to make a similar change in
chrome/browser/chrome_content_browser_client.cc when downstreaming, right?

[Jay Civelli, 2012-09-24 20:59:59]

https://codereview.chromium.org/10949027/diff/7007/content/browser/android/sa...
File content/browser/android/sandboxed_process_launcher.cc (right):

https://codereview.chromium.org/10949027/diff/7007/content/browser/android/sa...
content/browser/android/sandboxed_process_launcher.cc:47: size_t file_count =
files_to_register.size();
On 2012/09/24 18:53:14, Yaron wrote:
> assert > 0 ?

Done.

https://codereview.chromium.org/10949027/diff/7007/content/browser/android/sa...
content/browser/android/sandboxed_process_launcher.cc:57:
base::android::CheckException(env);
On 2012/09/24 18:53:14, Yaron wrote:
> You should probably CheckException for the GetIntArrayElement calls too,
right?
> Those aren't our wrappers and are straight JNI calls.

Good idea, also added some more after allocating.

https://codereview.chromium.org/10949027/diff/7007/content/browser/android/sa...
content/browser/android/sandboxed_process_launcher.cc:64:
env->ReleaseIntArrayElements(j_file_ids.obj(), file_ids, JNI_ABORT);
On 2012/09/24 18:53:14, Yaron wrote:
> So we were previously leaking these local references too?
No, we were using the JNI SetIntArrayRegion method, that does not require any
releasing.
We now use GetIntArrayElements, which does require a release.

https://codereview.chromium.org/10949027/diff/7007/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/FileDescriptorInfo.java
(right):

https://codereview.chromium.org/10949027/diff/7007/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/FileDescriptorInfo.java:13:
public boolean mAutoclose;
On 2012/09/24 18:53:14, Yaron wrote:
> s/mAutoclose/mAutoClose/
Done.
It's one of those for which I am not sure. For ex: infobar instead of infoBar,
checkbox instead of checkBox...

https://codereview.chromium.org/10949027/diff/7007/content/shell/shell_conten...
File content/shell/shell_content_browser_client.cc (right):

https://codereview.chromium.org/10949027/diff/7007/content/shell/shell_conten...
content/shell/shell_content_browser_client.cc:92: mappings->push_back(
On 2012/09/24 18:53:14, Yaron wrote:
> You'll have to make a similar change in
> chrome/browser/chrome_content_browser_client.cc when downstreaming, right?
Yes, I am planning on downstreaming myself and making it work.

[Markus (顧孟勤), 2012-09-24 21:17:34]

lgtm

LGTM (for zygote_host_impl.*)

http://codereview.chromium.org/10949027/diff/19001/content/browser/zygote_hos...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

http://codereview.chromium.org/10949027/diff/19001/content/browser/zygote_hos...
content/browser/zygote_host/zygote_host_impl_linux.cc:258: // link_ptr.
s/link/linked/

http://codereview.chromium.org/10949027/diff/19001/content/browser/zygote_hos...
content/browser/zygote_host/zygote_host_impl_linux.cc:267:
autodelete_fds.push_back(ptr);
I think I understand what you are doing here, but it took some very careful
reading of the code. It probably wouldn't hurt, if you added a comment
explaining what we are doing with "auto_close" file descriptors.

[Yaron, 2012-09-24 21:27:02]

lgtm

http://codereview.chromium.org/10949027/diff/19001/content/browser/android/sa...
File content/browser/android/sandboxed_process_launcher.cc (right):

http://codereview.chromium.org/10949027/diff/19001/content/browser/android/sa...
content/browser/android/sandboxed_process_launcher.cc:81:
base::android::CheckException(env);
As this is a jni-generator function, you dont' need to CheckException.

[commit-bot: I haz the power, 2012-09-24 22:01:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/10949027/30001

[commit-bot: I haz the power, 2012-09-25 00:20:14]

Change committed as 158456

[tonyg, 2012-09-28 16:22:16]

On 2012/09/25 00:20:14, I haz the power (commit-bot) wrote:
> Change committed as 158456

After this patch, every time I start the content shell I get the below crash
stack. Rolling back the patch fixes it.

E/AndroidRuntime( 2784): FATAL EXCEPTION: main
E/AndroidRuntime( 2784): java.lang.RuntimeException: Unknown binder error code.
0xfffffff7
E/AndroidRuntime( 2784): 	at android.os.Parcel.nativeWriteFileDescriptor(Native
Method)
E/AndroidRuntime( 2784): 	at
android.os.Parcel.writeFileDescriptor(Parcel.java:552)
E/AndroidRuntime( 2784): 	at
android.os.ParcelFileDescriptor.writeToParcel(ParcelFileDescriptor.java:412)
E/AndroidRuntime( 2784): 	at android.os.Parcel.writeParcelable(Parcel.java:1254)
E/AndroidRuntime( 2784): 	at android.os.Parcel.writeValue(Parcel.java:1173)
E/AndroidRuntime( 2784): 	at android.os.Parcel.writeMapInternal(Parcel.java:591)
E/AndroidRuntime( 2784): 	at android.os.Bundle.writeToParcel(Bundle.java:1619)
E/AndroidRuntime( 2784): 	at
org.chromium.content.common.ISandboxedProcessService$Stub$Proxy.setupConnection(ISandboxedProcessService.java:111)
E/AndroidRuntime( 2784): 	at
org.chromium.content.browser.SandboxedProcessConnection.doConnectionSetup(SandboxedProcessConnection.java:235)
E/AndroidRuntime( 2784): 	at
org.chromium.content.browser.SandboxedProcessConnection.onServiceConnected(SandboxedProcessConnection.java:183)
E/AndroidRuntime( 2784): 	at
android.app.LoadedApk$ServiceDispatcher.doConnected(LoadedApk.java:1097)
E/AndroidRuntime( 2784): 	at
android.app.LoadedApk$ServiceDispatcher$RunConnection.run(LoadedApk.java:1114)
E/AndroidRuntime( 2784): 	at android.os.Handler.handleCallback(Handler.java:615)
E/AndroidRuntime( 2784): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/AndroidRuntime( 2784): 	at android.os.Looper.loop(Looper.java:142)
E/AndroidRuntime( 2784): 	at
android.app.ActivityThread.main(ActivityThread.java:4573)
E/AndroidRuntime( 2784): 	at java.lang.reflect.Method.invokeNative(Native
Method)
E/AndroidRuntime( 2784): 	at java.lang.reflect.Method.invoke(Method.java:511)
E/AndroidRuntime( 2784): 	at
com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:786)
E/AndroidRuntime( 2784): 	at
com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
E/AndroidRuntime( 2784): 	at dalvik.system.NativeStart.main(Native Method)

[Jay Civelli, 2012-09-28 16:57:22]

Sorry for the breakage, known problem.
Fix coming very soon (later on today).

Jay


On Fri, Sep 28, 2012 at 9:22 AM, <tonyg@chromium.org> wrote:

> On 2012/09/25 00:20:14, I haz the power (commit-bot) wrote:
>
>> Change committed as 158456
>>
>
> After this patch, every time I start the content shell I get the below
> crash
> stack. Rolling back the patch fixes it.
>
> E/AndroidRuntime( 2784): FATAL EXCEPTION: main
> E/AndroidRuntime( 2784): java.lang.RuntimeException: Unknown binder error
> code.
> 0xfffffff7
> E/AndroidRuntime( 2784):        at android.os.Parcel.**
> nativeWriteFileDescriptor(**Native
> Method)
> E/AndroidRuntime( 2784):        at
> android.os.Parcel.**writeFileDescriptor(Parcel.**java:552)
> E/AndroidRuntime( 2784):        at
> android.os.**ParcelFileDescriptor.**writeToParcel(**
> ParcelFileDescriptor.java:412)
> E/AndroidRuntime( 2784):        at android.os.Parcel.**
> writeParcelable(Parcel.java:**1254)
> E/AndroidRuntime( 2784):        at android.os.Parcel.writeValue(**
> Parcel.java:1173)
> E/AndroidRuntime( 2784):        at android.os.Parcel.**
> writeMapInternal(Parcel.java:**591)
> E/AndroidRuntime( 2784):        at android.os.Bundle.**
> writeToParcel(Bundle.java:**1619)
> E/AndroidRuntime( 2784):        at
> org.chromium.content.common.**ISandboxedProcessService$Stub$**
> Proxy.setupConnection(**ISandboxedProcessService.java:**111)
> E/AndroidRuntime( 2784):        at
> org.chromium.content.browser.**SandboxedProcessConnection.**
> doConnectionSetup(**SandboxedProcessConnection.**java:235)
> E/AndroidRuntime( 2784):        at
> org.chromium.content.browser.**SandboxedProcessConnection.**
> onServiceConnected(**SandboxedProcessConnection.**java:183)
> E/AndroidRuntime( 2784):        at
> android.app.LoadedApk$**ServiceDispatcher.doConnected(**
> LoadedApk.java:1097)
> E/AndroidRuntime( 2784):        at
> android.app.LoadedApk$**ServiceDispatcher$**RunConnection.run(LoadedApk.**
> java:1114)
> E/AndroidRuntime( 2784):        at android.os.Handler.**
> handleCallback(Handler.java:**615)
> E/AndroidRuntime( 2784):        at android.os.Handler.**
> dispatchMessage(Handler.java:**92)
> E/AndroidRuntime( 2784):        at android.os.Looper.loop(Looper.**
> java:142)
> E/AndroidRuntime( 2784):        at
> android.app.ActivityThread.**main(ActivityThread.java:4573)
> E/AndroidRuntime( 2784):        at java.lang.reflect.Method.**
> invokeNative(Native
> Method)
> E/AndroidRuntime( 2784):        at java.lang.reflect.Method.**
> invoke(Method.java:511)
> E/AndroidRuntime( 2784):        at
> com.android.internal.os.**ZygoteInit$**MethodAndArgsCaller.run(**
> ZygoteInit.java:786)
> E/AndroidRuntime( 2784):        at
> com.android.internal.os.**ZygoteInit.main(ZygoteInit.**java:553)
> E/AndroidRuntime( 2784):        at dalvik.system.NativeStart.**main(Native
> Method)
>
>
>
>
https://chromiumcodereview.**appspot.com/10949027/<https://chromiumcodereview...
>

[Jay Civelli, 2012-09-28 16:59:56]

And BTW, this should work in Debug mode (the bug is caused by a DCHECK
containing actual code that ends up not being run).

Jay


On Fri, Sep 28, 2012 at 9:57 AM, Jay Civelli <jcivelli@chromium.org> wrote:

> Sorry for the breakage, known problem.
> Fix coming very soon (later on today).
>
> Jay
>
>
> On Fri, Sep 28, 2012 at 9:22 AM, <tonyg@chromium.org> wrote:
>
>> On 2012/09/25 00:20:14, I haz the power (commit-bot) wrote:
>>
>>> Change committed as 158456
>>>
>>
>> After this patch, every time I start the content shell I get the below
>> crash
>> stack. Rolling back the patch fixes it.
>>
>> E/AndroidRuntime( 2784): FATAL EXCEPTION: main
>> E/AndroidRuntime( 2784): java.lang.RuntimeException: Unknown binder error
>> code.
>> 0xfffffff7
>> E/AndroidRuntime( 2784):        at android.os.Parcel.**
>> nativeWriteFileDescriptor(**Native
>> Method)
>> E/AndroidRuntime( 2784):        at
>> android.os.Parcel.**writeFileDescriptor(Parcel.**java:552)
>> E/AndroidRuntime( 2784):        at
>> android.os.**ParcelFileDescriptor.**writeToParcel(**
>> ParcelFileDescriptor.java:412)
>> E/AndroidRuntime( 2784):        at android.os.Parcel.**
>> writeParcelable(Parcel.java:**1254)
>> E/AndroidRuntime( 2784):        at android.os.Parcel.writeValue(**
>> Parcel.java:1173)
>> E/AndroidRuntime( 2784):        at android.os.Parcel.**
>> writeMapInternal(Parcel.java:**591)
>> E/AndroidRuntime( 2784):        at android.os.Bundle.**
>> writeToParcel(Bundle.java:**1619)
>> E/AndroidRuntime( 2784):        at
>> org.chromium.content.common.**ISandboxedProcessService$Stub$**
>> Proxy.setupConnection(**ISandboxedProcessService.java:**111)
>> E/AndroidRuntime( 2784):        at
>> org.chromium.content.browser.**SandboxedProcessConnection.**
>> doConnectionSetup(**SandboxedProcessConnection.**java:235)
>> E/AndroidRuntime( 2784):        at
>> org.chromium.content.browser.**SandboxedProcessConnection.**
>> onServiceConnected(**SandboxedProcessConnection.**java:183)
>> E/AndroidRuntime( 2784):        at
>> android.app.LoadedApk$**ServiceDispatcher.doConnected(**
>> LoadedApk.java:1097)
>> E/AndroidRuntime( 2784):        at
>> android.app.LoadedApk$**ServiceDispatcher$**RunConnection.run(LoadedApk.*
>> *java:1114)
>> E/AndroidRuntime( 2784):        at android.os.Handler.**
>> handleCallback(Handler.java:**615)
>> E/AndroidRuntime( 2784):        at android.os.Handler.**
>> dispatchMessage(Handler.java:**92)
>> E/AndroidRuntime( 2784):        at android.os.Looper.loop(Looper.**
>> java:142)
>> E/AndroidRuntime( 2784):        at
>> android.app.ActivityThread.**main(ActivityThread.java:4573)
>> E/AndroidRuntime( 2784):        at java.lang.reflect.Method.**
>> invokeNative(Native
>> Method)
>> E/AndroidRuntime( 2784):        at java.lang.reflect.Method.**
>> invoke(Method.java:511)
>> E/AndroidRuntime( 2784):        at
>> com.android.internal.os.**ZygoteInit$**MethodAndArgsCaller.run(**
>> ZygoteInit.java:786)
>> E/AndroidRuntime( 2784):        at
>> com.android.internal.os.**ZygoteInit.main(ZygoteInit.**java:553)
>> E/AndroidRuntime( 2784):        at dalvik.system.NativeStart.**main(Native
>> Method)
>>
>>
>>
>>
https://chromiumcodereview.**appspot.com/10949027/<https://chromiumcodereview...
>>
>
>