Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2687713003:
Stop base::OpenFile from leaking fds/handles into child procs. (Closed)
|
Created:
3 years, 10 months ago by grt (UTC plus 2) Modified:
3 years, 10 months ago CC:
chromium-reviews, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStop base::OpenFile from leaking fds/handles into child procs.
BUG=688362
TEST=New FileUtilTest.OpenFileNoInheritance fails with previous impl, passes with new impl.
Review-Url: https://codereview.chromium.org/2687713003
Cr-Commit-Position: refs/heads/master@{#449999}
Committed: https://chromium.googlesource.com/chromium/src/+/d0bc44fa24fba9f483aaddae8e2c29df3524a5d6
Patch Set 1 #Patch Set 2 : macOS fix #
Total comments: 26
Patch Set 3 : suppress glibc leak #
Total comments: 7
Patch Set 4 : addressed review comments #
Total comments: 6
Patch Set 5 : more review comments #
Total comments: 2
Patch Set 6 : gab comments #
Messages
Total messages: 46 (32 generated)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Stop base::OpenFile from leaking fds/handles into child procs. BUG=688362 ========== to ========== Stop base::OpenFile from leaking fds/handles into child procs. BUG=688362 ==========
grt@chromium.org changed reviewers: + gab@chromium.org
grt@chromium.org changed reviewers: + mark@chromium.org
Hi Gab and Mark. This does more-or-less what was we were discussing. Any callers who wish can turn inheritance back on after the fact. Unfortunately, macOS and iOS don't seem[1] to have a way to set O_CLOEXEC (an option for open(2)) via a mode char for fopen(3), so we set the option immediately after opening the stream. There will be a race, but that's the best we can do short of parsing the mode, open(2)ing the file with the right options, and then fdopen(3)ing the fd to get a stream. [1] I don't have a Mac, so I can't really verify this. The darwin manpages that Google found for me didn't indicate that there's a way. Please let me know if you have better sources and know of a magic char that works there.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There is no magic character. Here’s the source for fopen() on Mac: https://opensource.apple.com/source/Libc/Libc-1158.30.7/stdio/FreeBSD/fopen.c... Flags are translated by __sflags() in https://opensource.apple.com/source/Libc/Libc-1158.30.7/stdio/FreeBSD/flags.c.... As you can see, it’s somewhat straightforward to construct fopen() out of fdopen(open()) (fdopen() source is https://opensource.apple.com/source/Libc/Libc-1158.30.7/stdio/FreeBSD/fdopen....), but I don’t know if that’s really worthwhile. I checked a few libc implementations used on Linux, including glibc, bionic, and musl, and all implement 'e'. I didn’t check how far back this extension was supported, but the glibc man page says 2.7, which is vintage 2007. https://codereview.chromium.org/2687713003/diff/80001/base/files/file_util_po... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2687713003/diff/80001/base/files/file_util_po... base/files/file_util_posix.cc:189: // non-Mac platforms (which do not support this); see Why even bother defining this on Mac? Since you need an #ifdef at the point where you call this anyway, it doesn’t seem useful to even call it on Mac. https://codereview.chromium.org/2687713003/diff/80001/base/files/file_util_po... base/files/file_util_posix.cc:196: size_t comma_pos = result.find(L','); No L https://codereview.chromium.org/2687713003/diff/80001/base/files/file_util_po... base/files/file_util_posix.cc:730: DCHECK(::strchr(mode, 'e') == nullptr || No :: https://codereview.chromium.org/2687713003/diff/80001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2687713003/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:600: DCHECK(::strchr(mode, 'N') == nullptr || No ::
Awesome, thanks! https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util.h#... base/files/file_util.h:312: // configured to not be propagated to child processes. s/propagated to/inherited by/ (since this is what the nomenclature around these uses?) https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_po... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_po... base/files/file_util_posix.cc:189: // non-Mac platforms (which do not support this); see Link to https://developer.apple.com/legacy/library/documentation/Darwin/Reference/Man... for Mac. https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_po... base/files/file_util_posix.cc:190: // https://www.gnu.org/software/libc/manual/html_node/Opening-Streams.html for I don't see mention of the comma @ this link. https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:5: #include <fcntl.h> #if POSIX https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:11: #include <initializer_list> Is this implicitly required by the for-each below or is it a leftover from something else you tried? https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:251: // Sets |is_inheritable| to true if |stream| is set up to be inerhited into This reads to me as though it "sets it to true iff it's on and doesn't touch it otherwise". Maybe: // Barring a fatal assertion: sets |is_inheritable| to indicate whether |stream| is set up to ... https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:1740: TEST_F(FileUtilTest, OpenFileNoInheritance) { Assuming you verified that this test fails with the old impl? (I tend to like an explicit TEST= field in CL desc mentioning that for regression tests) https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:1743: for (const char* mode : {"wb", "r,ccs=UNICODE"}) { SCOPED_TRACE(mode); https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:1746: ASSERT_NE(nullptr, file); ASSERT_TRUE(file); https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:1747: bool is_inheritable = false; Initialize to |true| to ensure GetIsInheritable() truly flipped it?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Stop base::OpenFile from leaking fds/handles into child procs. BUG=688362 ========== to ========== Stop base::OpenFile from leaking fds/handles into child procs. BUG=688362 TEST=FileUtilTest.OpenFileNoInheritance added to make sure it works as intended. ==========
On 2017/02/09 15:15:16, Mark Mentovai wrote: > There is no magic character. Here’s the source for fopen() on Mac: > > https://opensource.apple.com/source/Libc/Libc-1158.30.7/stdio/FreeBSD/fopen.c... > > Flags are translated by __sflags() in > https://opensource.apple.com/source/Libc/Libc-1158.30.7/stdio/FreeBSD/flags.c.... Awesome. Thanks for the pointers. > As you can see, it’s somewhat straightforward to construct fopen() out of > fdopen(open()) (fdopen() source is > https://opensource.apple.com/source/Libc/Libc-1158.30.7/stdio/FreeBSD/fdopen....), > but I don’t know if that’s really worthwhile. I'll take that as an indication that you're okay with this approach. :-)
grt (UTC plus 1) wrote: > On 2017/02/09 15:15:16, Mark Mentovai wrote: > > As you can see, it’s somewhat straightforward to construct fopen() out of > > fdopen(open()) (fdopen() source is > > > https://opensource.apple.com/source/Libc/Libc-1158.30.7/stdio/FreeBSD/fdopen....), > > but I don’t know if that’s really worthwhile. > > I'll take that as an indication that you're okay with this approach. :-) Affirmative!
Responses to some comments below. I will send up another patch set that addresses the others later. https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util.h#... base/files/file_util.h:312: // configured to not be propagated to child processes. On 2017/02/09 15:18:08, gab wrote: > s/propagated to/inherited by/ (since this is what the nomenclature around these > uses?) I avoided "inherited" since it's Windows-specific. "Propagate" seemed more platform neutral and conveyed the meaning. WDYT? https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_po... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_po... base/files/file_util_posix.cc:190: // https://www.gnu.org/software/libc/manual/html_node/Opening-Streams.html for On 2017/02/09 15:18:09, gab wrote: > I don't see mention of the comma @ this link. Search for "If the opentype string contains the sequence ,ccs=STRING" https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:5: #include <fcntl.h> On 2017/02/09 15:18:09, gab wrote: > #if POSIX Is this conventional? For some reason I'd thought that only platform-specific headers were generally put in #if blocks. https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:11: #include <initializer_list> On 2017/02/09 15:18:09, gab wrote: > Is this implicitly required by the for-each below or is it a leftover from > something else you tried? It is explicitly required for the for loop. :-) https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:1740: TEST_F(FileUtilTest, OpenFileNoInheritance) { On 2017/02/09 15:18:09, gab wrote: > Assuming you verified that this test fails with the old impl? Yes, indeed. > (I tend to like an explicit TEST= field in CL desc mentioning that for > regression tests) Added (is that what you meant?) https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:1746: ASSERT_NE(nullptr, file); On 2017/02/09 15:18:09, gab wrote: > ASSERT_TRUE(file); I don't get this. |file| is a pointer. Why do a bool conversion when what I care about is that it isn't null?
Description was changed from ========== Stop base::OpenFile from leaking fds/handles into child procs. BUG=688362 TEST=FileUtilTest.OpenFileNoInheritance added to make sure it works as intended. ========== to ========== Stop base::OpenFile from leaking fds/handles into child procs. BUG=688362 TEST=New FileUtilTest.OpenFileNoInheritance fails with previous impl, passes with new impl. ==========
https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util.h#... base/files/file_util.h:312: // configured to not be propagated to child processes. On 2017/02/09 15:57:01, grt (UTC plus 1) wrote: > On 2017/02/09 15:18:08, gab wrote: > > s/propagated to/inherited by/ (since this is what the nomenclature around > these > > uses?) > > I avoided "inherited" since it's Windows-specific. "Propagate" seemed more > platform neutral and conveyed the meaning. WDYT? Ah ok, sounds fine to me then. https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:5: #include <fcntl.h> On 2017/02/09 15:57:01, grt (UTC plus 1) wrote: > On 2017/02/09 15:18:09, gab wrote: > > #if POSIX > > Is this conventional? For some reason I'd thought that only platform-specific > headers were generally put in #if blocks. Actually, fcntl.h is already included in the OS_POSIX block below :). FWIW, I like includes only used by a section of the file to only be included for it (especially system headers and especially in a file like this which already has OS specific blocks). https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:1740: TEST_F(FileUtilTest, OpenFileNoInheritance) { On 2017/02/09 15:57:01, grt (UTC plus 1) wrote: > On 2017/02/09 15:18:09, gab wrote: > > Assuming you verified that this test fails with the old impl? > > Yes, indeed. > > > (I tend to like an explicit TEST= field in CL desc mentioning that for > > regression tests) > > Added (is that what you meant?) Not quite, i.e. wanted to explicitly state that it fails under old impl and passes now, I edited it already :) https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:1746: ASSERT_NE(nullptr, file); On 2017/02/09 15:57:01, grt (UTC plus 1) wrote: > On 2017/02/09 15:18:09, gab wrote: > > ASSERT_TRUE(file); > > I don't get this. |file| is a pointer. Why do a bool conversion when what I care > about is that it isn't null? I don't care strongly but the way I see it: if this were code logic we'd write if (file) not if (file != nullptr) right? (I can't find where but I swear there was recommendation to not add == null or != null in code logic) The ASSERT_NE macro on failure can't give a better printed output than ASSERT_TRUE (i.e. we already know one of the values so |file| would have to be == nullptr).
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:100001) has been deleted
Thanks. PTAL. https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_po... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_po... base/files/file_util_posix.cc:189: // non-Mac platforms (which do not support this); see On 2017/02/09 15:18:09, gab wrote: > Link to > https://developer.apple.com/legacy/library/documentation/Darwin/Reference/Man... > for Mac. Done. https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:5: #include <fcntl.h> On 2017/02/09 17:01:24, gab wrote: > On 2017/02/09 15:57:01, grt (UTC plus 1) wrote: > > On 2017/02/09 15:18:09, gab wrote: > > > #if POSIX > > > > Is this conventional? For some reason I'd thought that only platform-specific > > headers were generally put in #if blocks. > > Actually, fcntl.h is already included in the OS_POSIX block below :). Well, then... > FWIW, I like includes only used by a section of the file to only be included for > it (especially system headers and especially in a file like this which already > has OS specific blocks). SGTM https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:251: // Sets |is_inheritable| to true if |stream| is set up to be inerhited into On 2017/02/09 15:18:09, gab wrote: > This reads to me as though it "sets it to true iff it's on and doesn't touch it > otherwise". > > Maybe: > > // Barring a fatal assertion: sets |is_inheritable| to indicate whether |stream| > is set up to ... Comment updated. https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:1743: for (const char* mode : {"wb", "r,ccs=UNICODE"}) { On 2017/02/09 15:18:09, gab wrote: > SCOPED_TRACE(mode); Done. https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:1746: ASSERT_NE(nullptr, file); On 2017/02/09 17:01:24, gab wrote: > On 2017/02/09 15:57:01, grt (UTC plus 1) wrote: > > On 2017/02/09 15:18:09, gab wrote: > > > ASSERT_TRUE(file); > > > > I don't get this. |file| is a pointer. Why do a bool conversion when what I > care > > about is that it isn't null? > > I don't care strongly but the way I see it: if this were code logic we'd write > > if (file) > > not > > if (file != nullptr) > > right? (I can't find where but I swear there was recommendation to not add == > null or != null in code logic) > > The ASSERT_NE macro on failure can't give a better printed output than > ASSERT_TRUE (i.e. we already know one of the values so |file| would have to be > == nullptr). I see where you're going, but looking at this failure: ../../base/files/file_util_unittest.cc(1746): error: Value of: file Actual: false Expected: true it's telling me a bool had the wrong value. I think the gap between seeing the failure and understanding it is a bit smaller without involving booleans. https://codereview.chromium.org/2687713003/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:1747: bool is_inheritable = false; On 2017/02/09 15:18:09, gab wrote: > Initialize to |true| to ensure GetIsInheritable() truly flipped it? Done. https://codereview.chromium.org/2687713003/diff/80001/base/files/file_util_po... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2687713003/diff/80001/base/files/file_util_po... base/files/file_util_posix.cc:189: // non-Mac platforms (which do not support this); see On 2017/02/09 15:15:16, Mark Mentovai wrote: > Why even bother defining this on Mac? Since you need an #ifdef at the point > where you call this anyway, it doesn’t seem useful to even call it on Mac. Done. https://codereview.chromium.org/2687713003/diff/80001/base/files/file_util_po... base/files/file_util_posix.cc:196: size_t comma_pos = result.find(L','); On 2017/02/09 15:15:16, Mark Mentovai wrote: > No L Done. https://codereview.chromium.org/2687713003/diff/80001/base/files/file_util_po... base/files/file_util_posix.cc:730: DCHECK(::strchr(mode, 'e') == nullptr || On 2017/02/09 15:15:16, Mark Mentovai wrote: > No :: Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_p... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_p... base/files/file_util_posix.cc:193: // Add |mode_char| immediately before the comma or at the end of the string. This comment says basically the same thing as the other comment. Pick one! https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_u... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_u... base/files/file_util_unittest.cc:1750: ASSERT_NO_FATAL_FAILURE(GetIsInheritable(file, &is_inheritable)); You do want to reach CloseFile() even if this fails, but the assert aspect of this will cause it to return immediately instead. https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_w... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_w... base/files/file_util_win.cc:76: // Add |mode_char| immediately before the comma or at the end of the string. Pretty much a dupe here too.
Thanks. Any final comments from you, Gab? If no, please feel at liberty to check the commit box. Thanks. https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_p... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_p... base/files/file_util_posix.cc:193: // Add |mode_char| immediately before the comma or at the end of the string. On 2017/02/11 00:52:29, Mark Mentovai wrote: > This comment says basically the same thing as the other comment. Pick one! Done. https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_u... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_u... base/files/file_util_unittest.cc:1750: ASSERT_NO_FATAL_FAILURE(GetIsInheritable(file, &is_inheritable)); On 2017/02/11 00:52:29, Mark Mentovai wrote: > You do want to reach CloseFile() even if this fails, but the assert aspect of > this will cause it to return immediately instead. Good point. Moved CloseFile into a ScopedClosureRunner so that it's run unconditionally. https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_w... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2687713003/diff/120001/base/files/file_util_w... base/files/file_util_win.cc:76: // Add |mode_char| immediately before the comma or at the end of the string. On 2017/02/11 00:52:29, Mark Mentovai wrote: > Pretty much a dupe here too. Done.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nit (sorry for delay was at offsite on Friday) https://codereview.chromium.org/2687713003/diff/140001/base/files/file_util_u... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2687713003/diff/140001/base/files/file_util_u... base/files/file_util_unittest.cc:273: FAIL() << "Unimplemented"; Typically this is done as #error Not implemented (compile error is better than runtime error)
Thanks. https://codereview.chromium.org/2687713003/diff/140001/base/files/file_util_u... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2687713003/diff/140001/base/files/file_util_u... base/files/file_util_unittest.cc:273: FAIL() << "Unimplemented"; On 2017/02/13 15:10:01, gab wrote: > Typically this is done as > > #error Not implemented > > (compile error is better than runtime error) Done.
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2687713003/#ps160001 (title: "gab comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487002604140140,
"parent_rev": "34bda99007770ad9795662b7dc544affc2b778da", "commit_rev":
"d0bc44fa24fba9f483aaddae8e2c29df3524a5d6"}
Message was sent while issue was closed.
Description was changed from ========== Stop base::OpenFile from leaking fds/handles into child procs. BUG=688362 TEST=New FileUtilTest.OpenFileNoInheritance fails with previous impl, passes with new impl. ========== to ========== Stop base::OpenFile from leaking fds/handles into child procs. BUG=688362 TEST=New FileUtilTest.OpenFileNoInheritance fails with previous impl, passes with new impl. Review-Url: https://codereview.chromium.org/2687713003 Cr-Commit-Position: refs/heads/master@{#449999} Committed: https://chromium.googlesource.com/chromium/src/+/d0bc44fa24fba9f483aaddae8e2c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/d0bc44fa24fba9f483aaddae8e2c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
