New issue
Advanced search Search tips

Issue 872887 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 23
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: ----
Type: ----



Sign in to add a comment

Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader

Project Member Reported by dxie@google.com, Aug 9

Issue description


 
Labels: Proj-Servicification Proj-Servicification-VendorBug Hotlist-KnownIssue
Labels: Proj-Servicification-Network-Url
Owner: svil...@igalia.com
Status: Assigned (was: Available)
Taking this
Owner: toniki...@chromium.org
Status: Started (was: Assigned)
Agreed with svillar offline to take over the work here.
FTR: https://crrev.com/c/1289849 is up for review.
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c7e36b09013c046e265d70dfed0ca40b73bc23da

commit c7e36b09013c046e265d70dfed0ca40b73bc23da
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Sun Oct 21 03:54:44 2018

Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader

URLFetcher will stop working with advent of Network Service, and
SimpleURLLoader is the replacement API for most clients.
This CL migrates chromedriver and its respective unittests away from
URLFetcher.

Note that because of the multithreaded nature of the code, the natural
approach was to simply call SharedURLLoaderFactory::Clone and pass
the "info" mojo handle accross threads, until the operational thread.
However, the structure holding this mojo handle is called/passed from

  (i) an indirect chain of BindRepeating calls,
  (ii) various threads.

Given that network::mojom::URLLoaderFactoryPtrInfo and
network::SharedURLLoaderFactoryInfo are both move-only object
(which does not comply with BingRepeating) and that raw
a network::SharedURLoaderLoader instance must operate on the same
thread it was created on, this CL comes up with a local thread-agnostic
"wrapper" to mojom::URLLoaderFactory, named WrapperURLLoaderFactory
(see chrome/test/chromedriver/server/http_handler.cc).
Its instance replaces URLRequestContextGetter instead across threads
and BindRepeating chains just fine.

This CL is based on the original work of Sergio Villar (svillar@igalia.com)
on [1].

TBR=johnchen@chromium.org (John +1'ed the original incarnation of this CL at [1]).

[1] https://crrev.com/c/1221256

Bug:  872887 

Change-Id: I21386edb04b25438ba3aa40dd5ecc1461342aecf
Reviewed-on: https://chromium-review.googlesource.com/c/1289849
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601422}
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/BUILD.gn
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/chrome/devtools_http_client.cc
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/chrome/devtools_http_client.h
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/chrome_launcher.cc
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/chrome_launcher.h
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/log_replay/replay_http_client.cc
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/log_replay/replay_http_client.h
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/net/net_util.cc
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/net/net_util.h
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/net/net_util_unittest.cc
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/server/chromedriver_server.cc
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/server/http_handler.cc
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/server/http_handler.h
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/session_commands.cc
[modify] https://crrev.com/c7e36b09013c046e265d70dfed0ca40b73bc23da/chrome/test/chromedriver/session_commands.h

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/557c93c3070e306ea9b685632dddc7c8c8fd8d4b

commit 557c93c3070e306ea9b685632dddc7c8c8fd8d4b
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Sun Oct 21 05:02:08 2018

Revert "Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader"

This reverts commit c7e36b09013c046e265d70dfed0ca40b73bc23da.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 601422 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2M3ZTM2YjA5MDEzYzA0NmUyNjVkNzBkZmVkMGNhNDBiNzNiYzIzZGEM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium/mac-dbg/1239

Sample Failed Step: compile

Original change's description:
> Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader
> 
> URLFetcher will stop working with advent of Network Service, and
> SimpleURLLoader is the replacement API for most clients.
> This CL migrates chromedriver and its respective unittests away from
> URLFetcher.
> 
> Note that because of the multithreaded nature of the code, the natural
> approach was to simply call SharedURLLoaderFactory::Clone and pass
> the "info" mojo handle accross threads, until the operational thread.
> However, the structure holding this mojo handle is called/passed from
> 
>   (i) an indirect chain of BindRepeating calls,
>   (ii) various threads.
> 
> Given that network::mojom::URLLoaderFactoryPtrInfo and
> network::SharedURLLoaderFactoryInfo are both move-only object
> (which does not comply with BingRepeating) and that raw
> a network::SharedURLoaderLoader instance must operate on the same
> thread it was created on, this CL comes up with a local thread-agnostic
> "wrapper" to mojom::URLLoaderFactory, named WrapperURLLoaderFactory
> (see chrome/test/chromedriver/server/http_handler.cc).
> Its instance replaces URLRequestContextGetter instead across threads
> and BindRepeating chains just fine.
> 
> This CL is based on the original work of Sergio Villar (svillar@igalia.com)
> on [1].
> 
> TBR=johnchen@chromium.org (John +1'ed the original incarnation of this CL at [1]).
> 
> [1] https://crrev.com/c/1221256
> 
> Bug:  872887 
> 
> Change-Id: I21386edb04b25438ba3aa40dd5ecc1461342aecf
> Reviewed-on: https://chromium-review.googlesource.com/c/1289849
> Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#601422}

Change-Id: I0bb5f07fde222dacb7e34892d907458c8b8535a5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  872887 
Reviewed-on: https://chromium-review.googlesource.com/c/1293041
Cr-Commit-Position: refs/heads/master@{#601430}
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/BUILD.gn
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/chrome/devtools_http_client.cc
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/chrome/devtools_http_client.h
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/chrome_launcher.cc
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/chrome_launcher.h
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/log_replay/replay_http_client.cc
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/log_replay/replay_http_client.h
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/net/net_util.cc
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/net/net_util.h
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/net/net_util_unittest.cc
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/server/chromedriver_server.cc
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/server/http_handler.cc
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/server/http_handler.h
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/session_commands.cc
[modify] https://crrev.com/557c93c3070e306ea9b685632dddc7c8c8fd8d4b/chrome/test/chromedriver/session_commands.h

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/930cc098af1abb8e09065037de9dc3938d24e858

commit 930cc098af1abb8e09065037de9dc3938d24e858
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Mon Oct 22 04:59:46 2018

Reland "Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader"

This is a reland of c7e36b09013c046e265d70dfed0ca40b73bc23da

The difference between this CL and its original version
(https://crrev.com/c/c1289849) is that dependencies are made
more explicit in chrome/test/chromedriver/BUILD.gn.

TBR=mmenke@chromium.org,johnchen@chromium.org (John +1'ed the original incarnation of this CL at [1]).

Original change's description:
> Migrate chrome/test/chromedriver/net/net_util.cc using SimpleURLLoader
>
> URLFetcher will stop working with advent of Network Service, and
> SimpleURLLoader is the replacement API for most clients.
> This CL migrates chromedriver and its respective unittests away from
> URLFetcher.
>
> Note that because of the multithreaded nature of the code, the natural
> approach was to simply call SharedURLLoaderFactory::Clone and pass
> the "info" mojo handle accross threads, until the operational thread.
> However, the structure holding this mojo handle is called/passed from
>
>   (i) an indirect chain of BindRepeating calls,
>   (ii) various threads.
>
> Given that network::mojom::URLLoaderFactoryPtrInfo and
> network::SharedURLLoaderFactoryInfo are both move-only object
> (which does not comply with BingRepeating) and that raw
> a network::SharedURLoaderLoader instance must operate on the same
> thread it was created on, this CL comes up with a local thread-agnostic
> "wrapper" to mojom::URLLoaderFactory, named WrapperURLLoaderFactory
> (see chrome/test/chromedriver/server/http_handler.cc).
> Its instance replaces URLRequestContextGetter instead across threads
> and BindRepeating chains just fine.
>
> This CL is based on the original work of Sergio Villar (svillar@igalia.com)
> on [1].
>
> TBR=johnchen@chromium.org (John +1'ed the original incarnation of this CL at [1]).
>
> [1] https://crrev.com/c/1221256
>
> Bug:  872887 
>
> Change-Id: I21386edb04b25438ba3aa40dd5ecc1461342aecf
> Reviewed-on: https://chromium-review.googlesource.com/c/1289849
> Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#601422}

Bug:  872887 
Change-Id: I645a34d28cfb92c15f2f2db409e0fe7cb7a5ac6b
Reviewed-on: https://chromium-review.googlesource.com/c/1292933
Reviewed-by: Antonio Gomes <tonikitoo@igalia.com>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#601478}
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/BUILD.gn
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/chrome/devtools_http_client.cc
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/chrome/devtools_http_client.h
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/chrome_launcher.cc
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/chrome_launcher.h
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/log_replay/replay_http_client.cc
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/log_replay/replay_http_client.h
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/net/net_util.cc
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/net/net_util.h
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/net/net_util_unittest.cc
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/server/chromedriver_server.cc
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/server/http_handler.cc
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/server/http_handler.h
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/session_commands.cc
[modify] https://crrev.com/930cc098af1abb8e09065037de9dc3938d24e858/chrome/test/chromedriver/session_commands.h

Status: Fixed (was: Started)

Sign in to add a comment