Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
977573002:
content: Add an overridable task runner to ChildThreadImpl (Closed)
|
Created:
5 years, 9 months ago by Sami Modified:
5 years, 7 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, kinuko+watch, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncontent: Add an overridable task runner to ChildThreadImpl
Make it possible to get a task runner for the main thread via
ChildThreadImpl::Get()->GetTaskRunner(). This lets the renderer
scheduler override the main thread task runner for the message filters
which get created by ChildThreadImpl to ensure correct task ordering.
This fixes a task ordering issue with WebWorkers. Previously the
worker's IPC message filter was configured to post tasks directly to
the main message loop. This caused them to run out of order w.r.t.
other worker tasks posted through the renderer scheduler. As a result,
the http/tests/serviceworker/ready.html layout test started timing out
because the notification from worker registration was getting dropped
due to an inconsistent internal state.
BUG=444764
Patch Set 1 #Patch Set 2 : Rebased. #Patch Set 3 : Rebased. #
Total comments: 2
Patch Set 4 : Rebased. #
Total comments: 4
Patch Set 5 : Review comments. #Patch Set 6 : Test build fix. #
Total comments: 1
Patch Set 7 : Work around MSVC bug. #
Total comments: 7
Patch Set 8 : Explicit tweaks. #Patch Set 9 : Move GetTaskRunner back to the RenderThread. #
Messages
Total messages: 51 (18 generated)
skyostil@chromium.org changed reviewers: + rmcilroy@chromium.org
PTAL, depends on https://codereview.chromium.org/974933002/
lgtm with one comment for a followon CL (which may or may not be possible) https://codereview.chromium.org/977573002/diff/40001/content/child/worker_thr... File content/child/worker_thread_message_filter.cc (left): https://codereview.chromium.org/977573002/diff/40001/content/child/worker_thr... content/child/worker_thread_message_filter.cc:16: : main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()), Can we set ThreadTaskRunnerHandle to be the RenderScheduler's default task runner on the main thread (possibly in follow up CLs) to ensure that any other code which uses it also get's the correct task runner?
skyostil@chromium.org changed reviewers: + sievers@chromium.org
Thanks Ross. Daniel, could you check this one too? https://codereview.chromium.org/977573002/diff/40001/content/child/worker_thr... File content/child/worker_thread_message_filter.cc (left): https://codereview.chromium.org/977573002/diff/40001/content/child/worker_thr... content/child/worker_thread_message_filter.cc:16: : main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()), On 2015/03/03 19:57:14, rmcilroy wrote: > Can we set ThreadTaskRunnerHandle to be the RenderScheduler's default task > runner on the main thread (possibly in follow up CLs) to ensure that any other > code which uses it also get's the correct task runner? Yeah, I suppose we could do that too. There are 65 remaining calls to base::ThreadTaskRunnerHandle in content/ after this patch and changing them from using one static global to another static global doesn't really seem that useful :) I'll put together a patch.
lgtm https://codereview.chromium.org/977573002/diff/60001/content/child/child_thre... File content/child/child_thread_impl.h (right): https://codereview.chromium.org/977573002/diff/60001/content/child/child_thre... content/child/child_thread_impl.h:83: scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() override; Is there a way (at least longer term) how we could make it more clear how to get the right task runner for the different call sites? Esp. since ChildThreadImpl::message_loop() is also public, and MessageLoop also has GetTaskRunner(). Could you also explain in the cl description what exactly the impact/bug was of using the wrong task runner and bypassing the scheduler? https://codereview.chromium.org/977573002/diff/60001/content/public/child/chi... File content/public/child/child_thread.h (right): https://codereview.chromium.org/977573002/diff/60001/content/public/child/chi... content/public/child/child_thread.h:30: virtual scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() = 0; Should this have a comment and how this could potentially be different from the thread's lower level task runner?
Thanks! https://codereview.chromium.org/977573002/diff/60001/content/child/child_thre... File content/child/child_thread_impl.h (right): https://codereview.chromium.org/977573002/diff/60001/content/child/child_thre... content/child/child_thread_impl.h:83: scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() override; On 2015/03/04 19:15:02, sievers wrote: > Is there a way (at least longer term) how we could make it more clear how to get > the right task runner for the different call sites? Esp. since > ChildThreadImpl::message_loop() is also public, and MessageLoop also has > GetTaskRunner(). I agree that it's a mess :( To add on top of the things you mentioned, there's base::MessageLoop::current(), base::MessageLoopProxy::current() and ThreadTaskRunnerHandle::Get(). I tried removing message_loop() here but couldn't because some callers want to add task observers to it, which you can't do with a SingleThreadTaskRunner. Maybe one way to clean this up would be to separate out task observing into a new interface (or possibly into SingleThreadTaskRunner) so that fewer things need to care about the concrete message loop. Longer term I'd like to remove most if not all global ways to get to a task runner and require everyone to plumb the task runners to where they are needed. That's a little ways off though :) I'll add some instructions here on how to use this. > Could you also explain in the cl description what exactly the impact/bug was of > using the wrong task runner and bypassing the scheduler? Done. https://codereview.chromium.org/977573002/diff/60001/content/public/child/chi... File content/public/child/child_thread.h (right): https://codereview.chromium.org/977573002/diff/60001/content/public/child/chi... content/public/child/child_thread.h:30: virtual scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() = 0; On 2015/03/04 19:15:02, sievers wrote: > Should this have a comment and how this could potentially be different from the > thread's lower level task runner? Yep, I'll add a note here too.
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/977573002/#ps80001 (title: "Review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/977573002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel_tests_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_rel_tes...)
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/977573002/#ps100001 (title: "Test build fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/977573002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by skyostil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/977573002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by skyostil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/977573002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
https://codereview.chromium.org/977573002/diff/100001/content/child/child_thr... File content/child/child_thread_impl.cc (right): https://codereview.chromium.org/977573002/diff/100001/content/child/child_thr... content/child/child_thread_impl.cc:304: : ChildThreadImpl(Options::Builder().Build()) { So turns out this was triggering an MSVC compiler code generation bug[1] which caused the win8 trybot failures. I've reverted back to the non-delegated constructor we had here before. I'll send a notice to blink-dev too. [1] https://connect.microsoft.com/VisualStudio/feedbackdetail/view/841488
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/977573002/#ps120001 (title: "Work around MSVC bug.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/977573002/120001
The CQ bit was unchecked by rsleevi@chromium.org
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/977573002/diff/120001/content/renderer/render... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/977573002/diff/120001/content/renderer/render... content/renderer/render_thread_impl.h:134: RenderThreadImpl(scoped_ptr<RendererScheduler> renderer_scheduler); This should be explicit.
jabdelmalek@google.com changed reviewers: + jabdelmalek@google.com
https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... File content/public/child/child_thread.h (right): https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... content/public/child/child_thread.h:33: // going to the message loop directly. this seems like quite a large change, as there are lots of places that get the current message loop in the renderer. i.e. see https://code.google.com/p/chromium/codesearch#search/&q=%22MessageLoop::curre... 1) are you planning on updating all of them? 2) what would enforce that people use this method instead of getting the raw MessageLoop? https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... content/public/child/child_thread.h:34: virtual scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() = 0; why is this in the public interface, if only content-internal code uses it? this should be on ChildThreadImpl only. RenderThread can still expose it if it's used by code outside of content, and it can go through ChildThreadImpl
https://codereview.chromium.org/977573002/diff/120001/content/renderer/render... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/977573002/diff/120001/content/renderer/render... content/renderer/render_thread_impl.h:134: RenderThreadImpl(scoped_ptr<RendererScheduler> renderer_scheduler); On 2015/03/06 17:55:23, Ryan Sleevi wrote: > This should be explicit. Thanks, fixed.
https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... File content/public/child/child_thread.h (right): https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... content/public/child/child_thread.h:33: // going to the message loop directly. On 2015/03/06 18:06:13, jabdelmalek wrote: > this seems like quite a large change, as there are lots of places that get the > current message loop in the renderer. i.e. see > https://code.google.com/p/chromium/codesearch#search/&q=%22MessageLoop::curre... > > 1) are you planning on updating all of them? Yes, it's unfortunate that a lot of code is hard coded to use the global message loop. The other popular way to do that is ThreadTaskRunnerHandle::Get(). We're incrementally migrating code to use and pass around the correct the task runner explicitly. Longer term we'd like to delete the global getters completely. > 2) what would enforce that people use this method instead of getting the raw > MessageLoop? A presubmit check for content/ is the best way I can think of. I don't think it can be fully automated because it's not obvious on which thread a given piece of code is running. https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... content/public/child/child_thread.h:34: virtual scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() = 0; On 2015/03/06 18:06:13, jabdelmalek wrote: > why is this in the public interface, if only content-internal code uses it? this > should be on ChildThreadImpl only. RenderThread can still expose it if it's used > by code outside of content, and it can go through ChildThreadImpl Good point, I've now moved GetTaskRunner() back to RenderThread.
https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... File content/public/child/child_thread.h (right): https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... content/public/child/child_thread.h:33: // going to the message loop directly. On 2015/03/06 18:41:46, Sami wrote: > Yes, it's unfortunate that a lot of code is hard coded to use the global message > loop. The other popular way to do that is ThreadTaskRunnerHandle::Get(). We're > incrementally migrating code to use and pass around the correct the task runner > explicitly. Longer term we'd like to delete the global getters completely. Then you really should have been having this design discussion on chromium-dev. I won't pile on to this review, but I think that's a HUGE statement and, while admirable, definitely deserves discussion of the trade-offs and whether that goal is good (from a code health perspective) Having more code use ThreadTaskRunnerHandle::Get() and is something I ding everyone on during reviews. But I would encourage you to be public about that discussion, rather than subtlely through CLs like this. If I've missed the discussion, apologies, but you're fundamentally talking about a change to the API guarantees of //base, even if it may not be obvious as such.
On 2015/03/06 18:41:46, Sami wrote: > https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... > File content/public/child/child_thread.h (right): > > https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... > content/public/child/child_thread.h:33: // going to the message loop directly. > On 2015/03/06 18:06:13, jabdelmalek wrote: > > this seems like quite a large change, as there are lots of places that get the > > current message loop in the renderer. i.e. see > > > https://code.google.com/p/chromium/codesearch#search/&q=%22MessageLoop::curre... > > > > 1) are you planning on updating all of them? > > Yes, it's unfortunate that a lot of code is hard coded to use the global message > loop. This has been standard practice from the beginning of the project. I agree with Ryan that this should involve some discussion on chromium-dev, as it's a big change. > The other popular way to do that is ThreadTaskRunnerHandle::Get(). We're > incrementally migrating code to use and pass around the correct the task runner > explicitly. Longer term we'd like to delete the global getters completely. > > > 2) what would enforce that people use this method instead of getting the raw > > MessageLoop? > > A presubmit check for content/ is the best way I can think of. I don't think it > can be fully automated because it's not obvious on which thread a given piece of > code is running. presubmit check for content/ isn't enough. There's also chrome, components, other embedders like content_shell, cast, webview etc... also i don't think that even a presubmit check for all of them would work. i.e. someone can get the messageloop/task runner pointer and store it as a different named variable, and then call it later. so i think you'll need some programmatic way to achieve/enforce this. > > https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... > content/public/child/child_thread.h:34: virtual > scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() = 0; > On 2015/03/06 18:06:13, jabdelmalek wrote: > > why is this in the public interface, if only content-internal code uses it? > this > > should be on ChildThreadImpl only. RenderThread can still expose it if it's > used > > by code outside of content, and it can go through ChildThreadImpl > > Good point, I've now moved GetTaskRunner() back to RenderThread.
I think it was obvious that this would be a drawback of making the renderer scheduler a concept on top of the existing task primitives. Eventually you end up with some shared code (here code shared between different processes) that accesses the lower level primitives directly. That being said, the immediate goal is to fix the issues in the renderer where it isn't posting to the right task runner yet (msg filters). Would it be less controversial to try and not expose yet another task runner interface in ChildThread(Impl) yet and try to special-case this more for the renderer for the time being (if possible)?
On 2015/03/06 19:00:31, Ryan Sleevi wrote: > https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... > File content/public/child/child_thread.h (right): > > https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... > content/public/child/child_thread.h:33: // going to the message loop directly. > On 2015/03/06 18:41:46, Sami wrote: > > Yes, it's unfortunate that a lot of code is hard coded to use the global > message > > loop. The other popular way to do that is ThreadTaskRunnerHandle::Get(). We're > > incrementally migrating code to use and pass around the correct the task > runner > > explicitly. Longer term we'd like to delete the global getters completely. > > Then you really should have been having this design discussion on chromium-dev. > > I won't pile on to this review, but I think that's a HUGE statement and, while > admirable, definitely deserves discussion of the trade-offs and whether that > goal is good (from a code health perspective) > > Having more code use ThreadTaskRunnerHandle::Get() and is something I ding > everyone on during reviews. But I would encourage you to be public about that > discussion, rather than subtlely through CLs like this. If I've missed the > discussion, apologies, but you're fundamentally talking about a change to the > API guarantees of //base, even if it may not be obvious as such. Sure, I'm open to finding the best solution here and don't want to sneak anything by anyone. I think the fundamental problem is that we (as in the renderer scheduler) are trying to provide a replacement for functionality in base/, but because the corresponding API in base isn't fully virtualized, we can't replace it completely. Early on one of the options was doing more of the work in base[1], but the consensus was that making changes at that level was too risky, so we went for the layered model we have today[2]. I think it might be time to re-evaluate that decision and consider extending base in a way that lets us do things in a safer way. That said, this patch does fix a known ordering bug and replaces the use a global static with, ahem, a global static with slightly smaller scope, so I think it's still an improvement. [1] https://docs.google.com/a/chromium.org/document/d/1hsrqvBXy-6xwLERHPYpswMp0DV... [2] https://docs.google.com/a/chromium.org/document/d/16f_RIhZa47uEK_OdtTgzWdRU0R...
On 2015/03/06 19:09:01, jam wrote: > On 2015/03/06 18:41:46, Sami wrote: > > > https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... > > File content/public/child/child_thread.h (right): > > > > > https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... > > content/public/child/child_thread.h:33: // going to the message loop directly. > > On 2015/03/06 18:06:13, jabdelmalek wrote: > > > this seems like quite a large change, as there are lots of places that get > the > > > current message loop in the renderer. i.e. see > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=%22MessageLoop::curre... > > > > > > 1) are you planning on updating all of them? > > > > Yes, it's unfortunate that a lot of code is hard coded to use the global > message > > loop. > > This has been standard practice from the beginning of the project. > > I agree with Ryan that this should involve some discussion on chromium-dev, as > it's a big change. FWIW, some subsystems like cc/ prefer explicit task runners because just using the one in TLS is ambiguous. I think there may not be a one size fits all solution here and we'll just have to support both kinds of uses. > > The other popular way to do that is ThreadTaskRunnerHandle::Get(). We're > > incrementally migrating code to use and pass around the correct the task > runner > > explicitly. Longer term we'd like to delete the global getters completely. > > > > > 2) what would enforce that people use this method instead of getting the raw > > > MessageLoop? > > > > A presubmit check for content/ is the best way I can think of. I don't think > it > > can be fully automated because it's not obvious on which thread a given piece > of > > code is running. > > presubmit check for content/ isn't enough. There's also chrome, components, > other embedders like content_shell, cast, webview etc... also i don't think that > even a presubmit check for all of them would work. i.e. someone can get the > messageloop/task runner pointer and store it as a different named variable, and > then call it later. > > so i think you'll need some programmatic way to achieve/enforce this. Agreed, I was being too optimistic there. > > > > > https://codereview.chromium.org/977573002/diff/120001/content/public/child/ch... > > content/public/child/child_thread.h:34: virtual > > scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() = 0; > > On 2015/03/06 18:06:13, jabdelmalek wrote: > > > why is this in the public interface, if only content-internal code uses it? > > this > > > should be on ChildThreadImpl only. RenderThread can still expose it if it's > > used > > > by code outside of content, and it can go through ChildThreadImpl > > > > Good point, I've now moved GetTaskRunner() back to RenderThread.
What do people feel about landing this patch now and iterating on a more general way to achieve this? I can draft a design doc with some options.
I'm not sure that we even need to enforce changing any usage patterns or semantics to untangle this. For example, if we just constructed (destructed) everything simultaneously early (late) enough, then anything that uses TLS would point to the right thing (redirection to scheduler if enabled). We could also redirect internally (so that you can hold a ref but internally there is a level of indirection). MsgLoopProxy already does something similar for when a msg loop goes away (drops tasks). (I think of the scheduler as a filter that allows reordering where possible, so it doesn't seem totally unreasonable to insert or remove it.) Whether a client uses a TLS getter on the current thread vs. a reference doesn't really matter (well, if you post from another thread you obviously want to hold a reference). I don't consider this patch as making anything worse, since there are no plans to have the different task runner getters return different values other than for the renderer thread (where it now makes it a little easier to get the correct runner).
On 2015/03/06 20:26:33, Sami wrote: > What do people feel about landing this patch now and iterating on a more general > way to achieve this? I can draft a design doc with some options. I worry that having two different ways of posting tasks in the renderer, with separate queues, is going to introduce other problems. i.e. other code on the worker thread is still using MessageLoop's queue. Are you sure we're not going to run into this same problem with this cl as is?
On 2015/03/06 20:42:35, jabdelmalek wrote: > On 2015/03/06 20:26:33, Sami wrote: > > What do people feel about landing this patch now and iterating on a more > general > > way to achieve this? I can draft a design doc with some options. > > I worry that having two different ways of posting tasks in the renderer, with > separate queues, is going to introduce other problems. i.e. other code on the > worker thread is still using MessageLoop's queue. Are you sure we're not going > to run into this same problem with this cl as is? We already have those two different queues on the renderer main thread. This patch just makes it possible for ChildThreadImpl to use the same queue as RendererThreadImpl is using. In other words, having those two different queues exposed at the same time is a problem, and this patch is fixing one instance of that problem. I think we're all agreed that we want to make it impossible to use the wrong task queue, but doing that needs a bit more work.
On 2015/03/06 20:55:13, Sami wrote: > On 2015/03/06 20:42:35, jabdelmalek wrote: > > On 2015/03/06 20:26:33, Sami wrote: > > > What do people feel about landing this patch now and iterating on a more > > general > > > way to achieve this? I can draft a design doc with some options. > > > > I worry that having two different ways of posting tasks in the renderer, with > > separate queues, is going to introduce other problems. i.e. other code on the > > worker thread is still using MessageLoop's queue. Are you sure we're not going > > to run into this same problem with this cl as is? > > We already have those two different queues on the renderer main thread. This > patch just makes it possible for ChildThreadImpl to use the same queue as > RendererThreadImpl is using. In other words, having those two different queues > exposed at the same time is a problem, and this patch is fixing one instance of > that problem. I think we're all agreed that we want to make it impossible to use > the wrong task queue, but doing that needs a bit more work. Yep I understand that. Perhaps my concern wasn't clear. Even with this patch, not all code running on the web worker thread uses the scheduler queue. So how do we know that it doesn't introduce any more bugs, i.e. because code which uses MessageLoop::PostTask there now uses a different queue from the code this cl changes?
This just caught my attention... not meant to stir up more, but have a meta comment: could this CL be tracked by a separate bug (in addition to the current one) that describes the problem more clearly? The CL description says it well, but the bug related to this CL is really misleading (I thought it's just one of perf regressions in IDB). I think the underlying problem deserves its own bug. From an experience having been bitten by the same problem recently (having two queues caused unexpected task ordering) I really would like to have an assertion or some safe-guards not to introduce any similar bugs, and would love to see this discussed on chromium-dev.
On 2015/03/08 11:11:24, kinuko wrote: > This just caught my attention... not meant to stir up more, but have a meta > comment: could this CL be tracked by a separate bug (in addition to the current > one) that describes the problem more clearly? The CL description says it well, > but the bug related to this CL is really misleading (I thought it's just one of > perf regressions in IDB). I think the underlying problem deserves its own bug. > > From an experience having been bitten by the same problem recently (having two > queues caused unexpected task ordering) I really would like to have an assertion > or some safe-guards not to introduce any similar bugs, and would love to see > this discussed on chromium-dev. Excellent point, I've opened https://code.google.com/p/chromium/issues/detail?id=465354 to track this. Our current thinking is that if we can make sure the TLS task runners point to the right thing, there's no way to make the wrong choice anymore.
On 2015/03/06 21:42:40, jabdelmalek wrote: > Yep I understand that. Perhaps my concern wasn't clear. Even with this patch, > not all code running on the web worker thread uses the scheduler queue. So how > do we know that it doesn't introduce any more bugs, i.e. because code which uses > MessageLoop::PostTask there now uses a different queue from the code this cl > changes? Ok, now I see what you meant: doing this might introduce new bugs because it's not guaranteed to cover all the worker tasks that have ordering dependencies. I agree, there are no safeguards for that. Let's try to fix this at a lower level instead.
rsleevi@chromium.org changed reviewers: - rsleevi@chromium.org
Here's a design doc with a proposal to make using the wrong task runner impossible: https://docs.google.com/a/chromium.org/document/d/1qxdh2I61_aB_Uzh1QgNqvdWFBC... With this approach we wouldn't need to chance any task posting practices for any code running in the renderer. All comments welcome! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
