Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043753002:
Declarative resource hints go through mojo IPC to //content
|
Created:
4 years, 6 months ago by Charlie Harrison Modified:
3 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, gavinp+prerender_chromium.org, gavinp+loader_chromium.org, jam, Nate Chapin, kinuko+watch, loading-reviews_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, tyoshino+watch_chromium.org, viettrungluu+watch_chromium.org, Yoav Weiss, yzshen+watch_chromium.org, horo Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDeclarative resource hints go through mojo IPC to //content
This patch adds a mojo IPC route for preconnects / preresolves
that are declared by site authors (i.e. not speculative).
Speculative hints (anchor tag scanning, mouse hovering) still
go through traditional IPC, and will be ported to this sytem
(using a delegate system) in a follow up patch.
BUG=610750, 565719
Patch Set 1 #Patch Set 2 : remove platform dependency from KURL.typemap #Patch Set 3 : #
Total comments: 6
Patch Set 4 : mek@ review #Patch Set 5 #
Total comments: 19
Patch Set 6 : kinuko@ + yzshen@ reviews, also big refactor for mojo in Platform #Patch Set 7 : Move PlatformMojoMock into platform/testing #Patch Set 8 : Use unique_ptr to avoid memory leaks in unit tests #
Total comments: 27
Patch Set 9 : Don't kill NetworkHints yet #Patch Set 10 : Update comment and get rid of the platform mock #
Total comments: 8
Patch Set 11 : kinuko@ review: comments and remove PlatformMojoMock.h from gypi #
Total comments: 2
Patch Set 12 : remove gmocking + add another browser test #
Total comments: 1
Messages
Total messages: 70 (10 generated)
Description was changed from ========== Declarative resource hints go through mojo IPC to //content BUG= ========== to ========== Declarative resource hints go through mojo IPC to //content This patch adds a mojo IPC route for preconnects / preresolves that are declared by site authors (i.e. not speculative). Speculative hints (anchor tag scanning, mouse hovering) still go through traditional IPC, and will be ported to this sytem (using a delegate system) in a follow up patch. BUG=610750, 565719 ==========
yzshen@, do you mind taking a look at this?
I've refactored a bit since we last spoke, and now the mojo client is in
platform/. It seems like platform cannot reference mojom files yet, and adding a
dependency to component("platform") with
"third_party/WebKit/public:mojo_bindings" doesn't seem to work.
csharrison@chromium.org changed reviewers: + yzshen@chromium.org
yzshen@, do you mind taking a look at this?
I've refactored a bit since we last spoke, and now the mojo client is in
platform/. It seems like platform cannot reference mojom files yet, and adding a
dependency to component("platform") with
"third_party/WebKit/public:mojo_bindings" doesn't seem to work.
On 2016/06/06 14:45:29, csharrison wrote:
> yzshen@, do you mind taking a look at this?
>
> I've refactored a bit since we last spoke, and now the mojo client is in
> platform/. It seems like platform cannot reference mojom files yet, and adding
a
> dependency to component("platform") with
> "third_party/WebKit/public:mojo_bindings" doesn't seem to work.
Also, could you look at resource_hints_controller.cc? In particular, I'd like to
know the semantics around keeping a service scoped to the lifetime of a message
channel. In this case, the ResourceContext* must live longer than the channel.
csharrison@chromium.org changed reviewers: + kinuko@chromium.org
+kinuko@ for a general review as well.
On 2016/06/06 14:45:12, csharrison wrote:
> yzshen@, do you mind taking a look at this?
>
> I've refactored a bit since we last spoke, and now the mojo client is in
> platform/.
I am not sure we have had a conversation about this particular work. Are you
sending this review to the right person? (Maybe I have totally lost track of
things? :))
> It seems like platform cannot reference mojom files yet, and adding a
> dependency to component("platform") with
> "third_party/WebKit/public:mojo_bindings" doesn't seem to work.
What is the problem that you see?
On 2016/06/06 21:12:07, yzshen1 wrote:
> On 2016/06/06 14:45:12, csharrison wrote:
> > yzshen@, do you mind taking a look at this?
> >
> > I've refactored a bit since we last spoke, and now the mojo client is in
> > platform/.
> I am not sure we have had a conversation about this particular work. Are you
> sending this review to the right person? (Maybe I have totally lost track of
> things? :))
>
> > It seems like platform cannot reference mojom files yet, and adding a
> > dependency to component("platform") with
> > "third_party/WebKit/public:mojo_bindings" doesn't seem to work.
>
> What is the problem that you see?
I see the exact same compile errors (i.e. undefined reference to
blink::mojom::blink::ResourceHintsDispatcherHostProxy::ResourceHintsDispatcherHostProxy...)
If I also add a dependency on "mojo_bindings_blink" (which I can't see the
definition for, for some reason), then I get a cyclic dependency:
ERROR Dependency cycle:
//third_party/WebKit/Source/platform:platform ->
//third_party/WebKit/public:mojo_bindings_blink ->
//third_party/WebKit/public:mojo_bindings_blink_cpp_sources ->
//url/mojo:url_mojom_gurl_blink_cpp_sources ->
//third_party/WebKit/Source/platform:platform
I only tried this because that's what "modules" uses for dependencies (i.e.
mojo_bindings and mojo_bindings_blink).
On 2016/06/06 at 21:30:53, csharrison wrote:
> On 2016/06/06 21:12:07, yzshen1 wrote:
> > On 2016/06/06 14:45:12, csharrison wrote:
> > > yzshen@, do you mind taking a look at this?
> > >
> > > I've refactored a bit since we last spoke, and now the mojo client is in
> > > platform/.
> > I am not sure we have had a conversation about this particular work. Are you
> > sending this review to the right person? (Maybe I have totally lost track of
> > things? :))
> >
> > > It seems like platform cannot reference mojom files yet, and adding a
> > > dependency to component("platform") with
> > > "third_party/WebKit/public:mojo_bindings" doesn't seem to work.
> >
> > What is the problem that you see?
>
> I see the exact same compile errors (i.e. undefined reference to
blink::mojom::blink::ResourceHintsDispatcherHostProxy::ResourceHintsDispatcherHostProxy...)
>
> If I also add a dependency on "mojo_bindings_blink" (which I can't see the
definition for, for some reason), then I get a cyclic dependency:
> ERROR Dependency cycle:
> //third_party/WebKit/Source/platform:platform ->
> //third_party/WebKit/public:mojo_bindings_blink ->
> //third_party/WebKit/public:mojo_bindings_blink_cpp_sources ->
> //url/mojo:url_mojom_gurl_blink_cpp_sources ->
> //third_party/WebKit/Source/platform:platform
>
> I only tried this because that's what "modules" uses for dependencies (i.e.
mojo_bindings and mojo_bindings_blink).
Ah, fun... That sounds like (part of) the problem might be that I put the blink
typemaps for urls and origins in platform/, which currently means that any mojo
binding using urls ends up depending on platform (via the "deps" in
src/third_party/WebKit/Source/platform/mojo/KURL.typemap). I'm afraid I don't
have any helpful suggestions on what to do about this. Not sure which dependency
is "wrong" or how our build system (and in particular all the magic around GN
rules for mojo bindings) is supposed to work in these cases...
On 2016/06/06 21:36:34, Marijn Kruisselbrink wrote:
> On 2016/06/06 at 21:30:53, csharrison wrote:
> > On 2016/06/06 21:12:07, yzshen1 wrote:
> > > On 2016/06/06 14:45:12, csharrison wrote:
> > > > yzshen@, do you mind taking a look at this?
> > > >
> > > > I've refactored a bit since we last spoke, and now the mojo client is in
> > > > platform/.
> > > I am not sure we have had a conversation about this particular work. Are
you
> > > sending this review to the right person? (Maybe I have totally lost track
of
> > > things? :))
> > >
> > > > It seems like platform cannot reference mojom files yet, and adding a
> > > > dependency to component("platform") with
> > > > "third_party/WebKit/public:mojo_bindings" doesn't seem to work.
> > >
> > > What is the problem that you see?
> >
> > I see the exact same compile errors (i.e. undefined reference to
>
blink::mojom::blink::ResourceHintsDispatcherHostProxy::ResourceHintsDispatcherHostProxy...)
> >
> > If I also add a dependency on "mojo_bindings_blink" (which I can't see the
> definition for, for some reason), then I get a cyclic dependency:
> > ERROR Dependency cycle:
> > //third_party/WebKit/Source/platform:platform ->
> > //third_party/WebKit/public:mojo_bindings_blink ->
> > //third_party/WebKit/public:mojo_bindings_blink_cpp_sources ->
> > //url/mojo:url_mojom_gurl_blink_cpp_sources ->
> > //third_party/WebKit/Source/platform:platform
> >
> > I only tried this because that's what "modules" uses for dependencies (i.e.
> mojo_bindings and mojo_bindings_blink).
>
> Ah, fun... That sounds like (part of) the problem might be that I put the
blink
> typemaps for urls and origins in platform/, which currently means that any
mojo
> binding using urls ends up depending on platform (via the "deps" in
> src/third_party/WebKit/Source/platform/mojo/KURL.typemap). I'm afraid I don't
> have any helpful suggestions on what to do about this. Not sure which
dependency
> is "wrong" or how our build system (and in particular all the magic around GN
> rules for mojo bindings) is supposed to work in these cases...
Hm thanks for chiming in, mek@. Yeah this seems like it's probably the
fundamental problem.
In general, it seems like this should be possible, so I'd like to find a way to
do this without resorting to workarounds (I had a compileable version with the
mojo client in core but this is cleaner).
Is there a build expert who can help out on this?
On 2016/06/06 21:36:34, Marijn Kruisselbrink wrote:
> On 2016/06/06 at 21:30:53, csharrison wrote:
> > On 2016/06/06 21:12:07, yzshen1 wrote:
> > > On 2016/06/06 14:45:12, csharrison wrote:
> > > > yzshen@, do you mind taking a look at this?
> > > >
> > > > I've refactored a bit since we last spoke, and now the mojo client is in
> > > > platform/.
> > > I am not sure we have had a conversation about this particular work. Are
you
> > > sending this review to the right person? (Maybe I have totally lost track
of
> > > things? :))
> > >
> > > > It seems like platform cannot reference mojom files yet, and adding a
> > > > dependency to component("platform") with
> > > > "third_party/WebKit/public:mojo_bindings" doesn't seem to work.
> > >
> > > What is the problem that you see?
> >
> > I see the exact same compile errors (i.e. undefined reference to
>
blink::mojom::blink::ResourceHintsDispatcherHostProxy::ResourceHintsDispatcherHostProxy...)
> >
> > If I also add a dependency on "mojo_bindings_blink" (which I can't see the
> definition for, for some reason), then I get a cyclic dependency:
> > ERROR Dependency cycle:
> > //third_party/WebKit/Source/platform:platform ->
> > //third_party/WebKit/public:mojo_bindings_blink ->
> > //third_party/WebKit/public:mojo_bindings_blink_cpp_sources ->
> > //url/mojo:url_mojom_gurl_blink_cpp_sources ->
> > //third_party/WebKit/Source/platform:platform
> >
> > I only tried this because that's what "modules" uses for dependencies (i.e.
> mojo_bindings and mojo_bindings_blink).
>
> Ah, fun... That sounds like (part of) the problem might be that I put the
blink
> typemaps for urls and origins in platform/, which currently means that any
mojo
> binding using urls ends up depending on platform (via the "deps" in
> src/third_party/WebKit/Source/platform/mojo/KURL.typemap). I'm afraid I don't
> have any helpful suggestions on what to do about this. Not sure which
dependency
> is "wrong" or how our build system (and in particular all the magic around GN
> rules for mojo bindings) is supposed to work in these cases...
Right. That seems to be the problem.
Some ideas that may work:
- remove the "platform" dependency from KURL.typemap and replace it with
something smaller: maybe just necessary GN config targets, and code that uses
generated bindings referring to KURL will need to explicitly depend on
"platform"; or
- put the user in a target separate from "platform".
WDYT?
On 2016/06/06 22:12:37, yzshen1 wrote:
> On 2016/06/06 21:36:34, Marijn Kruisselbrink wrote:
> > On 2016/06/06 at 21:30:53, csharrison wrote:
> > > On 2016/06/06 21:12:07, yzshen1 wrote:
> > > > On 2016/06/06 14:45:12, csharrison wrote:
> > > > > yzshen@, do you mind taking a look at this?
> > > > >
> > > > > I've refactored a bit since we last spoke, and now the mojo client is
in
> > > > > platform/.
> > > > I am not sure we have had a conversation about this particular work. Are
> you
> > > > sending this review to the right person? (Maybe I have totally lost
track
> of
> > > > things? :))
> > > >
> > > > > It seems like platform cannot reference mojom files yet, and adding a
> > > > > dependency to component("platform") with
> > > > > "third_party/WebKit/public:mojo_bindings" doesn't seem to work.
> > > >
> > > > What is the problem that you see?
> > >
> > > I see the exact same compile errors (i.e. undefined reference to
> >
>
blink::mojom::blink::ResourceHintsDispatcherHostProxy::ResourceHintsDispatcherHostProxy...)
> > >
> > > If I also add a dependency on "mojo_bindings_blink" (which I can't see the
> > definition for, for some reason), then I get a cyclic dependency:
> > > ERROR Dependency cycle:
> > > //third_party/WebKit/Source/platform:platform ->
> > > //third_party/WebKit/public:mojo_bindings_blink ->
> > > //third_party/WebKit/public:mojo_bindings_blink_cpp_sources ->
> > > //url/mojo:url_mojom_gurl_blink_cpp_sources ->
> > > //third_party/WebKit/Source/platform:platform
> > >
> > > I only tried this because that's what "modules" uses for dependencies
(i.e.
> > mojo_bindings and mojo_bindings_blink).
> >
> > Ah, fun... That sounds like (part of) the problem might be that I put the
> blink
> > typemaps for urls and origins in platform/, which currently means that any
> mojo
> > binding using urls ends up depending on platform (via the "deps" in
> > src/third_party/WebKit/Source/platform/mojo/KURL.typemap). I'm afraid I
don't
> > have any helpful suggestions on what to do about this. Not sure which
> dependency
> > is "wrong" or how our build system (and in particular all the magic around
GN
> > rules for mojo bindings) is supposed to work in these cases...
>
> Right. That seems to be the problem.
>
> Some ideas that may work:
> - remove the "platform" dependency from KURL.typemap and replace it with
> something smaller: maybe just necessary GN config targets, and code that uses
> generated bindings referring to KURL will need to explicitly depend on
> "platform"; or
> - put the user in a target separate from "platform".
>
> WDYT?
I've removed the "platform" dependency from KURL.typemap and gn builds are
looking good. I think we still need something to get GYP builds running but I
can't seem to figure it out. I've added dependencies on
public/blink.gyp:mojo_bindings as well as the KURL typemap.
mek@chromium.org changed reviewers: + mek@chromium.org
https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/BUILD.gn (right): https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/BUILD.gn:368: "//third_party/WebKit/public:mojo_bindings", Why are you depending on the non-blink mojo bindings? https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mojo/KURL.typemap (right): https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mojo/KURL.typemap:10: "//mojo/public/cpp/bindings", It probably makes sense to make a similar change to SecurityOrigin.typemap? https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/blink.gyp (right): https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/blink.gyp:149: 'mojom_typemaps': [ You should add these typemaps to the mojo_bindings_mojom and mojo_bindings_blink_mojom targets rather than to this static library. (the KURL one for the blink version and the GURL one for the non-blink version).
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043753002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks a lot mek@ for the help. Looks like everything is good now except chromeos Debug builds? https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/BUILD.gn (right): https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/BUILD.gn:368: "//third_party/WebKit/public:mojo_bindings", On 2016/06/07 20:22:25, Marijn Kruisselbrink wrote: > Why are you depending on the non-blink mojo bindings? Removed. I thought I needed these but I guess not. https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/mojo/KURL.typemap (right): https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/mojo/KURL.typemap:10: "//mojo/public/cpp/bindings", On 2016/06/07 20:22:25, Marijn Kruisselbrink wrote: > It probably makes sense to make a similar change to SecurityOrigin.typemap? Done. https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/blink.gyp (right): https://codereview.chromium.org/2043753002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/blink.gyp:149: 'mojom_typemaps': [ On 2016/06/07 at 20:22:25, Marijn Kruisselbrink wrote: > You should add these typemaps to the mojo_bindings_mojom and mojo_bindings_blink_mojom targets rather than to this static library. (the KURL one for the blink version and the GURL one for the non-blink version). Done.
https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... File content/browser/loader/resource_hints_controller.cc (right): https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:25: mojo::InterfaceRequest<blink::mojom::ResourceHintsDispatcherHost> request) { nit: in case you don't know, you can use a shorter name blink::mojom::ResourceHintsDispatcherHostRequest https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:30: render_process_host->GetBrowserContext()->GetResourceContext(), Is it guaranteed that resource context outlives this post task and also the resulted controller? https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:54: delete this; Please take a look at strong_binding.h. It does this for you. https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkHints.cpp (right): https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkHints.cpp:71: DCHECK(!m_host.is_bound()); nit: this DCHECK is not very useful because default constructed InterfacePtr is always not bound. https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkHints.h (right): https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkHints.h:45: class ResourceHintsDispatcher { It seems this class can be hidden in an anonymous namespace in NetworkHints.cpp? https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/resource_hints.mojom (right): https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/resource_hints.mojom:13: interface ResourceHintsDispatcherHost { Naming: I don't know details about this feature, does it make sense to call it something like ResourceHintsHandler/Reporter/etc? (consider this as an optional comment.) Also, the methods could be Preconnect and Preresolve. https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/resource_hints.mojom:14: DispatchPreconnect(url.mojom.Url url, bool credentials_flag, uint8 num_connections); nit: 80 char line length
https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... File content/browser/loader/resource_hints_controller.cc (right): https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:54: delete this; On 2016/06/08 17:40:53, yzshen1 wrote: > Please take a look at strong_binding.h. > > It does this for you. Yeah we shouldn't need this. https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkHints.cpp (right): https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkHints.cpp:54: DEFINE_STATIC_LOCAL(ResourceHintsDispatcher, dispatcher, ()); For platform-level mojo calls I'm still not fully sure if we should just keep adding these as static (I imagine we'll have more)... I think we could possibly think about gathering these in a single place, e.g. within Platform. https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/resource_hints.mojom (right): https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/resource_hints.mojom:13: interface ResourceHintsDispatcherHost { On 2016/06/08 17:40:54, yzshen1 wrote: > Naming: I don't know details about this feature, does it make sense to call it > something like ResourceHintsHandler/Reporter/etc? (consider this as an optional > comment.) > > Also, the methods could be Preconnect and Preresolve. +1
Thanks for the reviews. Refactored the code so that all the calls go through Platform directly, which holds a PlatformMojoInterface for adding new mojo handles / logic. I kept the calls to preconnect/preresolve in Platform for ease of testing, where creating a mock Platform is trivial and simplifies the unit tests a lot. https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... File content/browser/loader/resource_hints_controller.cc (right): https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:25: mojo::InterfaceRequest<blink::mojom::ResourceHintsDispatcherHost> request) { On 2016/06/08 at 17:40:53, yzshen1 wrote: > nit: in case you don't know, you can use a shorter name blink::mojom::ResourceHintsDispatcherHostRequest Thank you :) done. https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:30: render_process_host->GetBrowserContext()->GetResourceContext(), On 2016/06/08 at 17:40:53, yzshen1 wrote: > Is it guaranteed that resource context outlives this post task and also the resulted controller? I'm unsure. What are the semantics of the channel's lifetime? The ResourceContext will be destroyed when the IO thread is terminated (~ProfileIOData, specifically). I chatted with mmenke@ who told me that ProfileIOData will be deleted after WebContents/RenderFrame. If the strong binding deletes us at that time I think we're good. https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:54: delete this; On 2016/06/09 at 07:09:19, kinuko wrote: > On 2016/06/08 17:40:53, yzshen1 wrote: > > Please take a look at strong_binding.h. > > > > It does this for you. > > Yeah we shouldn't need this. Done. Thanks for the pointer. https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkHints.cpp (right): https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkHints.cpp:54: DEFINE_STATIC_LOCAL(ResourceHintsDispatcher, dispatcher, ()); On 2016/06/09 at 07:09:19, kinuko wrote: > For platform-level mojo calls I'm still not fully sure if we should just keep adding these as static (I imagine we'll have more)... I think we could possibly think about gathering these in a single place, e.g. within Platform. Good idea. This will make testing simpler too (I think we can use MockPlatform instead of mocking NetworkHintsInterface). https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkHints.cpp:71: DCHECK(!m_host.is_bound()); On 2016/06/08 at 17:40:53, yzshen1 wrote: > nit: this DCHECK is not very useful because default constructed InterfacePtr is always not bound. Done. https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkHints.h (right): https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkHints.h:45: class ResourceHintsDispatcher { On 2016/06/08 at 17:40:53, yzshen1 wrote: > It seems this class can be hidden in an anonymous namespace in NetworkHints.cpp? Done. https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/resource_hints.mojom (right): https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/resource_hints.mojom:13: interface ResourceHintsDispatcherHost { On 2016/06/09 at 07:09:19, kinuko wrote: > On 2016/06/08 17:40:54, yzshen1 wrote: > > Naming: I don't know details about this feature, does it make sense to call it > > something like ResourceHintsHandler/Reporter/etc? (consider this as an optional > > comment.) > > > > Also, the methods could be Preconnect and Preresolve. > > +1 Done. Changed to ResourceHintsHandler. I used the "Dispatcher/DispatcherHost" to align with some other renderer <=> browser naming schemes, but Handler is simpler. https://codereview.chromium.org/2043753002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/resource_hints.mojom:14: DispatchPreconnect(url.mojom.Url url, bool credentials_flag, uint8 num_connections); On 2016/06/08 at 17:40:54, yzshen1 wrote: > nit: 80 char line length Done.
LGTM (for the way of using Mojo stuff; I don't know much about the resource hints feature itself.) https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... File content/browser/loader/resource_hints_controller.cc (right): https://codereview.chromium.org/2043753002/diff/80001/content/browser/loader/... content/browser/loader/resource_hints_controller.cc:30: render_process_host->GetBrowserContext()->GetResourceContext(), On 2016/06/10 14:20:00, csharrison wrote: > On 2016/06/08 at 17:40:53, yzshen1 wrote: > > Is it guaranteed that resource context outlives this post task and also the > resulted controller? > > I'm unsure. What are the semantics of the channel's lifetime? The > ResourceContext will be destroyed when the IO thread is terminated > (~ProfileIOData, specifically). I chatted with mmenke@ who told me that > ProfileIOData will be deleted after WebContents/RenderFrame. If the strong > binding deletes us at that time I think we're good. If the renderer doesn't disconnect earlier, the strong binding will get a connection error notification (and therefore delete us) from a message loop destruction observer, i.e., in the MessageLoop destructor. I don't know much as the life time of ResourceContext. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/resource_hints.mojom (right): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/resource_hints.mojom:9: // The ResourceHintsDispatcherHost sends preconnect/preresolve requests based please update the comments accordingly
yoav@yoav.ws changed reviewers: + yoav@yoav.ws
I'm probably not fully understanding the new tests, but at first glance it seems like they not verifying some things that the old tests did? Can you point out how CORS mode and protocol are still being tested? https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp (right): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp:54: preloader->preload(std::move(preloadRequest)); Where are we verifying that we're actually preconnecting with the right CORS mode and protocol? https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp (left): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp:209: ASSERT_EQ(testCase.shouldLoad, networkHints.didDnsPrefetch()); Again, where did the asserts go?
https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp (right): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp:54: preloader->preload(std::move(preloadRequest)); On 2016/06/13 at 08:26:08, Yoav Weiss wrote: > Where are we verifying that we're actually preconnecting with the right CORS mode and protocol? The protocol is included in the EXPECT_CALL with the right url. I thought that would be enough because if the urls match, the protocol should match. CORS mode is checks as the second argument, where we pass cors != CrossOriginAnonymous (credentials flag) into the preconnect call. This is the opposite of isCors, so we use testing::Ne here. The latter could probably be improved, but I was saving it for a follow up patch where we move the CrossOriginAttributeValue into platform to become part of this API.
https://codereview.chromium.org/2043753002/diff/140001/content/browser/loader... File content/browser/loader/resource_hints_controller.h (right): https://codereview.chromium.org/2043753002/diff/140001/content/browser/loader... content/browser/loader/resource_hints_controller.h:20: class ResourceHintsController : public blink::mojom::ResourceHintsHandler { nit: the most common naming pattern seems to be to name it like MojoInterfaceNameImpl, e.g. ResourceHintsHandlerImpl, which might make this file more findable for others https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/PlatformMojoInterface.h (right): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/PlatformMojoInterface.h:12: class PlatformMojoInterface { nit: PlatformMojoInterface's'... or maybe PlatformMojoServices (I slightly prefer latter, as what we're accessing through this are mojo services) https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/PlatformMojoInterface.h:16: const mojom::blink::ResourceHintsHandlerPtr& resourceHintsHandlerHost(); nit: why we add suffix 'Host' here? Just resourceHintsHandler() (or getResourceHintsHandler()) looks enough to me https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/Platform.cpp (right): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/Platform.cpp:56: , m_mojoInterface(nullptr) shouldn't need this https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/Platform.cpp:60: Platform::~Platform() = default works? https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/Platform.cpp:110: void Platform::shutdown() should we clear mojoInterface here? https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/Platform.cpp:162: void Platform::preconnect(const KURL& url, bool credentialsFlag, int numConnections) Do we need to expose methods for each of these? Just expose mojoInterface would be enough? https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/NetworkHints.h (left): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/NetworkHints.h:38: PLATFORM_EXPORT void preconnect(const KURL&, const CrossOriginAttributeValue); I feel we can keep these as is, which could just call into Platform::current()->mojoServices()->resourceHintsController()->preconnect() etc? We don't need to make Platform class just get bloated then. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:537: virtual void speculativePreresolve(const KURL&); Do we need to expose these here? https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:620: std::unique_ptr<PlatformMojoInterface> m_mojoInterface; m_mojoInterface's' or m_mojoServices
Haven't uploaded the new PS until we come to agreement about NetworkHints.h / NetworkHintsInterface. If you want, I can keep the current interface and punt unit test refactors / call site refactors to a follow up CL. https://codereview.chromium.org/2043753002/diff/140001/content/browser/loader... File content/browser/loader/resource_hints_controller.h (right): https://codereview.chromium.org/2043753002/diff/140001/content/browser/loader... content/browser/loader/resource_hints_controller.h:20: class ResourceHintsController : public blink::mojom::ResourceHintsHandler { On 2016/06/16 at 05:39:58, kinuko wrote: > nit: the most common naming pattern seems to be to name it like MojoInterfaceNameImpl, e.g. ResourceHintsHandlerImpl, which might make this file more findable for others Done. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/PlatformMojoInterface.h (right): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/PlatformMojoInterface.h:12: class PlatformMojoInterface { On 2016/06/16 at 05:39:58, kinuko wrote: > nit: PlatformMojoInterface's'... or maybe PlatformMojoServices (I slightly prefer latter, as what we're accessing through this are mojo services) Done. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/PlatformMojoInterface.h:16: const mojom::blink::ResourceHintsHandlerPtr& resourceHintsHandlerHost(); On 2016/06/16 at 05:39:58, kinuko wrote: > nit: why we add suffix 'Host' here? Just resourceHintsHandler() (or getResourceHintsHandler()) looks enough to me Done. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/Platform.cpp (right): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/Platform.cpp:56: , m_mojoInterface(nullptr) On 2016/06/16 at 05:39:58, kinuko wrote: > shouldn't need this Done. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/Platform.cpp:60: Platform::~Platform() On 2016/06/16 at 05:39:58, kinuko wrote: > = default works? Done. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/Platform.cpp:162: void Platform::preconnect(const KURL& url, bool credentialsFlag, int numConnections) On 2016/06/16 at 05:39:58, kinuko wrote: > Do we need to expose methods for each of these? Just expose mojoInterface would be enough? SGTM. Done. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/NetworkHints.h (left): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/NetworkHints.h:38: PLATFORM_EXPORT void preconnect(const KURL&, const CrossOriginAttributeValue); On 2016/06/16 at 05:39:58, kinuko wrote: > I feel we can keep these as is, which could just call into Platform::current()->mojoServices()->resourceHintsController()->preconnect() etc? > > We don't need to make Platform class just get bloated then. What is the point of this class then? I feel like this is a layer of abstraction that is not really giving us anything, especially if we can just mock Platform/PlatformMojoServices. I think the unit tests are much clearer now, and by mocking the mojo services we can create a mock test harness to mock out all platform IPCs in a general way. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:537: virtual void speculativePreresolve(const KURL&); On 2016/06/16 at 05:39:59, kinuko wrote: > Do we need to expose these here? No, I did it here to make mocking easy for unit tests. I'll revisit this decision by only exposing the mojo interface. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:620: std::unique_ptr<PlatformMojoInterface> m_mojoInterface; On 2016/06/16 at 05:39:59, kinuko wrote: > m_mojoInterface's' or m_mojoServices m_mojoServices SGTM.
https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/NetworkHints.h (left): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/NetworkHints.h:38: PLATFORM_EXPORT void preconnect(const KURL&, const CrossOriginAttributeValue); On 2016/06/16 11:00:19, csharrison wrote: > On 2016/06/16 at 05:39:58, kinuko wrote: > > I feel we can keep these as is, which could just call into > Platform::current()->mojoServices()->resourceHintsController()->preconnect() > etc? > > > > We don't need to make Platform class just get bloated then. > > What is the point of this class then? I feel like this is a layer of abstraction > that is not really giving us anything, especially if we can just mock > Platform/PlatformMojoServices. > > I think the unit tests are much clearer now, and by mocking the mojo services we > can create a mock test harness to mock out all platform IPCs in a general way. For this one I don't have strong opinion, I seem to have confused you by saying multiple things... What I basically wanted to say is that I don't think we should have all these methods directly exposed on Platform (and guessed you might have done that to make the callsite's code simpler-- and if that's the case we could just keep this thin glue layer around). I'm cool with removing this part too
https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/NetworkHints.h (left): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/NetworkHints.h:38: PLATFORM_EXPORT void preconnect(const KURL&, const CrossOriginAttributeValue); On 2016/06/16 at 13:04:49, kinuko wrote: > On 2016/06/16 11:00:19, csharrison wrote: > > On 2016/06/16 at 05:39:58, kinuko wrote: > > > I feel we can keep these as is, which could just call into > > Platform::current()->mojoServices()->resourceHintsController()->preconnect() > > etc? > > > > > > We don't need to make Platform class just get bloated then. > > > > What is the point of this class then? I feel like this is a layer of abstraction > > that is not really giving us anything, especially if we can just mock > > Platform/PlatformMojoServices. > > > > I think the unit tests are much clearer now, and by mocking the mojo services we > > can create a mock test harness to mock out all platform IPCs in a general way. > > For this one I don't have strong opinion, I seem to have confused you by saying multiple things... What I basically wanted to say is that I don't think we should have all these methods directly exposed on Platform (and guessed you might have done that to make the callsite's code simpler-- and if that's the case we could just keep this thin glue layer around). I'm cool with removing this part too OK that sounds good to me. I agree shoving this in Platform is not great. Will upload an updated patch set soon.
Ok. I did my best to revert back to using all the NetworkHints infrastructure to make this a more minimal change. I still plan on refactoring that in a follow up, though. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/Platform.cpp (right): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/Platform.cpp:110: void Platform::shutdown() On 2016/06/16 at 05:39:58, kinuko wrote: > should we clear mojoInterface here? Done. https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/resource_hints.mojom (right): https://codereview.chromium.org/2043753002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/resource_hints.mojom:9: // The ResourceHintsDispatcherHost sends preconnect/preresolve requests based On 2016/06/10 15:45:43, yzshen1 wrote: > please update the comments accordingly Done.
lgtm As you write we could probably cleanup the interfaces / glue code further, but that can be probably done in a follow-up patch. https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/blink_platform.gypi (right): https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/blink_platform.gypi:1290: 'testing/PlatformMojoMock.h', do we still need this? https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/BUILD.gn (right): https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/BUILD.gn:172: "platform/resource_hints.mojom", (I feel these .mojom could be probably in subdirectories, e.g. platform/interfaces/... etc, but I found we already have other .mojom's at this top level directory) https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/resource_hints.mojom (right): https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/resource_hints.mojom:11: // content/public/resource_hints.h. Would be still nice to have a lightweight comment for each method, as this one defines 'interface' of this service (and not all params are too obvious) https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/resource_hints.mojom:12: // TODO(csharrison): Extend this interface to include page scans and link hovers. nit: exceeds 80 cols?
csharrison@chromium.org changed reviewers: + mmenke@chromium.org
mmenke@, feel free to punt this if you're busy. The ResourceContext won't outlive the IO thread's message loop, right? (Or, at least, they share lifetime with the life of the IO thread). I can't really reproduce the scenario where the renderer doesn't disconnect earlier. If that's the case, I'll add lifetime comments on the HandlerImpl. https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/blink_platform.gypi (right): https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/blink_platform.gypi:1290: 'testing/PlatformMojoMock.h', On 2016/06/17 04:30:24, kinuko wrote: > do we still need this? Done. https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/BUILD.gn (right): https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/BUILD.gn:172: "platform/resource_hints.mojom", On 2016/06/17 04:30:25, kinuko wrote: > (I feel these .mojom could be probably in subdirectories, e.g. > platform/interfaces/... etc, but I found we already have other .mojom's at this > top level directory) Yeah I'm not sure where to best put them. But here seems ok for now. https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/resource_hints.mojom (right): https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/resource_hints.mojom:11: // content/public/resource_hints.h. On 2016/06/17 04:30:25, kinuko wrote: > Would be still nice to have a lightweight comment for each method, as this one > defines 'interface' of this service (and not all params are too obvious) Done. https://codereview.chromium.org/2043753002/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/resource_hints.mojom:12: // TODO(csharrison): Extend this interface to include page scans and link hovers. On 2016/06/17 04:30:25, kinuko wrote: > nit: exceeds 80 cols? Done.
https://codereview.chromium.org/2043753002/diff/200001/content/browser/loader... File content/browser/loader/resource_hints_browsertest.cc (right): https://codereview.chromium.org/2043753002/diff/200001/content/browser/loader... content/browser/loader/resource_hints_browsertest.cc:50: EXPECT_CALL(*mock_host_resolver_proc_, Resolve("chromium.org", _, _, _, _)) I'd recommend against using Gmock - it reduces the amount of needed information to start hacking on chrome, and makes for much more obscure code.
https://codereview.chromium.org/2043753002/diff/200001/content/browser/loader... File content/browser/loader/resource_hints_browsertest.cc (right): https://codereview.chromium.org/2043753002/diff/200001/content/browser/loader... content/browser/loader/resource_hints_browsertest.cc:50: EXPECT_CALL(*mock_host_resolver_proc_, Resolve("chromium.org", _, _, _, _)) On 2016/06/17 20:05:36, mmenke wrote: > I'd recommend against using Gmock - it reduces the amount of needed information > to start hacking on chrome, and makes for much more obscure code. Hm. I think I disagree with you. To extend these tests, we'll need to keep track of a vector of calls to Resolve, with added boilerplate code to iterate through and find out if various hosts were called. Do you think that's a better approach? I think Gmock gets rid of a ton of unnecessary boilerplate and makes tests much simpler to understand if they follow a particular pattern (tracking method calls). Happy to defer to your opinion if you feel strongly about this though.
On 2016/06/19 11:01:57, csharrison wrote: > https://codereview.chromium.org/2043753002/diff/200001/content/browser/loader... > File content/browser/loader/resource_hints_browsertest.cc (right): > > https://codereview.chromium.org/2043753002/diff/200001/content/browser/loader... > content/browser/loader/resource_hints_browsertest.cc:50: > EXPECT_CALL(*mock_host_resolver_proc_, Resolve("chromium.org", _, _, _, _)) > On 2016/06/17 20:05:36, mmenke wrote: > > I'd recommend against using Gmock - it reduces the amount of needed > information > > to start hacking on chrome, and makes for much more obscure code. > > Hm. I think I disagree with you. To extend these tests, we'll need to keep track > of a vector of calls to Resolve, with added boilerplate code to iterate through > and find out if various hosts were called. > > Do you think that's a better approach? I think Gmock gets rid of a ton of > unnecessary boilerplate and makes tests much simpler to understand if they > follow a particular pattern (tracking method calls). > > Happy to defer to your opinion if you feel strongly about this though. fyi - in general we (chrome code base) are not a big fan of gmock, if it doesn't add too much code writing up dumb mock is still sometimes preferred.
On 2016/06/20 00:04:42, kinuko wrote: > On 2016/06/19 11:01:57, csharrison wrote: > > > https://codereview.chromium.org/2043753002/diff/200001/content/browser/loader... > > File content/browser/loader/resource_hints_browsertest.cc (right): > > > > > https://codereview.chromium.org/2043753002/diff/200001/content/browser/loader... > > content/browser/loader/resource_hints_browsertest.cc:50: > > EXPECT_CALL(*mock_host_resolver_proc_, Resolve("chromium.org", _, _, _, _)) > > On 2016/06/17 20:05:36, mmenke wrote: > > > I'd recommend against using Gmock - it reduces the amount of needed > > information > > > to start hacking on chrome, and makes for much more obscure code. > > > > Hm. I think I disagree with you. To extend these tests, we'll need to keep > track > > of a vector of calls to Resolve, with added boilerplate code to iterate > through > > and find out if various hosts were called. > > > > Do you think that's a better approach? I think Gmock gets rid of a ton of > > unnecessary boilerplate and makes tests much simpler to understand if they > > follow a particular pattern (tracking method calls). > > > > Happy to defer to your opinion if you feel strongly about this though. > > fyi - in general we (chrome code base) are not a big fan of gmock, if it doesn't > add too much code writing up dumb mock is still sometimes preferred. OK if this is general chrome style I'll remove the gmocking. Best to keep things consistent.
Ok I removed the Gmock stuff in favor of a vector / helper methods. Thanks for the input.
csharrison@chromium.org changed reviewers: + jochen@chromium.org
+jochen@ for chrome_render_message_filter.cc. Feel free to look at other stuff if you want too :)
lgtm
No need to wait for me to weigh in on this one, just to be explicit about it.
I chatted with mmenke@ offline about the lifetime issues I brought up earlier. Unfortunately, MessageLoop is destructed after ProfileIOData, so there's a chance we could use-after-free here. yzshen@, is there a weaker guarantee here that the code is safe (i.e. maybe the Handler object won't be killed, but its methods won't be called after some point X)? Alternatively, we're okay if we can prove that the renderer disconnects before the IO thread is destroyed.
On 2016/06/22 18:11:47, csharrison wrote: > I chatted with mmenke@ offline about the lifetime issues I brought up earlier. > > Unfortunately, MessageLoop is destructed after ProfileIOData, so there's a > chance we could use-after-free here. > > yzshen@, is there a weaker guarantee here that the code is safe (i.e. maybe the > Handler object won't be killed, but its methods won't be called after some point > X)? > > Alternatively, we're okay if we can prove that the renderer disconnects before > the IO thread is destroyed. Note that all RenderViewHosts should have been destroyed by the time the ProfileIOData is torn down, but I'd guess that's not enough to ensure Mojo for those processes is dead, and if we're getting mojo commands from non-renderer processes, or even the same process...That's still wouldn't be enough for any guarantees.
On 2016/06/22 18:39:01, mmenke wrote: > On 2016/06/22 18:11:47, csharrison wrote: > > I chatted with mmenke@ offline about the lifetime issues I brought up earlier. > > > > Unfortunately, MessageLoop is destructed after ProfileIOData, so there's a > > chance we could use-after-free here. > > > > yzshen@, is there a weaker guarantee here that the code is safe (i.e. maybe > the > > Handler object won't be killed, but its methods won't be called after some > point > > X)? > > > > Alternatively, we're okay if we can prove that the renderer disconnects before > > the IO thread is destroyed. > > Note that all RenderViewHosts should have been destroyed by the time the > ProfileIOData is torn down, but I'd guess that's not enough to ensure Mojo for > those processes is dead, and if we're getting mojo commands from non-renderer > processes, or even the same process...That's still wouldn't be enough for any > guarantees. I think we're ok assuming (for now at least), that the only consumer of these are render processes. The handles are registered with the service registry associated with the RPH after all. Worst comes to worst I might just make a RPH scoped object that holds these handles.
one more nit.. https://codereview.chromium.org/2043753002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/PlatformMojoServices.cpp (right): https://codereview.chromium.org/2043753002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/PlatformMojoServices.cpp:17: { Platform methods are exposed to any threads so potentially this could be called on any threads. Currently we assume this is only called on main thread, is that right? Could we add isMainThread assertion here?
On 2016/06/22 18:39:01, mmenke wrote: > On 2016/06/22 18:11:47, csharrison wrote: > > I chatted with mmenke@ offline about the lifetime issues I brought up earlier. > > > > Unfortunately, MessageLoop is destructed after ProfileIOData, so there's a > > chance we could use-after-free here. > > > > yzshen@, is there a weaker guarantee here that the code is safe (i.e. maybe > the > > Handler object won't be killed, but its methods won't be called after some > point > > X)? Can we get some kind of notification from ProfileIOData or ResourceContext when it is destroyed? > > > > Alternatively, we're okay if we can prove that the renderer disconnects before > > the IO thread is destroyed. We probably shouldn't rely on renderer's behavior because (1) they are not trusted (2) disconnect is notified asynchronously. > > Note that all RenderViewHosts should have been destroyed by the time the > ProfileIOData is torn down, but I'd guess that's not enough to ensure Mojo for > those processes is dead, and if we're getting mojo commands from non-renderer > processes, or even the same process...That's still wouldn't be enough for any > guarantees.
On 2016/06/23 16:19:38, yzshen1 wrote: > On 2016/06/22 18:39:01, mmenke wrote: > > On 2016/06/22 18:11:47, csharrison wrote: > > > I chatted with mmenke@ offline about the lifetime issues I brought up > earlier. > > > > > > Unfortunately, MessageLoop is destructed after ProfileIOData, so there's a > > > chance we could use-after-free here. > > > > > > yzshen@, is there a weaker guarantee here that the code is safe (i.e. maybe > > the > > > Handler object won't be killed, but its methods won't be called after some > > point > > > X)? > > Can we get some kind of notification from ProfileIOData or ResourceContext when > it is destroyed? > > > > > > > Alternatively, we're okay if we can prove that the renderer disconnects > before > > > the IO thread is destroyed. > > We probably shouldn't rely on renderer's behavior because (1) they are not > trusted (2) disconnect is notified asynchronously. True, but it seems like tearing down an RDH really should prevent us from getting renderer messages - I suspect most things don't expect to be receiving messages from a zombie process, except perhaps UMA data. > > Note that all RenderViewHosts should have been destroyed by the time the > > ProfileIOData is torn down, but I'd guess that's not enough to ensure Mojo for > > those processes is dead, and if we're getting mojo commands from non-renderer > > processes, or even the same process...That's still wouldn't be enough for any > > guarantees.
On 2016/06/23 16:19:38, yzshen1 wrote: > On 2016/06/22 18:39:01, mmenke wrote: > > On 2016/06/22 18:11:47, csharrison wrote: > > > I chatted with mmenke@ offline about the lifetime issues I brought up > earlier. > > > > > > Unfortunately, MessageLoop is destructed after ProfileIOData, so there's a > > > chance we could use-after-free here. > > > > > > yzshen@, is there a weaker guarantee here that the code is safe (i.e. maybe > > the > > > Handler object won't be killed, but its methods won't be called after some > > point > > > X)? > > Can we get some kind of notification from ProfileIOData or ResourceContext when > it is destroyed? I'll be a little tricky. ProfileIOData could probably send a notification somewhere, but it is a //chrome object and this code is //content. We could 1. Add methods to ResourceContext to safely scope bindings. ResourceContext can properly destroy them when it is destroyed. 2. Add general mojo functionality to destroy bindings when ProfileIOData goes away There's other ideas mmenke@ and I had, but none really feel that great. Talking to him, it seems like other IO thread objects have similar lifetime issues with mojo. It would be great if we could come up with a general strategy here that could be used elsewhere. > > > > > > > Alternatively, we're okay if we can prove that the renderer disconnects > before > > > the IO thread is destroyed. > > We probably shouldn't rely on renderer's behavior because (1) they are not > trusted (2) disconnect is notified asynchronously. Noted, yeah these are both good points. > > > > > Note that all RenderViewHosts should have been destroyed by the time the > > ProfileIOData is torn down, but I'd guess that's not enough to ensure Mojo for > > those processes is dead, and if we're getting mojo commands from non-renderer > > processes, or even the same process...That's still wouldn't be enough for any > > guarantees.
On Thu, Jun 23, 2016 at 9:30 AM, <mmenke@chromium.org> wrote: > On 2016/06/23 16:19:38, yzshen1 wrote: > > On 2016/06/22 18:39:01, mmenke wrote: > > > On 2016/06/22 18:11:47, csharrison wrote: > > > > I chatted with mmenke@ offline about the lifetime issues I brought > up > > earlier. > > > > > > > > Unfortunately, MessageLoop is destructed after ProfileIOData, so > there's a > > > > chance we could use-after-free here. > > > > > > > > yzshen@, is there a weaker guarantee here that the code is safe > (i.e. > maybe > > > the > > > > Handler object won't be killed, but its methods won't be called > after some > > > point > > > > X)? > > > > Can we get some kind of notification from ProfileIOData or > ResourceContext > when > > it is destroyed? > > > > > > > > > > Alternatively, we're okay if we can prove that the renderer > disconnects > > before > > > > the IO thread is destroyed. > > > > We probably shouldn't rely on renderer's behavior because (1) they are > not > > trusted (2) disconnect is notified asynchronously. > > True, but it seems like tearing down an RDH really should prevent us from > getting renderer messages > Sorry I probably wasn't clear: I meant we shouldn't rely on the renderer process to disconnect the mojo interface before the browser IO thread is destroyed. If you was suggesting that using RVH destruction as signal to disconnect the mojo interface from the browser side, then I think it is fine. > - I suspect most things don't expect to be receiving > messages from a zombie process, except perhaps UMA data. > > > > Note that all RenderViewHosts should have been destroyed by the time > the > > > ProfileIOData is torn down, but I'd guess that's not enough to ensure > Mojo > for > > > those processes is dead, and if we're getting mojo commands from > non-renderer > > > processes, or even the same process...That's still wouldn't be enough > for > any > > > guarantees. > > > > https://codereview.chromium.org/2043753002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, Jun 23, 2016 at 9:30 AM, <mmenke@chromium.org> wrote: > On 2016/06/23 16:19:38, yzshen1 wrote: > > On 2016/06/22 18:39:01, mmenke wrote: > > > On 2016/06/22 18:11:47, csharrison wrote: > > > > I chatted with mmenke@ offline about the lifetime issues I brought > up > > earlier. > > > > > > > > Unfortunately, MessageLoop is destructed after ProfileIOData, so > there's a > > > > chance we could use-after-free here. > > > > > > > > yzshen@, is there a weaker guarantee here that the code is safe > (i.e. > maybe > > > the > > > > Handler object won't be killed, but its methods won't be called > after some > > > point > > > > X)? > > > > Can we get some kind of notification from ProfileIOData or > ResourceContext > when > > it is destroyed? > > > > > > > > > > Alternatively, we're okay if we can prove that the renderer > disconnects > > before > > > > the IO thread is destroyed. > > > > We probably shouldn't rely on renderer's behavior because (1) they are > not > > trusted (2) disconnect is notified asynchronously. > > True, but it seems like tearing down an RDH really should prevent us from > getting renderer messages > Sorry I probably wasn't clear: I meant we shouldn't rely on the renderer process to disconnect the mojo interface before the browser IO thread is destroyed. If you was suggesting that using RVH destruction as signal to disconnect the mojo interface from the browser side, then I think it is fine. > - I suspect most things don't expect to be receiving > messages from a zombie process, except perhaps UMA data. > > > > Note that all RenderViewHosts should have been destroyed by the time > the > > > ProfileIOData is torn down, but I'd guess that's not enough to ensure > Mojo > for > > > those processes is dead, and if we're getting mojo commands from > non-renderer > > > processes, or even the same process...That's still wouldn't be enough > for > any > > > guarantees. > > > > https://codereview.chromium.org/2043753002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/23 16:32:55, csharrison wrote: > On 2016/06/23 16:19:38, yzshen1 wrote: > > On 2016/06/22 18:39:01, mmenke wrote: > > > On 2016/06/22 18:11:47, csharrison wrote: > > > > I chatted with mmenke@ offline about the lifetime issues I brought up > > earlier. > > > > > > > > Unfortunately, MessageLoop is destructed after ProfileIOData, so there's a > > > > chance we could use-after-free here. > > > > > > > > yzshen@, is there a weaker guarantee here that the code is safe (i.e. > maybe > > > the > > > > Handler object won't be killed, but its methods won't be called after some > > > point > > > > X)? > > > > Can we get some kind of notification from ProfileIOData or ResourceContext > when > > it is destroyed? > I'll be a little tricky. ProfileIOData could probably send a notification > somewhere, but it is a //chrome object and this code is //content. We could > 1. Add methods to ResourceContext to safely scope bindings. ResourceContext can > properly destroy them when it is destroyed. We've been having several ResourceContext u-a-f issues in other //content modules too (regardless of mojo usage), one idea that has been proposed several times was to let ResourceContext support WeakPtr semantics, while this has never been implemented because 1. general skepticism / code health concern around WeakPtr usage, and 2. such unsafe access happens mostly during browser shutdown sequence, which is usually not really exploitable (and many other things could happen during shutdown) so the risk is considered relatively lower priority... given that I don't feel the lifetime issue needs to be a blocker of this change, while it'd be really nice that we have clearer lifetime guarantees with mojo. > 2. Add general mojo functionality to destroy bindings when ProfileIOData goes > away > There's other ideas mmenke@ and I had, but none really feel that great. Talking > to him, it seems like other IO thread objects have similar lifetime issues with > mojo. It would be great if we could come up with a general strategy here that > could be used elsewhere. > > > > > > > > > > Alternatively, we're okay if we can prove that the renderer disconnects > > before > > > > the IO thread is destroyed. > > > > We probably shouldn't rely on renderer's behavior because (1) they are not > > trusted (2) disconnect is notified asynchronously. > Noted, yeah these are both good points. > > > > > > > > Note that all RenderViewHosts should have been destroyed by the time the > > > ProfileIOData is torn down, but I'd guess that's not enough to ensure Mojo > for > > > those processes is dead, and if we're getting mojo commands from > non-renderer > > > processes, or even the same process...That's still wouldn't be enough for > any > > > guarantees.
On 2016/06/24 05:36:24, kinuko wrote: > On 2016/06/23 16:32:55, csharrison wrote: > > On 2016/06/23 16:19:38, yzshen1 wrote: > > > On 2016/06/22 18:39:01, mmenke wrote: > > > > On 2016/06/22 18:11:47, csharrison wrote: > > > > > I chatted with mmenke@ offline about the lifetime issues I brought up > > > earlier. > > > > > > > > > > Unfortunately, MessageLoop is destructed after ProfileIOData, so there's > a > > > > > chance we could use-after-free here. > > > > > > > > > > yzshen@, is there a weaker guarantee here that the code is safe (i.e. > > maybe > > > > the > > > > > Handler object won't be killed, but its methods won't be called after > some > > > > point > > > > > X)? > > > > > > Can we get some kind of notification from ProfileIOData or ResourceContext > > when > > > it is destroyed? > > I'll be a little tricky. ProfileIOData could probably send a notification > > somewhere, but it is a //chrome object and this code is //content. We could > > 1. Add methods to ResourceContext to safely scope bindings. ResourceContext > can > > properly destroy them when it is destroyed. > > We've been having several ResourceContext u-a-f issues in other //content > modules too (regardless of mojo usage), one idea that has been proposed several > times was to let ResourceContext support WeakPtr semantics, while this has never > been implemented because 1. general skepticism / code health concern around > WeakPtr usage, and 2. such unsafe access happens mostly during browser shutdown > sequence, which is usually not really exploitable (and many other things could > happen during shutdown) so the risk is considered relatively lower priority... > given that I don't feel the lifetime issue needs to be a blocker of this change, > while it'd be really nice that we have clearer lifetime guarantees with mojo. Thanks for the additional context. Is there a crbug out for the ResourceContext lifetime issues already? Otherwise let's make one. mmenke@, how does landing this as-is sound to you? > > > 2. Add general mojo functionality to destroy bindings when ProfileIOData goes > > away > > > There's other ideas mmenke@ and I had, but none really feel that great. > Talking > > to him, it seems like other IO thread objects have similar lifetime issues > with > > mojo. It would be great if we could come up with a general strategy here that > > could be used elsewhere. > > > > > > > > > > > > > Alternatively, we're okay if we can prove that the renderer disconnects > > > before > > > > > the IO thread is destroyed. > > > > > > We probably shouldn't rely on renderer's behavior because (1) they are not > > > trusted (2) disconnect is notified asynchronously. > > Noted, yeah these are both good points. > > > > > > > > > > > Note that all RenderViewHosts should have been destroyed by the time the > > > > ProfileIOData is torn down, but I'd guess that's not enough to ensure Mojo > > for > > > > those processes is dead, and if we're getting mojo commands from > > non-renderer > > > > processes, or even the same process...That's still wouldn't be enough for > > any > > > > guarantees.
On 2016/06/24 12:31:01, csharrison wrote: > On 2016/06/24 05:36:24, kinuko wrote: > > On 2016/06/23 16:32:55, csharrison wrote: > > > On 2016/06/23 16:19:38, yzshen1 wrote: > > > > On 2016/06/22 18:39:01, mmenke wrote: > > > > > On 2016/06/22 18:11:47, csharrison wrote: > > > > > > I chatted with mmenke@ offline about the lifetime issues I brought up > > > > earlier. > > > > > > > > > > > > Unfortunately, MessageLoop is destructed after ProfileIOData, so > there's > > a > > > > > > chance we could use-after-free here. > > > > > > > > > > > > yzshen@, is there a weaker guarantee here that the code is safe (i.e. > > > maybe > > > > > the > > > > > > Handler object won't be killed, but its methods won't be called after > > some > > > > > point > > > > > > X)? > > > > > > > > Can we get some kind of notification from ProfileIOData or ResourceContext > > > when > > > > it is destroyed? > > > I'll be a little tricky. ProfileIOData could probably send a notification > > > somewhere, but it is a //chrome object and this code is //content. We could > > > 1. Add methods to ResourceContext to safely scope bindings. ResourceContext > > can > > > properly destroy them when it is destroyed. > > > > We've been having several ResourceContext u-a-f issues in other //content > > modules too (regardless of mojo usage), one idea that has been proposed > several > > times was to let ResourceContext support WeakPtr semantics, while this has > never > > been implemented because 1. general skepticism / code health concern around > > WeakPtr usage, and 2. such unsafe access happens mostly during browser > shutdown > > sequence, which is usually not really exploitable (and many other things could > > happen during shutdown) so the risk is considered relatively lower priority... > > given that I don't feel the lifetime issue needs to be a blocker of this > change, > > while it'd be really nice that we have clearer lifetime guarantees with mojo. > Thanks for the additional context. Is there a crbug out for the ResourceContext > lifetime issues already? Otherwise let's make one. > > mmenke@, how does landing this as-is sound to you? I haven't really looked at this CL, but it's worth noting this isn't only an issue during shutdown: It's also a problem when closing the last incognito window. I'm not a fan of landing known crashy code, even if we have a clear way forward. In this case, I don't think we even have that.
On 2016/06/24 12:36:40, mmenke wrote: > On 2016/06/24 12:31:01, csharrison wrote: > > On 2016/06/24 05:36:24, kinuko wrote: > > > On 2016/06/23 16:32:55, csharrison wrote: > > > > On 2016/06/23 16:19:38, yzshen1 wrote: > > > > > On 2016/06/22 18:39:01, mmenke wrote: > > > > > > On 2016/06/22 18:11:47, csharrison wrote: > > > > > > > I chatted with mmenke@ offline about the lifetime issues I brought > up > > > > > earlier. > > > > > > > > > > > > > > Unfortunately, MessageLoop is destructed after ProfileIOData, so > > there's > > > a > > > > > > > chance we could use-after-free here. > > > > > > > > > > > > > > yzshen@, is there a weaker guarantee here that the code is safe > (i.e. > > > > maybe > > > > > > the > > > > > > > Handler object won't be killed, but its methods won't be called > after > > > some > > > > > > point > > > > > > > X)? > > > > > > > > > > Can we get some kind of notification from ProfileIOData or > ResourceContext > > > > when > > > > > it is destroyed? > > > > I'll be a little tricky. ProfileIOData could probably send a notification > > > > somewhere, but it is a //chrome object and this code is //content. We > could > > > > 1. Add methods to ResourceContext to safely scope bindings. > ResourceContext > > > can > > > > properly destroy them when it is destroyed. > > > > > > We've been having several ResourceContext u-a-f issues in other //content > > > modules too (regardless of mojo usage), one idea that has been proposed > > several > > > times was to let ResourceContext support WeakPtr semantics, while this has > > never > > > been implemented because 1. general skepticism / code health concern around > > > WeakPtr usage, and 2. such unsafe access happens mostly during browser > > shutdown > > > sequence, which is usually not really exploitable (and many other things > could > > > happen during shutdown) so the risk is considered relatively lower > priority... > > > given that I don't feel the lifetime issue needs to be a blocker of this > > change, > > > while it'd be really nice that we have clearer lifetime guarantees with > mojo. > > Thanks for the additional context. Is there a crbug out for the > ResourceContext > > lifetime issues already? Otherwise let's make one. > > > > mmenke@, how does landing this as-is sound to you? > > I haven't really looked at this CL, but it's worth noting this isn't only an > issue during shutdown: It's also a problem when closing the last incognito > window. That's assuming the object is per-profile, and not a global of some sort. > > I'm not a fan of landing known crashy code, even if we have a clear way forward. > In this case, I don't think we even have that.
On 2016/06/24 12:37:12, mmenke wrote: > On 2016/06/24 12:36:40, mmenke wrote: > > On 2016/06/24 12:31:01, csharrison wrote: > > > On 2016/06/24 05:36:24, kinuko wrote: > > > > On 2016/06/23 16:32:55, csharrison wrote: > > > > > On 2016/06/23 16:19:38, yzshen1 wrote: > > > > > > On 2016/06/22 18:39:01, mmenke wrote: > > > > > > > On 2016/06/22 18:11:47, csharrison wrote: > > > > > > > > I chatted with mmenke@ offline about the lifetime issues I brought > > up > > > > > > earlier. > > > > > > > > > > > > > > > > Unfortunately, MessageLoop is destructed after ProfileIOData, so > > > there's > > > > a > > > > > > > > chance we could use-after-free here. > > > > > > > > > > > > > > > > yzshen@, is there a weaker guarantee here that the code is safe > > (i.e. > > > > > maybe > > > > > > > the > > > > > > > > Handler object won't be killed, but its methods won't be called > > after > > > > some > > > > > > > point > > > > > > > > X)? > > > > > > > > > > > > Can we get some kind of notification from ProfileIOData or > > ResourceContext > > > > > when > > > > > > it is destroyed? > > > > > I'll be a little tricky. ProfileIOData could probably send a > notification > > > > > somewhere, but it is a //chrome object and this code is //content. We > > could > > > > > 1. Add methods to ResourceContext to safely scope bindings. > > ResourceContext > > > > can > > > > > properly destroy them when it is destroyed. > > > > > > > > We've been having several ResourceContext u-a-f issues in other //content > > > > modules too (regardless of mojo usage), one idea that has been proposed > > > several > > > > times was to let ResourceContext support WeakPtr semantics, while this has > > > never > > > > been implemented because 1. general skepticism / code health concern > around > > > > WeakPtr usage, and 2. such unsafe access happens mostly during browser > > > shutdown > > > > sequence, which is usually not really exploitable (and many other things > > could > > > > happen during shutdown) so the risk is considered relatively lower > > priority... > > > > given that I don't feel the lifetime issue needs to be a blocker of this > > > change, > > > > while it'd be really nice that we have clearer lifetime guarantees with > > mojo. > > > Thanks for the additional context. Is there a crbug out for the > > ResourceContext > > > lifetime issues already? Otherwise let's make one. > > > > > > mmenke@, how does landing this as-is sound to you? > > > > I haven't really looked at this CL, but it's worth noting this isn't only an > > issue during shutdown: It's also a problem when closing the last incognito > > window. > > That's assuming the object is per-profile, and not a global of some sort. Oops...doesn't matter, I guess - the issue is the RC goes away, not the mojo thingy. So Incognito is an issue, etiher way. > > > > > I'm not a fan of landing known crashy code, even if we have a clear way > forward. > > In this case, I don't think we even have that.
Yes, mmenke@ is right. For incognito mode we have to rely on the connection to the renderer closing before profile destruction. kinuko@, I wouldn't be surprised if the other consumers of ResourceContext might also be burned by this incognito problem. i.e. we have a subset of ResourceContext's that go away before browser shutdown. In that case, do we want to move forward w/ WeakPtr semantics, or were there other plans to solve this general problem?
On 2016/06/24 13:20:16, csharrison wrote: > Yes, mmenke@ is right. For incognito mode we have to rely on the connection to > the renderer closing before profile destruction. > > kinuko@, I wouldn't be surprised if the other consumers of ResourceContext might > also be burned by this incognito problem. i.e. we have a subset of > ResourceContext's that go away before browser shutdown. > > In that case, do we want to move forward w/ WeakPtr semantics, or were there > other plans to solve this general problem? Having everything use a WeakPtr for this really doesn't seem like a great idea. I'm not sufficiently familiar with the relevant content/ architecture to suggest a better one, but think we should do some more thinking about it before rushing ahead with that approach.
On 2016/06/24 14:18:15, mmenke wrote: > On 2016/06/24 13:20:16, csharrison wrote: > > Yes, mmenke@ is right. For incognito mode we have to rely on the connection to > > the renderer closing before profile destruction. > > > > kinuko@, I wouldn't be surprised if the other consumers of ResourceContext > might > > also be burned by this incognito problem. i.e. we have a subset of > > ResourceContext's that go away before browser shutdown. Well yes, incognito could be an issue, I should have mentioned that too. And reg: relevang crbugs: here're some (and there're a lot many that have either been fixed by a local bandaid or wontfix'ed): crbug.com/91398 crbug.com/243840 crbug.com/411736 crbug.com/529896 We should fix this problem, yes, while I didn't feel like we want this change to be totally blocked until we figure out this entire lifetime problem. (I wanted to have the blink::Platform related change landed so I might have phrased things in a bit too optimistic way) > > In that case, do we want to move forward w/ WeakPtr semantics, or were there > > other plans to solve this general problem? > > Having everything use a WeakPtr for this really doesn't seem like a great idea. > I'm not sufficiently familiar with the relevant content/ architecture to suggest > a better one, but think we should do some more thinking about it before rushing > ahead with that approach. I'm not supporting that we should be rushing ahead ahead with WeakPtr solution either, while I'm not fully convinced by an argument like WeakPtr is a plain bad solution for this type of problem-- but there could be other options too. Hm... ok we could probably at least make sure we try to reset / cut down mojo handlers in profile shutdown sequence as much as we do today with RVH -> RPH before landing this, while we can continue to look for a better longer term solution reg: ResourceContext lifetime and ProfileIO shutdown sequence separately. Does that sound sane / am I reading your concern correctly?
On 2016/06/27 at 04:29:40, kinuko wrote: > On 2016/06/24 14:18:15, mmenke wrote: > > On 2016/06/24 13:20:16, csharrison wrote: > > > Yes, mmenke@ is right. For incognito mode we have to rely on the connection to > > > the renderer closing before profile destruction. > > > > > > kinuko@, I wouldn't be surprised if the other consumers of ResourceContext > > might > > > also be burned by this incognito problem. i.e. we have a subset of > > > ResourceContext's that go away before browser shutdown. > > Well yes, incognito could be an issue, I should have mentioned that too. > > And reg: relevang crbugs: here're some (and there're a lot many that have either been fixed by a local bandaid or wontfix'ed): > crbug.com/91398 crbug.com/243840 crbug.com/411736 crbug.com/529896 Thanks for the links! > > We should fix this problem, yes, while I didn't feel like we want this change to be totally blocked until we figure out this entire lifetime problem. (I wanted to have the blink::Platform related change landed so I might have phrased things in a bit too optimistic way) I agree. I would be happy with a bandaid fix if we can find one :) > > > > In that case, do we want to move forward w/ WeakPtr semantics, or were there > > > other plans to solve this general problem? > > > > Having everything use a WeakPtr for this really doesn't seem like a great idea. > > I'm not sufficiently familiar with the relevant content/ architecture to suggest > > a better one, but think we should do some more thinking about it before rushing > > ahead with that approach. > > I'm not supporting that we should be rushing ahead ahead with WeakPtr solution either, while I'm not fully convinced by an argument like WeakPtr is a plain bad solution for this type of problem-- but there could be other options too. > > Hm... ok we could probably at least make sure we try to reset / cut down mojo handlers in profile shutdown sequence as much as we do today with RVH -> RPH before landing this, while we can continue to look for a better longer term solution reg: ResourceContext lifetime and ProfileIO shutdown sequence separately. Does that sound sane / am I reading your concern correctly? Resetting mojo handlers in profile shutdown SGTM if we can figure out a good way to accomplish it. I'm a little nervous that some interfaces will expect to live longer and we'll introduce bugs if we close connections wholesale though. yzshen@, thoughts?
On 2016/06/27 13:11:52, csharrison wrote: > On 2016/06/27 at 04:29:40, kinuko wrote: > > On 2016/06/24 14:18:15, mmenke wrote: > > > On 2016/06/24 13:20:16, csharrison wrote: > > > > Yes, mmenke@ is right. For incognito mode we have to rely on the > connection to > > > > the renderer closing before profile destruction. > > > > > > > > kinuko@, I wouldn't be surprised if the other consumers of ResourceContext > > > might > > > > also be burned by this incognito problem. i.e. we have a subset of > > > > ResourceContext's that go away before browser shutdown. > > > > Well yes, incognito could be an issue, I should have mentioned that too. > > > > And reg: relevang crbugs: here're some (and there're a lot many that have > either been fixed by a local bandaid or wontfix'ed): > > crbug.com/91398 crbug.com/243840 crbug.com/411736 crbug.com/529896 > Thanks for the links! > > > > We should fix this problem, yes, while I didn't feel like we want this change > to be totally blocked until we figure out this entire lifetime problem. (I > wanted to have the blink::Platform related change landed so I might have phrased > things in a bit too optimistic way) > I agree. I would be happy with a bandaid fix if we can find one :) > > > > > > In that case, do we want to move forward w/ WeakPtr semantics, or were > there > > > > other plans to solve this general problem? > > > > > > Having everything use a WeakPtr for this really doesn't seem like a great > idea. > > > I'm not sufficiently familiar with the relevant content/ architecture to > suggest > > > a better one, but think we should do some more thinking about it before > rushing > > > ahead with that approach. > > > > I'm not supporting that we should be rushing ahead ahead with WeakPtr solution > either, while I'm not fully convinced by an argument like WeakPtr is a plain bad > solution for this type of problem-- but there could be other options too. > > > > Hm... ok we could probably at least make sure we try to reset / cut down mojo > handlers in profile shutdown sequence as much as we do today with RVH -> RPH > before landing this, while we can continue to look for a better longer term > solution reg: ResourceContext lifetime and ProfileIO shutdown sequence > separately. Does that sound sane / am I reading your concern correctly? > Resetting mojo handlers in profile shutdown SGTM if we can figure out a good way > to accomplish it. I'm a little nervous that some interfaces will expect to live > longer and we'll introduce bugs if we close connections wholesale though. > yzshen@, thoughts? I am not sure I have understood the idea yet: what do you mean by closing connections wholesale? Would you please explain a little more? Do you mean terminating all message pipes at the mojo system layer? That basically means we tearing down entire IPC support before destructing ProfileIO. I don't know for sure, but that probably won't work well. It seems to me we still have to have some kind of destruction observer allowing users of ProfileData to observe its destruction and do whatever cleanup they want.
On 2016/06/28 20:40:27, yzshen1 wrote: > On 2016/06/27 13:11:52, csharrison wrote: > > On 2016/06/27 at 04:29:40, kinuko wrote: > > > On 2016/06/24 14:18:15, mmenke wrote: > > > > On 2016/06/24 13:20:16, csharrison wrote: > > > > > Yes, mmenke@ is right. For incognito mode we have to rely on the > > connection to > > > > > the renderer closing before profile destruction. > > > > > > > > > > kinuko@, I wouldn't be surprised if the other consumers of > ResourceContext > > > > might > > > > > also be burned by this incognito problem. i.e. we have a subset of > > > > > ResourceContext's that go away before browser shutdown. > > > > > > Well yes, incognito could be an issue, I should have mentioned that too. > > > > > > And reg: relevang crbugs: here're some (and there're a lot many that have > > either been fixed by a local bandaid or wontfix'ed): > > > crbug.com/91398 crbug.com/243840 crbug.com/411736 crbug.com/529896 > > Thanks for the links! > > > > > > We should fix this problem, yes, while I didn't feel like we want this > change > > to be totally blocked until we figure out this entire lifetime problem. (I > > wanted to have the blink::Platform related change landed so I might have > phrased > > things in a bit too optimistic way) > > I agree. I would be happy with a bandaid fix if we can find one :) > > > > > > > > In that case, do we want to move forward w/ WeakPtr semantics, or were > > there > > > > > other plans to solve this general problem? > > > > > > > > Having everything use a WeakPtr for this really doesn't seem like a great > > idea. > > > > I'm not sufficiently familiar with the relevant content/ architecture to > > suggest > > > > a better one, but think we should do some more thinking about it before > > rushing > > > > ahead with that approach. > > > > > > I'm not supporting that we should be rushing ahead ahead with WeakPtr > solution > > either, while I'm not fully convinced by an argument like WeakPtr is a plain > bad > > solution for this type of problem-- but there could be other options too. > > > > > > Hm... ok we could probably at least make sure we try to reset / cut down > mojo > > handlers in profile shutdown sequence as much as we do today with RVH -> RPH > > before landing this, while we can continue to look for a better longer term > > solution reg: ResourceContext lifetime and ProfileIO shutdown sequence > > separately. Does that sound sane / am I reading your concern correctly? > > Resetting mojo handlers in profile shutdown SGTM if we can figure out a good > way > > to accomplish it. I'm a little nervous that some interfaces will expect to > live > > longer and we'll introduce bugs if we close connections wholesale though. > > yzshen@, thoughts? > > I am not sure I have understood the idea yet: what do you mean by closing > connections wholesale? Would you please explain a little more? Do you mean > terminating all message pipes at the mojo system layer? That basically means we > tearing down entire IPC support before destructing ProfileIO. I don't know for > sure, but that probably won't work well. > > It seems to me we still have to have some kind of destruction observer allowing > users of ProfileData to observe its destruction and do whatever cleanup they > want. Note that this isn't an observer of ProfileData. This is a content class, and doesn't even know about the class. The chrome/content divide seems to be what makes this a pain to do. If everything were on one side of the divide, this could just be owned by ProfileIOData, and browser/loader code could just get the pointer from ProfileIOData directly.
On 2016/06/28 at 20:40:27, yzshen wrote: > On 2016/06/27 13:11:52, csharrison wrote: > > On 2016/06/27 at 04:29:40, kinuko wrote: > > > On 2016/06/24 14:18:15, mmenke wrote: > > > > On 2016/06/24 13:20:16, csharrison wrote: > > > > > Yes, mmenke@ is right. For incognito mode we have to rely on the > > connection to > > > > > the renderer closing before profile destruction. > > > > > > > > > > kinuko@, I wouldn't be surprised if the other consumers of ResourceContext > > > > might > > > > > also be burned by this incognito problem. i.e. we have a subset of > > > > > ResourceContext's that go away before browser shutdown. > > > > > > Well yes, incognito could be an issue, I should have mentioned that too. > > > > > > And reg: relevang crbugs: here're some (and there're a lot many that have > > either been fixed by a local bandaid or wontfix'ed): > > > crbug.com/91398 crbug.com/243840 crbug.com/411736 crbug.com/529896 > > Thanks for the links! > > > > > > We should fix this problem, yes, while I didn't feel like we want this change > > to be totally blocked until we figure out this entire lifetime problem. (I > > wanted to have the blink::Platform related change landed so I might have phrased > > things in a bit too optimistic way) > > I agree. I would be happy with a bandaid fix if we can find one :) > > > > > > > > In that case, do we want to move forward w/ WeakPtr semantics, or were > > there > > > > > other plans to solve this general problem? > > > > > > > > Having everything use a WeakPtr for this really doesn't seem like a great > > idea. > > > > I'm not sufficiently familiar with the relevant content/ architecture to > > suggest > > > > a better one, but think we should do some more thinking about it before > > rushing > > > > ahead with that approach. > > > > > > I'm not supporting that we should be rushing ahead ahead with WeakPtr solution > > either, while I'm not fully convinced by an argument like WeakPtr is a plain bad > > solution for this type of problem-- but there could be other options too. > > > > > > Hm... ok we could probably at least make sure we try to reset / cut down mojo > > handlers in profile shutdown sequence as much as we do today with RVH -> RPH > > before landing this, while we can continue to look for a better longer term > > solution reg: ResourceContext lifetime and ProfileIO shutdown sequence > > separately. Does that sound sane / am I reading your concern correctly? > > Resetting mojo handlers in profile shutdown SGTM if we can figure out a good way > > to accomplish it. I'm a little nervous that some interfaces will expect to live > > longer and we'll introduce bugs if we close connections wholesale though. > > yzshen@, thoughts? > > I am not sure I have understood the idea yet: what do you mean by closing connections wholesale? Would you please explain a little more? Do you mean terminating all message pipes at the mojo system layer? That basically means we tearing down entire IPC support before destructing ProfileIO. I don't know for sure, but that probably won't work well. > > It seems to me we still have to have some kind of destruction observer allowing users of ProfileData to observe its destruction and do whatever cleanup they want. I think kinuko@ and I were thinking of something more specific to mojo than a general destruction observer of ProfileIOData. I'm not quite sure what it would look like, as obviously terminating all message pipes could not work. Maybe there could be a service registry for profile scoped pipes? I think your proposal of a ProfileIO destruction observer (in //content) would solve the shutdown cleanup described by kinuko@ in #61.
On 2016/06/28 20:47:30, csharrison wrote: > On 2016/06/28 at 20:40:27, yzshen wrote: > > On 2016/06/27 13:11:52, csharrison wrote: > > > On 2016/06/27 at 04:29:40, kinuko wrote: > > > > On 2016/06/24 14:18:15, mmenke wrote: > > > > > On 2016/06/24 13:20:16, csharrison wrote: > > > > > > Yes, mmenke@ is right. For incognito mode we have to rely on the > > > connection to > > > > > > the renderer closing before profile destruction. > > > > > > > > > > > > kinuko@, I wouldn't be surprised if the other consumers of > ResourceContext > > > > > might > > > > > > also be burned by this incognito problem. i.e. we have a subset of > > > > > > ResourceContext's that go away before browser shutdown. > > > > > > > > Well yes, incognito could be an issue, I should have mentioned that too. > > > > > > > > And reg: relevang crbugs: here're some (and there're a lot many that have > > > either been fixed by a local bandaid or wontfix'ed): > > > > crbug.com/91398 crbug.com/243840 crbug.com/411736 crbug.com/529896 > > > Thanks for the links! > > > > > > > > We should fix this problem, yes, while I didn't feel like we want this > change > > > to be totally blocked until we figure out this entire lifetime problem. (I > > > wanted to have the blink::Platform related change landed so I might have > phrased > > > things in a bit too optimistic way) > > > I agree. I would be happy with a bandaid fix if we can find one :) > > > > > > > > > > In that case, do we want to move forward w/ WeakPtr semantics, or were > > > there > > > > > > other plans to solve this general problem? > > > > > > > > > > Having everything use a WeakPtr for this really doesn't seem like a > great > > > idea. > > > > > I'm not sufficiently familiar with the relevant content/ architecture to > > > suggest > > > > > a better one, but think we should do some more thinking about it before > > > rushing > > > > > ahead with that approach. > > > > > > > > I'm not supporting that we should be rushing ahead ahead with WeakPtr > solution > > > either, while I'm not fully convinced by an argument like WeakPtr is a plain > bad > > > solution for this type of problem-- but there could be other options too. > > > > > > > > Hm... ok we could probably at least make sure we try to reset / cut down > mojo > > > handlers in profile shutdown sequence as much as we do today with RVH -> RPH > > > before landing this, while we can continue to look for a better longer term > > > solution reg: ResourceContext lifetime and ProfileIO shutdown sequence > > > separately. Does that sound sane / am I reading your concern correctly? > > > Resetting mojo handlers in profile shutdown SGTM if we can figure out a good > way > > > to accomplish it. I'm a little nervous that some interfaces will expect to > live > > > longer and we'll introduce bugs if we close connections wholesale though. > > > yzshen@, thoughts? > > > > I am not sure I have understood the idea yet: what do you mean by closing > connections wholesale? Would you please explain a little more? Do you mean > terminating all message pipes at the mojo system layer? That basically means we > tearing down entire IPC support before destructing ProfileIO. I don't know for > sure, but that probably won't work well. > > > > It seems to me we still have to have some kind of destruction observer > allowing users of ProfileData to observe its destruction and do whatever cleanup > they want. > I think kinuko@ and I were thinking of something more specific to mojo than a > general destruction observer of ProfileIOData. I'm not quite sure what it would > look like, as obviously terminating all message pipes could not work. Maybe > there could be a service registry for profile scoped pipes? Interfaces (i.e., message pipes) established by a service registry are not owned by the registry. Moreover, new interfaces can be created and passed by interfaces, they don't have to be established via any service registry. So I think this direction probably won't work. :/ > > I think your proposal of a ProfileIO destruction observer (in //content) would > solve the shutdown cleanup described by kinuko@ in #61. > Note that this isn't an observer of ProfileData. This is a content class, and > doesn't even know about the class. The chrome/content divide seems to be what > makes this a pain to do. If everything were on one side of the divide, this > could just be owned by ProfileIOData, and browser/loader code could just get the > pointer from ProfileIOData directly. Acknowledged. I don't think I know this code well enough to give specific/useful suggestions. :) I just guess we may be able to find a reasonable way to expose that to the content side.
On 2016/06/30 16:22:56, yzshen1 wrote: > On 2016/06/28 20:47:30, csharrison wrote: > > On 2016/06/28 at 20:40:27, yzshen wrote: > > > On 2016/06/27 13:11:52, csharrison wrote: > > > > On 2016/06/27 at 04:29:40, kinuko wrote: > > > > > On 2016/06/24 14:18:15, mmenke wrote: > > > > > > On 2016/06/24 13:20:16, csharrison wrote: > > > > > > > Yes, mmenke@ is right. For incognito mode we have to rely on the > > > > connection to > > > > > > > the renderer closing before profile destruction. > > > > > > > > > > > > > > kinuko@, I wouldn't be surprised if the other consumers of > > ResourceContext > > > > > > might > > > > > > > also be burned by this incognito problem. i.e. we have a subset of > > > > > > > ResourceContext's that go away before browser shutdown. > > > > > > > > > > Well yes, incognito could be an issue, I should have mentioned that too. > > > > > > > > > > And reg: relevang crbugs: here're some (and there're a lot many that > have > > > > either been fixed by a local bandaid or wontfix'ed): > > > > > crbug.com/91398 crbug.com/243840 crbug.com/411736 crbug.com/529896 > > > > Thanks for the links! > > > > > > > > > > We should fix this problem, yes, while I didn't feel like we want this > > change > > > > to be totally blocked until we figure out this entire lifetime problem. (I > > > > wanted to have the blink::Platform related change landed so I might have > > phrased > > > > things in a bit too optimistic way) > > > > I agree. I would be happy with a bandaid fix if we can find one :) > > > > > > > > > > > > In that case, do we want to move forward w/ WeakPtr semantics, or > were > > > > there > > > > > > > other plans to solve this general problem? > > > > > > > > > > > > Having everything use a WeakPtr for this really doesn't seem like a > > great > > > > idea. > > > > > > I'm not sufficiently familiar with the relevant content/ architecture > to > > > > suggest > > > > > > a better one, but think we should do some more thinking about it > before > > > > rushing > > > > > > ahead with that approach. > > > > > > > > > > I'm not supporting that we should be rushing ahead ahead with WeakPtr > > solution > > > > either, while I'm not fully convinced by an argument like WeakPtr is a > plain > > bad > > > > solution for this type of problem-- but there could be other options too. > > > > > > > > > > Hm... ok we could probably at least make sure we try to reset / cut down > > mojo > > > > handlers in profile shutdown sequence as much as we do today with RVH -> > RPH > > > > before landing this, while we can continue to look for a better longer > term > > > > solution reg: ResourceContext lifetime and ProfileIO shutdown sequence > > > > separately. Does that sound sane / am I reading your concern correctly? > > > > Resetting mojo handlers in profile shutdown SGTM if we can figure out a > good > > way > > > > to accomplish it. I'm a little nervous that some interfaces will expect to > > live > > > > longer and we'll introduce bugs if we close connections wholesale though. > > > > yzshen@, thoughts? > > > > > > I am not sure I have understood the idea yet: what do you mean by closing > > connections wholesale? Would you please explain a little more? Do you mean > > terminating all message pipes at the mojo system layer? That basically means > we > > tearing down entire IPC support before destructing ProfileIO. I don't know for > > sure, but that probably won't work well. > > > > > > It seems to me we still have to have some kind of destruction observer > > allowing users of ProfileData to observe its destruction and do whatever > cleanup > > they want. > > > I think kinuko@ and I were thinking of something more specific to mojo than a > > general destruction observer of ProfileIOData. I'm not quite sure what it > would > > look like, as obviously terminating all message pipes could not work. Maybe > > there could be a service registry for profile scoped pipes? > > Interfaces (i.e., message pipes) established by a service registry are not owned > by the registry. Moreover, new interfaces can be created and passed by > interfaces, they don't have to be established via any service registry. So I > think this direction probably won't work. :/ > > > > > I think your proposal of a ProfileIO destruction observer (in //content) would > > solve the shutdown cleanup described by kinuko@ in #61. > > > Note that this isn't an observer of ProfileData. This is a content class, and > > doesn't even know about the class. The chrome/content divide seems to be what > > makes this a pain to do. If everything were on one side of the divide, this > > could just be owned by ProfileIOData, and browser/loader code could just get > the > > pointer from ProfileIOData directly. > > Acknowledged. I don't think I know this code well enough to give specific/useful > suggestions. :) I just guess we may be able to find a reasonable way to expose > that to the content side. (I want to study the code around this a little closer during no-meeting weeks, I want to have a better view about how new IPC code should be plumbed in general but haven't had time to do that)
Thanks, everyone for being patient and helping out with this :)
On 2016/07/01 12:06:28, Charlie Harrison wrote: > Thanks, everyone for being patient and helping out with this :) Removing myself from this CL. Feel free to add me back if you pick it up again.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org |
