[sensors] Dispatch sensor events using postTask

[sensors] Dispatch sensor events using postTask

In order to avoid re-entrancy issues, this CL modifies sensor event
dispatching code and uses postTask to schedule event dispatch.
New test is added to ambient-light-sensor.html that checks if generic
sensor code works correctly when sensor is not supported

BUG=606766
Committed: https://crrev.com/28cd84c55e4fc6c1789fca46a4e23d2c4b90f517
Cr-Commit-Position: refs/heads/master@{#422761}

[shalamov, 2016-09-29 12:15:14]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 12:15:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shalamov, 2016-09-29 12:48:51]

alexander.shalamov@intel.com changed reviewers:
+ haraken@chromium.org, mikhail.pozdnyakov@intel.com, timvolodine@chromium.org

[shalamov, 2016-09-29 12:48:52]

Please take a look.

[commit-bot: I haz the power, 2016-09-29 13:45:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 13:45:40]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-09-29 14:06:51]

lgtm

[haraken, 2016-09-29 16:19:19]

Maybe can we use LocalDOMWindow::enqueueXXXEvent instead?

[shalamov, 2016-09-30 07:03:44]

On 2016/09/29 16:19:19, haraken wrote:
> Maybe can we use LocalDOMWindow::enqueueXXXEvent instead?

LocalDOMWindow::enqueueXXXEvent queues events for window or document objects.
Sensor events must have event target to be sensor object and they should not
bubble. Other modules, webaudio, filesystem, quota, etc., also use postTask to
dispatch event to avoid reentrancy issues. For example
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webaud...

[haraken, 2016-09-30 13:26:45]

On 2016/09/30 07:03:44, shalamov wrote:
> On 2016/09/29 16:19:19, haraken wrote:
> > Maybe can we use LocalDOMWindow::enqueueXXXEvent instead?
> 
> LocalDOMWindow::enqueueXXXEvent queues events for window or document objects.
> Sensor events must have event target to be sensor object and they should not
> bubble. Other modules, webaudio, filesystem, quota, etc., also use postTask to
> dispatch event to avoid reentrancy issues. For example
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webaud...

Ah, thanks, makes sense.

https://codereview.chromium.org/2381913002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2381913002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:269:
createSameThreadTask(&Sensor::notifySensorReadingChanged,
wrapWeakPersistent(this)));

wrapWeakPersistent means that the event may not be dispatched if the Sensor
object is gc-ed before the posted task runs. Is that okay?

[shalamov, 2016-09-30 13:51:44]

https://codereview.chromium.org/2381913002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2381913002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:269:
createSameThreadTask(&Sensor::notifySensorReadingChanged,
wrapWeakPersistent(this)));
On 2016/09/30 13:26:45, haraken wrote:
> 
> wrapWeakPersistent means that the event may not be dispatched if the Sensor
> object is gc-ed before the posted task runs. Is that okay?

Yes, that was intentional. If sensor goes out of scope and gc-ed, posted tasks
should not keep sensor object alive. That could happen when JS wrapper goes out
of scope and there are no event listeners for the sensor object.

[haraken, 2016-09-30 13:53:26]

On 2016/09/30 13:51:44, shalamov wrote:
>
https://codereview.chromium.org/2381913002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):
> 
>
https://codereview.chromium.org/2381913002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/sensor/Sensor.cpp:269:
> createSameThreadTask(&Sensor::notifySensorReadingChanged,
> wrapWeakPersistent(this)));
> On 2016/09/30 13:26:45, haraken wrote:
> > 
> > wrapWeakPersistent means that the event may not be dispatched if the Sensor
> > object is gc-ed before the posted task runs. Is that okay?
> 
> Yes, that was intentional. If sensor goes out of scope and gc-ed, posted tasks
> should not keep sensor object alive. That could happen when JS wrapper goes
out
> of scope and there are no event listeners for the sensor object.

LGTM

FWIW, it would be harmless to use wrapPersistent here because the posted task is
guaranteed to run in a finite time period.

[shalamov, 2016-10-03 06:10:24]

The CQ bit was checked by alexander.shalamov@intel.com

[commit-bot: I haz the power, 2016-10-03 06:10:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 07:19:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 07:19:55]

Failed to apply patch for third_party/WebKit/Source/modules/sensor/Sensor.cpp:
While running git apply --index -3 -p1;
  error: patch failed: third_party/WebKit/Source/modules/sensor/Sensor.cpp:263
  Falling back to three-way merge...
  Applied patch to 'third_party/WebKit/Source/modules/sensor/Sensor.cpp' with
conflicts.
  U third_party/WebKit/Source/modules/sensor/Sensor.cpp

Patch:       third_party/WebKit/Source/modules/sensor/Sensor.cpp
Index: third_party/WebKit/Source/modules/sensor/Sensor.cpp
diff --git a/third_party/WebKit/Source/modules/sensor/Sensor.cpp
b/third_party/WebKit/Source/modules/sensor/Sensor.cpp
index
d5f1b0421b799ab73d20664171a2d3c7b609c7ee..e0f7f31f1146d45f0ef669607a5376bb909452c4
100644
--- a/third_party/WebKit/Source/modules/sensor/Sensor.cpp
+++ b/third_party/WebKit/Source/modules/sensor/Sensor.cpp
@@ -6,6 +6,7 @@
 
 #include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
+#include "core/dom/ExecutionContextTask.h"
 #include "core/inspector/ConsoleMessage.h"
 #include "device/generic_sensor/public/interfaces/sensor.mojom-blink.h"
 #include "modules/sensor/SensorErrorEvent.h"
@@ -263,8 +264,10 @@ void Sensor::pollForData()
     m_sensorProxy->updateInternalReading();
 
     DCHECK(m_sensorReading);
-    if (m_sensorReading->isReadingUpdated(m_storedData))
-        dispatchEvent(SensorReadingEvent::create(EventTypeNames::change,
m_sensorReading));
+    if (getExecutionContext() &&
m_sensorReading->isReadingUpdated(m_storedData)) {
+        getExecutionContext()->postTask(BLINK_FROM_HERE,
+            createSameThreadTask(&Sensor::notifySensorReadingChanged,
wrapWeakPersistent(this)));
+    }
 
     m_storedData = m_sensorProxy->reading();
 }
@@ -274,7 +277,11 @@ void Sensor::updateState(Sensor::SensorState newState)
     if (newState == m_state)
         return;
     m_state = newState;
-    dispatchEvent(Event::create(EventTypeNames::statechange));
+    if (getExecutionContext()) {
+        getExecutionContext()->postTask(BLINK_FROM_HERE,
+            createSameThreadTask(&Sensor::notifyStateChanged,
wrapWeakPersistent(this)));
+    }
+
     updatePollingStatus();
 }
 
@@ -297,4 +304,14 @@ void Sensor::updatePollingStatus()
     }
 }
 
+void Sensor::notifySensorReadingChanged()
+{
+    dispatchEvent(SensorReadingEvent::create(EventTypeNames::change,
m_sensorReading));
+}
+
+void Sensor::notifyStateChanged()
+{
+    dispatchEvent(Event::create(EventTypeNames::statechange));
+}
+
 } // namespace blink

[shalamov, 2016-10-03 08:06:21]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 08:06:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 10:05:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 10:05:28]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[shalamov, 2016-10-04 08:04:51]

The CQ bit was checked by alexander.shalamov@intel.com

[shalamov, 2016-10-04 08:04:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, mikhail.pozdnyakov@intel.com
Link to the patchset: https://codereview.chromium.org/2381913002/#ps20001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-10-04 08:05:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 12:07:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-04 12:10:50]

Description was changed from

==========
[sensors] Dispatch sensor events using postTask

In order to avoid re-entrancy issues, this CL modifies sensor event
dispatching code and uses postTask to schedule event dispatch.
New test is added to ambient-light-sensor.html that checks if generic
sensor code works correctly when sensor is not supported

BUG=606766
==========

to

==========
[sensors] Dispatch sensor events using postTask

In order to avoid re-entrancy issues, this CL modifies sensor event
dispatching code and uses postTask to schedule event dispatch.
New test is added to ambient-light-sensor.html that checks if generic
sensor code works correctly when sensor is not supported

BUG=606766

Committed: https://crrev.com/28cd84c55e4fc6c1789fca46a4e23d2c4b90f517
Cr-Commit-Position: refs/heads/master@{#422761}
==========

[commit-bot: I haz the power, 2016-10-04 12:10:51]

Patchset 2 (id:??) landed as
https://crrev.com/28cd84c55e4fc6c1789fca46a4e23d2c4b90f517
Cr-Commit-Position: refs/heads/master@{#422761}