| 2016-01-21 21:43:57 |
Chris Coulson |
bug |
|
|
added bug |
| 2016-01-21 21:44:05 |
Chris Coulson |
oxide: importance |
Undecided |
Medium |
|
| 2016-01-21 21:44:07 |
Chris Coulson |
oxide: status |
New |
Triaged |
|
| 2016-01-21 21:49:32 |
Chris Coulson |
description |
I've reviewed the behaviour of Oxide if Qt or an application executes a nested QEventLoop on the main thread. This is needed to ensure we can safely handle drag and drop:
- Scenario 1: QEventLoop executed with no Chromium code on the stack.
In this case, the nested loop will pump the Chromium event queue as normal, and this is fine because there are no nested Chromium tasks.
- Scenario 2: QEventLoop executed from a non-nested Chromium task.
In this case, the nested loop will pump the Chromium event queue by re-entering oxide::qt::MessagePump::RunOneTask()
- It doesn't look like there's any re-entrancy issues here.
- MessageLoop::DoWork() will not run any tasks because the MessageLoop is in a task and nestable tasks haven't been explicitly allowed (MessageLoop::nestable_tasks_allowed_ is set to false in RunTask).
- MessageLoop::DoDelayedWork() - same as DoWork()
- MessageLoop::DoIdleWork() will process tasks from the deferred non-nestable work queue.
The last point is a bug, as DoIdleWork() should not run tasks in a nested loop. However, it seems like an edge case - tasks are only added to this work queue via nested calls to DoWork() and DoDelayedWork() when nestable tasks are allowed but the task isn't nestable. We don't do this anywhere in Oxide, although that doesn't mean it couldn't happen elsewhere in Chromium.
- Scenario 3: QEventLoop executed from a nested Chromium task (via RunLoop::Run)
As 2, above. Nested tasks are still blocked in DoIdle and DoDelayedWork because RunTask clears nestable_tasks_allowed_ before running the nested task. However, DoIdleWork will behave correctly here because the RunLoop::run_depth_ check in MessageLoop::ProcessNextDelayedNonNestableTask will work.
It seems like we should detect re-entrancy from a nested QEventLoop in oxide::qt::MessagePump::RunOneTask(). We could do this by adding an extra bit to RunState, that we set when calling in to MessageLoop.
- If we re-enter from a nested QEventLoop, the extra bit on the current RunState will be set. In this case, we should create a new RunLoop instance before calling in to MessageLoop.
- A nested RunLoop creates a new RunState in our MessagePump. In this case, we won't trigger the re-entrancy detection (and we don't need to) |
I've reviewed the behaviour of Oxide if Qt or an application executes a nested QEventLoop on the main thread. This is needed to ensure we can safely handle drag and drop:
- Scenario 1: QEventLoop executed with no Chromium code on the stack.
In this case, the nested loop will pump the Chromium event queue as normal, and this is fine because there are no nested Chromium tasks.
- Scenario 2: QEventLoop executed from a non-nested Chromium task.
In this case, the nested loop will pump the Chromium event queue by re-entering oxide::qt::MessagePump::RunOneTask()
- It doesn't look like there's any re-entrancy issues here.
- MessageLoop::DoWork() will not run any tasks because the MessageLoop is in a task and nestable tasks haven't been explicitly allowed (MessageLoop::nestable_tasks_allowed_ is set to false in RunTask).
- MessageLoop::DoDelayedWork() - same as DoWork()
- MessageLoop::DoIdleWork() will process tasks from the deferred non-nestable work queue.
The last point is a bug, as DoIdleWork() should not run tasks in a nested loop. However, it seems like an edge case - tasks are only added to this work queue via nested calls to DoWork() and DoDelayedWork() when nestable tasks are allowed but the task isn't nestable. We don't do this anywhere in Oxide, although that doesn't mean it couldn't happen elsewhere in Chromium.
- Scenario 3: QEventLoop executed from a nested Chromium task (via RunLoop::Run)
As 2, above. Nested tasks are still blocked in DoIdle and DoDelayedWork because RunTask clears nestable_tasks_allowed_ before running the nested task. However, DoIdleWork will behave correctly here because the RunLoop::run_depth_ check in MessageLoop::ProcessNextDelayedNonNestableTask will work.
It seems like we should detect re-entrancy from a nested QEventLoop in oxide::qt::MessagePump::RunOneTask(). We could do this by adding an extra bit to RunState, that we set when calling in to MessageLoop.
- If we re-enter from a nested QEventLoop, the extra bit on the current RunState will be set. In this case, we should create a new RunLoop instance before calling in to MessageLoop.
- A nested RunLoop creates a new RunState in our MessagePump. In this case, we won't trigger the re-entrancy detection (and we don't need to)
After this is fixed, a nested QEventLoop created outside of Oxide will process Qt events but won't run any Oxide or Chromium tasks. The only way to run a nested loop that processes nestable Oxide or Chromium tasks is to use MessageLoop::ScopedNestableTasksAllower and RunLoop, but that can only be done from code in Oxide. |
|
| 2016-01-21 22:24:26 |
Chris Coulson |
description |
I've reviewed the behaviour of Oxide if Qt or an application executes a nested QEventLoop on the main thread. This is needed to ensure we can safely handle drag and drop:
- Scenario 1: QEventLoop executed with no Chromium code on the stack.
In this case, the nested loop will pump the Chromium event queue as normal, and this is fine because there are no nested Chromium tasks.
- Scenario 2: QEventLoop executed from a non-nested Chromium task.
In this case, the nested loop will pump the Chromium event queue by re-entering oxide::qt::MessagePump::RunOneTask()
- It doesn't look like there's any re-entrancy issues here.
- MessageLoop::DoWork() will not run any tasks because the MessageLoop is in a task and nestable tasks haven't been explicitly allowed (MessageLoop::nestable_tasks_allowed_ is set to false in RunTask).
- MessageLoop::DoDelayedWork() - same as DoWork()
- MessageLoop::DoIdleWork() will process tasks from the deferred non-nestable work queue.
The last point is a bug, as DoIdleWork() should not run tasks in a nested loop. However, it seems like an edge case - tasks are only added to this work queue via nested calls to DoWork() and DoDelayedWork() when nestable tasks are allowed but the task isn't nestable. We don't do this anywhere in Oxide, although that doesn't mean it couldn't happen elsewhere in Chromium.
- Scenario 3: QEventLoop executed from a nested Chromium task (via RunLoop::Run)
As 2, above. Nested tasks are still blocked in DoIdle and DoDelayedWork because RunTask clears nestable_tasks_allowed_ before running the nested task. However, DoIdleWork will behave correctly here because the RunLoop::run_depth_ check in MessageLoop::ProcessNextDelayedNonNestableTask will work.
It seems like we should detect re-entrancy from a nested QEventLoop in oxide::qt::MessagePump::RunOneTask(). We could do this by adding an extra bit to RunState, that we set when calling in to MessageLoop.
- If we re-enter from a nested QEventLoop, the extra bit on the current RunState will be set. In this case, we should create a new RunLoop instance before calling in to MessageLoop.
- A nested RunLoop creates a new RunState in our MessagePump. In this case, we won't trigger the re-entrancy detection (and we don't need to)
After this is fixed, a nested QEventLoop created outside of Oxide will process Qt events but won't run any Oxide or Chromium tasks. The only way to run a nested loop that processes nestable Oxide or Chromium tasks is to use MessageLoop::ScopedNestableTasksAllower and RunLoop, but that can only be done from code in Oxide. |
I've reviewed the behaviour of Oxide if Qt or an application executes a nested QEventLoop on the main thread. This is needed to ensure we can safely handle drag and drop:
- Scenario 1: QEventLoop executed with no Chromium code on the stack.
In this case, the nested loop will pump the Chromium event queue as normal, and this is fine because there are no nested Chromium tasks.
- Scenario 2: QEventLoop executed from a non-nested Chromium task.
In this case, the nested loop will pump the Chromium event queue by re-entering oxide::qt::MessagePump::RunOneTask()
- It doesn't look like there's any re-entrancy issues here.
- MessageLoop::DoWork() will not run any tasks because the MessageLoop is in a task and nestable tasks haven't been explicitly allowed (MessageLoop::nestable_tasks_allowed_ is set to false in RunTask).
- MessageLoop::DoDelayedWork() - same as DoWork()
- MessageLoop::DoIdleWork() will process tasks from the deferred non-nestable work queue. This is a bug, as DoIdleWork() should not run tasks in a nested loop.
The last point seems like an edge case - tasks are only added to this work queue via nested calls to DoWork() and DoDelayedWork() when nestable tasks are allowed but the task isn't nestable. We don't do this anywhere in Oxide, although that doesn't mean it couldn't happen elsewhere in Chromium.
- Scenario 3: QEventLoop executed from a nested Chromium task (via RunLoop::Run)
As 2, above. Nested tasks are still blocked in DoIdle() and DoDelayedWork() because RunTask clears MessageLoop::nestable_tasks_allowed_ before running the nested task. However, DoIdleWork() will behave correctly here because the RunLoop::run_depth_ check in MessageLoop::ProcessNextDelayedNonNestableTask() will work.
- Scenario 4: QEventLoop executed from a non-nested Chromium task with nestable tasks enabled (via MessageLoop::ScopedNestableTaskAllower)
In this case, the nested loop will pump the Chromium event queue by re-entering oxide::qt::MessagePump::RunOneTask().
- MessageLoop::DoWork() will run any task. It's a bug that it will run non-nestable tasks too.
- MessageLoop::DoDelayedWork() - same as DoWork().
- MessageLoop::DoIdleWork() will process tasks from the deferred non-nestable work queue. As mentioned in 2, above - this is a bug.
- Scenario 5: QEventLoop executed from a nested Chromium task (via RunLoop::Run) with nestable tasks enabled (via MessageLoop::ScopedNestableTaskAllower)
- MessageLoop::DoWork() will run nestable tasks. Non-nestable tasks will be correctly blocked.
- MessageLoop::DoDelayedWork() - same as DoWork().
- MessageLoop::DoIdleWork() won't process any tasks, as expected.
It seems like we should detect re-entrancy from a nested QEventLoop in oxide::qt::MessagePump::RunOneTask() and increase the RunLoop depth. We could do this by adding an extra bit to RunState, that we set when calling in to MessageLoop.
- If we re-enter from a nested QEventLoop, the extra bit on the current RunState will be set. In this case, we should create a new RunLoop instance before calling in to MessageLoop.
- A nested RunLoop creates a new RunState in our MessagePump. In this case, we won't trigger the re-entrancy detection (and we don't need to because we already have the correct RunLoop depth)
After this is fixed, a nested QEventLoop created outside of Oxide will process Qt events but won't run any Oxide or Chromium tasks unless the call out of Oxide enables nestable tasks with MessageLoop::ScopedNestableTaskAllower.
Looking at the broken cases above:
- MessageLoop::DoIdleWork() will behave correctly because the RunLoop::run_depth_ check in MessageLoop::ProcessNextDelayedNonNestableTask() will always work.
- In scenario 4 (which will most likely happen with drag and drop), MessageLoop::DoWork() and MessageLoop::DoDelayedWork() won't run non-nestable tasks because the RunLoop::run_depth_ check in MessageLoop::DeferOrRunPendingTask() will always work. |
|
| 2016-01-22 11:28:06 |
Chris Coulson |
oxide: assignee |
|
Chris Coulson (chrisccoulson) |
|
| 2016-01-22 11:28:05 |
Launchpad Janitor |
branch linked |
|
lp:oxide |
|
| 2016-01-22 11:28:08 |
Chris Coulson |
oxide: milestone |
|
branch-1.13 |
|
| 2016-01-22 11:28:11 |
Chris Coulson |
oxide: status |
Triaged |
Fix Released |
|
| 2016-03-03 10:45:38 |
Launchpad Janitor |
branch linked |
|
lp:~oxide-developers/oxide/packaging.trusty.next |
|
| 2016-03-03 10:45:51 |
Launchpad Janitor |
branch linked |
|
lp:~oxide-developers/oxide/packaging.wily.next |
|
| 2016-03-03 10:46:12 |
Launchpad Janitor |
branch linked |
|
lp:~oxide-developers/oxide/packaging.xenial.next |
|
| 2016-03-03 10:46:59 |
Launchpad Janitor |
branch linked |
|
lp:~oxide-developers/oxide/packaging.vivid.overlay.next |
|
| 2016-03-07 10:55:17 |
Launchpad Janitor |
branch linked |
|
lp:~oxide-developers/oxide/packaging.xenial |
|
| 2016-03-07 10:56:05 |
Launchpad Janitor |
branch linked |
|
lp:~oxide-developers/oxide/packaging.wily |
|
| 2016-03-07 10:56:48 |
Launchpad Janitor |
branch linked |
|
lp:~oxide-developers/oxide/packaging.trusty |
|
| 2016-03-07 10:57:36 |
Launchpad Janitor |
branch linked |
|
lp:~oxide-developers/oxide/packaging.vivid.overlay |
|
