Removing dependency of c/b/loader on content/browser/host_zoom_map_impl.(cc|h) |
|||||||
Issue descriptionRemoving dependency on content/browser/host_zoom_map_impl.(cc|h) Another discussion topic in the vein of https://crbug.com/607741 . In this case, the only use in c/b/loader is AsyncResourceHandler::OnResponseStarted(). The browser keeps a hierarchy of maps that determine a zoom level for scheme/host/url (or something). When an async request starts, AsyncResourceHandler retrieves the zoom level from c/b/host_zoom_map_impl.cc and sends a message to the associated renderer, where it just saves it off: host_zoom_levels_[url] = zoom_level; This feels like pretty content/chrome-specific logic, so that it probably doesn't belong in network, especially since the resource request doesn't actually need to use the zoom level itself. We already delegate back to ChromeResourceDispatcherHostDelegate::OnResponseStarted() here, but in this case, we don't want to go all the way back to chrome, we just want the content HostZoomMapImpl implementation. So, my first thought is that want an analogous ContentResourceDispatcherHostDelegate that does the content-specific work before sending on to the existing ChromeResourceDispatcherHostDelegate. Does that seem reasonable and something we'll need elsewhere? Another way to go would be have content/browser push that data into the network, and have it push that back, similar to what we do now. That seems inefficient. We could also have content/browser push it directly to the renderer. At the moment from looking at render_frame_impl.cc it looks like we try to keep a fairly minimal set of URL->zoom entries in the map. We could consider doing something like keeping the zoom levels in a shared memory structure that the browser writes and the renderers all read from so there's nothing to negotiate and transfer explicitly, but that's a different sort of refactoring.
,
May 6 2016
,
May 13 2016
(Mostly making notes for myself, but if anyone else understands this code, pleeeeease let me know if you have any thoughts :) I tried an async mojo request here https://codereview.chromium.org/1964273002/. That almost works, but fails one test. Fortunately there was a single test IFrameZoomBrowserTest.RedirectToPageWithSubframeZoomsCorrectly that caught a problem (I'm not sure what to do about the problem now that I sort of understand it.) In that CL, I tried having RenderFrameImpl request the zoom level when it calls frame_->load() to navigate at the top level, which seemed to be where most top level requests went through. I think the problem is that redirect navigations don't go through RenderFrameImpl and instead go directly from ScheduledRedirect::fire() to ResourceDispatcher. (I don't exactly understand how that ends up navigating the top level frame yet) In the current code, resource_dispatcher requests the resource and sends a ResourceHostMsg_RequestResource (sent from content/child), which is handled by async_resource_request.cc, which sends back a ViewMsg_SetZoomLevelForLoadingURL. This is handled by render_view_impl (in content/renderer). So, the problem with the mojo attempt is that I'm not sure how/where to add the request for the zoom level so that content/child can do the request (so that it works for this redirect case) but have it set data into render_view_impl (or render_frame_impl). I guess I need to figure out how the redirect navigation ends up getting back to that frame now.
,
May 13 2016
Aha, possibly willSendRequest is the right place to do the request instead.
,
May 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/68c6f2ce16d9807b5cb82679099c82c40f39e911 commit 68c6f2ce16d9807b5cb82679099c82c40f39e911 Author: scottmg <scottmg@chromium.org> Date: Wed May 18 21:10:34 2016 Add FrameHost mojo service Adds new frame-level service with one initial method to handle host zoom level. This moves zoom level supply from async_resource_handler.cc to being a request made when render_frame_impl handles a willSendRequest. The goal of this change is to remove the dependency of content/browser/loader on the rest of content/browser in particular here, removing the use of c/b/host_zoom_map_impl.h in content/browser/loader/async_resource_handler.cc. BUG=598073, 609607 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/1964273002 Cr-Commit-Position: refs/heads/master@{#394547} [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/build/android/pylib/gtest/filter/content_browsertests_disabled [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/browser/host_zoom_map_impl.h [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/browser/loader/DEPS [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/browser/loader/async_resource_handler.cc [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/browser/loader/async_revalidation_manager_unittest.cc [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/browser/loader/resource_dispatcher_host_unittest.cc [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/browser/loader/resource_message_filter.cc [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/browser/loader/resource_message_filter.h [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/common/BUILD.gn [add] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/common/frame_host.mojom [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/common/view_messages.h [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/content_common_mojo_bindings.gyp [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/renderer/render_frame_impl.cc [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/renderer/render_frame_impl.h [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/renderer/render_frame_impl_browsertest.cc [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/renderer/render_view_browsertest.cc [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/renderer/render_view_impl.cc [modify] https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911/content/renderer/render_view_impl.h
,
May 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b commit 34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b Author: scottmg <scottmg@chromium.org> Date: Wed May 25 01:04:56 2016 Revert of Add FrameHost mojo service (patchset #23 id:490001 of https://codereview.chromium.org/1964273002/ ) Reason for revert: Async request for zoom level doesn't work in all cases https://crbug.com/614348 https://crbug.com/613979 . I thought https://www.chromium.org/developers/design-documents/mojo/chrome-ipc-to-mojo-ipc-cheet-sheet#TOC-Threading-Model implied the ordering would be correct, but it seems that was too hopeful. Original issue's description: > Add FrameHost mojo service > > Adds new frame-level service with one initial method to handle host zoom > level. > > This moves zoom level supply from async_resource_handler.cc to being a > request made when render_frame_impl handles a willSendRequest. > > The goal of this change is to remove the dependency of > content/browser/loader on the rest of content/browser in particular > here, removing the use of c/b/host_zoom_map_impl.h in > content/browser/loader/async_resource_handler.cc. > > BUG=598073, 609607 > CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation > > Committed: https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911 > Cr-Commit-Position: refs/heads/master@{#394547} TBR=dcheng@chromium.org,ben@chromium.org,jam@chromium.org,nasko@chromium.org,wjmaclean@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=598073, 609607 , 614348 , 613979 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2007203002 Cr-Commit-Position: refs/heads/master@{#395761} [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/build/android/pylib/gtest/filter/content_browsertests_disabled [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/browser/host_zoom_map_impl.h [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/browser/loader/DEPS [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/browser/loader/async_resource_handler.cc [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/browser/loader/async_revalidation_manager_unittest.cc [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/browser/loader/resource_dispatcher_host_unittest.cc [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/browser/loader/resource_message_filter.cc [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/browser/loader/resource_message_filter.h [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/common/BUILD.gn [delete] https://crrev.com/bcd06ba900e67c54ffffb3f421c6b0d1bcd8ef31/content/common/frame_host.mojom [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/common/view_messages.h [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/content_common_mojo_bindings.gyp [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/renderer/render_frame_impl.cc [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/renderer/render_frame_impl.h [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/renderer/render_frame_impl_browsertest.cc [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/renderer/render_view_browsertest.cc [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/renderer/render_view_impl.cc [modify] https://crrev.com/34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b/content/renderer/render_view_impl.h
,
May 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b43ea3087da4e351f0bf94481c9810738c3ae938 commit b43ea3087da4e351f0bf94481c9810738c3ae938 Author: Scott Graham <scottmg@chromium.org> Date: Fri May 27 22:40:35 2016 Revert of Add FrameHost mojo service (patchset #23 id:490001 of https://codereview.chromium.org/1964273002/ ) Reason for revert: Async request for zoom level doesn't work in all cases https://crbug.com/614348 https://crbug.com/613979 . I thought https://www.chromium.org/developers/design-documents/mojo/chrome-ipc-to-mojo-ipc-cheet-sheet#TOC-Threading-Model implied the ordering would be correct, but it seems that was too hopeful. Original issue's description: > Add FrameHost mojo service > > Adds new frame-level service with one initial method to handle host zoom > level. > > This moves zoom level supply from async_resource_handler.cc to being a > request made when render_frame_impl handles a willSendRequest. > > The goal of this change is to remove the dependency of > content/browser/loader on the rest of content/browser in particular > here, removing the use of c/b/host_zoom_map_impl.h in > content/browser/loader/async_resource_handler.cc. > > BUG=598073, 609607 > CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation > > Committed: https://crrev.com/68c6f2ce16d9807b5cb82679099c82c40f39e911 > Cr-Commit-Position: refs/heads/master@{#394547} TBR=dcheng@chromium.org,ben@chromium.org,jam@chromium.org,nasko@chromium.org,wjmaclean@chromium.org BUG=598073, 609607 , 614348 , 613979 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2007203002 Cr-Commit-Position: refs/heads/master@{#395761} (cherry picked from commit 34ce95dd9c2ca3b90fc87169f1e4ed266e7bb94b) Review URL: https://codereview.chromium.org/2020633003 . Cr-Commit-Position: refs/branch-heads/2743@{#111} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/build/android/pylib/gtest/filter/content_browsertests_disabled [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/browser/host_zoom_map_impl.h [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/browser/loader/DEPS [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/browser/loader/async_resource_handler.cc [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/browser/loader/async_revalidation_manager_unittest.cc [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/browser/loader/resource_dispatcher_host_unittest.cc [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/browser/loader/resource_message_filter.cc [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/browser/loader/resource_message_filter.h [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/common/BUILD.gn [delete] https://crrev.com/d2634566be6ed8b068e1ff5414525627a8adb7fc/content/common/frame_host.mojom [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/common/view_messages.h [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/content_common_mojo_bindings.gyp [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/renderer/render_frame_impl.cc [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/renderer/render_frame_impl.h [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/renderer/render_frame_impl_browsertest.cc [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/renderer/render_view_browsertest.cc [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/renderer/render_view_impl.cc [modify] https://crrev.com/b43ea3087da4e351f0bf94481c9810738c3ae938/content/renderer/render_view_impl.h
,
Jun 7 2016
Just to summarize the problem here, per bug 613979 (and other bugs duped into it) the ordering of getting the response for the host zoom level and resource response was broken in the CL above. In the all-Chrome IPC version content/browser/loader/async_resource_handler.cc first sends ViewMsg_SetZoomLevelForLoadingURL when it's necessary, then sends ResourceMsg_ReceivedResponse on the same channel. So the renderer can be sure that when it's handling ResourceMsg_ReceivedResponse, it already has the right zoom level in the renderer-side map. In the mixed mojo/Chrome version, I moved the zoom level request to RenderFrameImpl::willSendRequest. That goes from the main renderer thread, is handled in RenderFrameHostImpl on the UI thread and responds back to the renderer main thread. But the resource request goes through AsyncResourceHandler in the browser (which is on the IO thread), and responds with ResourceMsg_ReceivedResponse directly from there. So, (some of?) the mistakes I made here as I understand them: - there's no ordering guarantee for the new mojo message since it's just handled on UI vs. the chrome ipc being handled on IO. - (and maybe per Yuzhu?) there needs to be some association made to actually get the ordering guarantee even if both the mojo and chrome ipc messages were both on only the IO thread in the browser (I had been thinking there was another problem that during handling the request, the browser posts IO->UI->IO before responding, but that shouldn't be relevant because we want the resource request to be *after* the zoom level request, which would still be maintained if the mojo message was also on the IO thread.) I admit I didn't investigate exactly what-was-on-which-thread enough when first doing this, but now that I look at https://www.chromium.org/developers/design-documents/mojo/chrome-ipc-to-mojo-ipc-cheet-sheet#TOC-Threading-Model "Making a method call doesn't involve a thread hop, but receiving it on a thread other than the IO thread does involve a thread hop." I let myself be slightly misled by that, vaguely thinking that since the mojo message would also be delivered via the IO thread, things would work out fine. But I guess the problem then is that the mojo-message-receiver gets it on the IO thread, and then posts it to the UI thread to the handler, which means that chrome ipc code doing the work only-on-IO for the resource response gets out of sequence with the work that got posted to the UI thread. Does that seem like a reasonable explanation?
,
Jun 7 2016
RE #8:
"""But I guess the problem then is that the mojo-message-receiver gets it on the IO thread, and then posts it to the UI thread to the handler, which means that chrome ipc code doing the work only-on-IO for the resource response gets out of sequence with the work that got posted to the UI thread."""
One thing that may be worth noting: even if a chrome ipc message and a mojo message both go from the IO thread to the UI thread, they can still get out of sequence. The way mojo delivers messages is that it posts a task to signal pipe readable, and then the receiver reads available messages from the pipe. It doesn't post a task for each messages from IO to UI thread.
The ongoing work of rockot@ will address this, if the mojo interface is "associated" with the underlying mojo interface of ChannelMojo that is used to transfer chrome IPC messages. ("Associated interface" is explained here: http://www.chromium.org/developers/design-documents/mojo/associated-interfaces)
,
Jun 7 2016
Thanks for the in-depth explanation, that's helpful in understanding this. So it seems one solution is that we can create a per-render-frame interface that is on the IO thread. Perhaps we should add both FrameHost & FrameHostIO mojo interfaces?
,
Jun 8 2016
Putting it on the IO thread in the browser seems reasonable to me, but I'm not sure how to do that. My simple attempt (passing the IO thread into request Bind()) doesn't work (during request Bind, Watcher DCHECKs that it's on the wrong thread). https://codereview.chromium.org/2047573006/diff2/1:20001/content/browser/frame_host/render_frame_host_impl.cc?context=10&column_width=80&tab_spaces=10 I'm a little lost in Factorys and Connectors and Routers and Watchers, but I guess this is because the ServiceRegistry on RenderFrameHost is only suitable for services on the UI thread, is that right? So we'd need to add a ServiceRegistry that's serviced on the IO thread? Or is there a simpler way to do that?
,
Jun 8 2016
Oh, of course right as I send, I see there's some more default parameters to AddService() for the task runner I didn't notice. More debugging...
,
Jun 8 2016
ServiceRegistry is going away, so for now the recommended approach is to manually bind the request on the IO thread. The patchset above won't work since the Bind() call has to be done on the IO thread. Here's an example: https://cs.chromium.org/chromium/src/content/browser/background_sync/background_sync_context.cc?rcl=0&l=45
,
Jun 8 2016
Thanks I'll try it that way. FTR, passing in an IO task runner to ServiceRegistry::AddService "works" in that things run on the expected threads, but the ServiceRegistry::GetInterface call is serviced on a task on the UI thread on the first message (which then posts to run the IO thread), so the messages still can get out of order.
,
Jun 8 2016
Actually, I'm not sure if what you linked to helps unconfuse me :). BackgroundSyncContext::CreateService is still using the per-process ServiceRegistry? https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_process_host_impl.cc?rcl=0&l=1080 and the ServiceRegistry::GetInterface runs on UI.
,
Jun 8 2016
The example that John provided in #13: BackgroundSyncContext::CreateService is called on the UI thread, but it forwards the InterfaceRequest it receives to the IO thread and binds it there, all messages for that interface will be dispatched to the IO thread.
,
Jun 8 2016
I'm also not convinced that I'm getting the right ordering between these two messages that are sent from renderer main -> IO. The chrome ipc one is sent after the mojo ipc call in the renderer, but chrome is serviced before mojo on the browser side. (I think this is only on the first request where the interface has to be bound on the browser side; subsequent requests seem to be ordered correctly. So I think it might only be that I need to resolve my confusion over how to register properly on the io thread.)
,
Jun 8 2016
Thanks Yuzhu. Doesn't that mean it won't work though? When I make the first call from the renderer, it goes through ServiceRegistry::GetInterface which runs on the UI thread, forwards to IO, binds the interface (on IO), which means it's out of order with the first chrome ipc that was directly on the IO thread. I think subsequent requests are OK though. Should I be "prebinding" it somehow to avoid that maybe?
,
Jun 8 2016
If we "prebind" the thing on the IO thread, then I think we can preserve IPC and mojo message order on the IO thread (given that the mojo interface doesn't use sync method calls). But that really depends on undocumented implementation details. It seems either we should use Ken's work when it is ready; or we just accept the fact that messages from IPC/mojo can be out of order and handle that correctly. WDYT?
,
Jun 8 2016
IMO it would be unfortunate to not have ordering to the IO thread. What undocumented implementation details do you mean Yuzhu? I think the tricky thing will be to ensure that even with prebinding, we're guaranteed that the binding early enough.
,
Jun 8 2016
hmm, actually I don't see how we can do prebinding without hops to the UI thread as mentioned above. Yuzhu suggested one possible solution is sync call, but I don't think we want to do a sync call per RenderFrame (we avoid sync calls in general). Maybe we can have this zoom level interface be per-process, and we can prebind it at process startup. But that only works for process wide interfaces and won't solve the per-frame interfaces. Maybe we can try that? (just thinking out loud)
,
Jun 8 2016
I think the only reason we moved zoomlevel from process to frame was to avoid passing routing_ids around, but I think it'd be OK if it was per-process other than that. I'm still not sure quite how we'd prebind though in the renderer to guarantee ordering without a sync call. I guess something like semi-sync like sending a message that the render process has to wait on before continuing (at least for getting as far as creating a frame?) Does seem a bit unfortunate/subtle to move between process<->frame for this reason. Is it worthwhile to pursue having the bind happen directly on IO? Maybe that's a dumb question, I don't fully understand all that machinery.
,
Jun 9 2016
"What undocumented implementation details do you mean Yuzhu?" I mean, it depends on the assumption that a Mojo message, after it is read off the OS pipe, is delivered to the user code on the IO thread before returning to the message loop (i.e., not cached at the mojo system or binding layer and dispatched later). This is not guaranteed explicitly, and it is not always the case if sync call is involved.
,
Jun 9 2016
@scottmg: ah thanks for the reminder. We really don't want to have routing IDs in mojo interfaces since that's a step backwards. @yzshen: thanks for the extra info. can you expand on how sync calls would break the ordering between mojo & chrome ipc? I know that there's buffering of incoming messages when making a sync call. however the browser process wouldn't be making any sync calls?
,
Jun 9 2016
In response to #24: Right. We won't make sync calls from the browser process (not to mention on its IO thread, which leads to deadlock immediately). I mentioned sync call just as an example that message may be buffered, didn't mean it is a concern in this case. Another example is binding interface to a task runner different from the default task runner of the same thread. (Thinking out loud...) If we do want to preserve order, it seems reasonable to use the feature of associating mojo interfaces with ChannelMojo (once it is ready). The purpose of that work is to solve problem like this. It guarantees FIFOness, which should solve the pre-bind problem, too. Imagine we have an AssociatedFooRequest needs to jump from IO->UI->IO. The renderer can: - send AssociatedFooRequest; - no need to wait for the request to reach the browser, make a call using the corresponding AssociatedFooPtr (resulting in Mojo_Msg_Foo_1); - go ahead and send IPC_Msg_Bar_2 In the browser, as long as AssociatedFooRequest is not bound to process Mojo_Msg_Foo_1, IPC_Msg_Bar_2 will stay buffered. [1] That way, we don't have to hack things to work. And it doesn't implicitly rely on undocumented behavior. WDYT? Thanks! +CC Ken as well. ======================================= [1] This has a performance cost. That is one reason why it seems nice to consider solutions that don't rely on message ordering.
,
Jun 9 2016
(Accidentally removed myself; added back.)
,
Jun 10 2016
,
Jul 27 2016
btw Ben will soon update https://codereview.chromium.org/2157143002/ to have an InterfaceRegistry that's setup on the main thread before a renderer is launched, and then the registry will be used on the IO thread. This should fix the race conditions which caused this revert.
,
Aug 1 2016
ok that above change is now part of https://codereview.chromium.org/2183703005/
,
Aug 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d4423d511f18f4b0d2e94cf17bf6043bee449c55 commit d4423d511f18f4b0d2e94cf17bf6043bee449c55 Author: scottmg <scottmg@chromium.org> Date: Mon Aug 08 20:11:48 2016 Remove dependency of c/b/loader on c/b/host_zoom_map_impl.h Contrary to previous attempt at: https://chromium.googlesource.com/chromium/src.git/+/68c6f2ce16d9807b5cb82679099c82c40f39e911 RenderFrameMessageFilter is on the IO thread, and is a BrowserAssociatedInterface, so will maintain the ordering of the resource request and the zoom level request. See also https://codereview.chromium.org/2167513003. BUG= 609607 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2211713002 Cr-Commit-Position: refs/heads/master@{#410417} [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/build/android/pylib/gtest/filter/content_browsertests_disabled [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/browser/frame_host/render_frame_message_filter.cc [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/browser/frame_host/render_frame_message_filter.h [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/browser/host_zoom_map_impl.h [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/browser/loader/DEPS [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/browser/loader/async_resource_handler.cc [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/browser/loader/async_resource_handler_unittest.cc [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/browser/loader/async_revalidation_manager_unittest.cc [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/browser/loader/resource_dispatcher_host_unittest.cc [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/browser/loader/resource_message_filter.cc [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/browser/loader/resource_message_filter.h [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/common/render_frame_message_filter.mojom [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/common/view_messages.h [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/renderer/render_frame_impl.cc [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/renderer/render_frame_impl.h [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/renderer/render_frame_impl_browsertest.cc [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/renderer/render_view_browsertest.cc [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/renderer/render_view_impl.cc [modify] https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55/content/renderer/render_view_impl.h
,
Aug 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c commit 3ea157196bfdddd8dc3c82b2e994e4a932b8d46c Author: scottmg <scottmg@chromium.org> Date: Tue Aug 09 16:46:41 2016 Revert of Remove dependency of c/b/loader on c/b/host_zoom_map_impl.h (patchset #9 id:180001 of https://codereview.chromium.org/2211713002/ ) Reason for revert: Looks like there's a race during initialization on chromeos https://bugs.chromium.org/p/chromium/issues/detail?id=635832#c3 Original issue's description: > Remove dependency of c/b/loader on c/b/host_zoom_map_impl.h > > Contrary to previous attempt at: > https://chromium.googlesource.com/chromium/src.git/+/68c6f2ce16d9807b5cb82679099c82c40f39e911 > > RenderFrameMessageFilter is on the IO thread, and is a BrowserAssociatedInterface, so will maintain the ordering of the resource request and the zoom level request. See also https://codereview.chromium.org/2167513003. > > BUG= 609607 > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation > > Committed: https://crrev.com/d4423d511f18f4b0d2e94cf17bf6043bee449c55 > Cr-Commit-Position: refs/heads/master@{#410417} TBR=jam@chromium.org,tsepez@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 609607 , 635832 Review-Url: https://codereview.chromium.org/2222403002 Cr-Commit-Position: refs/heads/master@{#410705} [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/build/android/pylib/gtest/filter/content_browsertests_disabled [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/browser/frame_host/render_frame_message_filter.cc [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/browser/frame_host/render_frame_message_filter.h [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/browser/host_zoom_map_impl.h [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/browser/loader/DEPS [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/browser/loader/async_resource_handler.cc [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/browser/loader/async_resource_handler_unittest.cc [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/browser/loader/async_revalidation_manager_unittest.cc [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/browser/loader/resource_dispatcher_host_unittest.cc [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/browser/loader/resource_message_filter.cc [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/browser/loader/resource_message_filter.h [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/common/render_frame_message_filter.mojom [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/common/view_messages.h [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/renderer/render_frame_impl.cc [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/renderer/render_frame_impl.h [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/renderer/render_frame_impl_browsertest.cc [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/renderer/render_view_browsertest.cc [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/renderer/render_view_impl.cc [modify] https://crrev.com/3ea157196bfdddd8dc3c82b2e994e4a932b8d46c/content/renderer/render_view_impl.h
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/276753cf9b79b05acdc4a262241b8b31bf212e14 commit 276753cf9b79b05acdc4a262241b8b31bf212e14 Author: scottmg <scottmg@chromium.org> Date: Thu Oct 27 18:25:22 2016 Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation Had to pull the mojom out into a separate interface because Frame itself isn't associated, so a SetHostZoomLevel on Frame would be racy with the FrameMsg_CommitNavigation. Also fixes the IFrameZoomBrowserTest under PlzNavigate because it uses the new machinery. R=nasko@chromium.org BUG= 609607 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2374863002 Cr-Commit-Position: refs/heads/master@{#428084} [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/build/android/pylib/gtest/filter/content_browsertests_disabled [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/chrome/browser/previews/previews_infobar_tab_helper_unittest.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/BUILD.gn [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/frame_host/navigation_handle_impl_unittest.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/host_zoom_map_impl.h [add] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/host_zoom_map_observer.cc [add] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/host_zoom_map_observer.h [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/loader/DEPS [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/loader/async_resource_handler.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/loader/async_resource_handler_unittest.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/loader/async_revalidation_manager_unittest.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/loader/resource_dispatcher_host_unittest.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/loader/resource_message_filter.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/loader/resource_message_filter.h [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/loader/url_loader_factory_impl_unittest.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/browser/web_contents/web_contents_impl.h [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/common/BUILD.gn [add] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/common/host_zoom.mojom [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/common/view_messages.h [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/renderer/render_frame_impl.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/renderer/render_frame_impl.h [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/renderer/render_frame_impl_browsertest.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/renderer/render_view_browsertest.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/renderer/render_view_impl.cc [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/content/renderer/render_view_impl.h [modify] https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
,
Dec 10 2016
,
Mar 2 2017
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by scottmg@chromium.org
, May 5 2016