[Sensors] Fix reading updates after page refresh

[Sensors] Fix reading updates after page refresh

This patch fixes reading updates after page refresh. The route cause of the problem was that the outdated document instance was cached in SensorProxy.

BUG=679243
BUG=606766
Review-Url: https://codereview.chromium.org/2644873002
Cr-Commit-Position: refs/heads/master@{#446658}
Committed: https://chromium.googlesource.com/chromium/src/+/c78e848fc18f203167cb53aedc0e3736bce149b1

[Mikhail, 2017-01-19 12:40:49]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 12:41:07]

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

[Mikhail, 2017-01-19 12:43:24]

Description was changed from

==========
[Sensors] Fix reading updates after page refresh

This patch fixes reading updates after page refresh. The route cause of the
problem was that the outdated document instance was cached in SensorProxy.

BUG=679243
==========

to

==========
[Sensors] Fix reading updates after page refresh

This patch fixes reading updates after page refresh. The route cause of the
problem was that the outdated document instance was cached in SensorProxy.

BUG=679243
BUG=606766
==========

[Mikhail, 2017-01-19 12:43:24]

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

[commit-bot: I haz the power, 2017-01-19 12:50:43]

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

[commit-bot: I haz the power, 2017-01-19 12:50:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mikhail, 2017-01-19 12:59:36]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[Mikhail, 2017-01-19 12:59:49]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-01-19 13:00:00]

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

[Mikhail, 2017-01-19 13:13:56]

PTAL

[haraken, 2017-01-19 13:57:28]

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp (right):

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:37:
m_hasPendingAnimationFrameTask = false;

Would you help me understand why you need to set it to false here?

Also even if you set it to false, the animation frame task is not yet canceled.

[commit-bot: I haz the power, 2017-01-19 14:21:17]

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

[commit-bot: I haz the power, 2017-01-19 14:21:17]

Dry run: This issue passed the CQ dry run.

[shalamov, 2017-01-20 09:25:33]

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:73:
initSensorProxyIfNeeded();
Should we pass / use execution context of a start() call in
initSensorProxyIfNeeded()? At the moment we use context that was provided in the
constructor.

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:167: m_sensorProxy =
provider->createSensorProxy(m_type, document->page(),
should we throw "InvalidStateError" if context is no longer associated with a
page (page is null)?

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp (right):

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:23: auto
callback = WTF::bind(&SensorReadingUpdater::onAnimationFrame,
can be moved inside if (...)

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:37:
m_hasPendingAnimationFrameTask = false;
haraken@ is right, looks like if task was enqueued, setting to false would not
stop consecutive "enqueue" calls when callback is called, right?

[Mikhail, 2017-01-23 15:51:25]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 15:51:35]

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, 2017-01-23 15:52:24]

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

[commit-bot: I haz the power, 2017-01-23 15:52:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, no build
URL)

[Mikhail, 2017-01-23 16:34:50]

Patchset #2 (id:40001) has been deleted

[Mikhail, 2017-01-24 11:55:39]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 11:55:55]

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

[Mikhail, 2017-01-24 12:00:18]

PTAL

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:73:
initSensorProxyIfNeeded();
On 2017/01/20 09:25:32, shalamov wrote:
> Should we pass / use execution context of a start() call in
> initSensorProxyIfNeeded()? At the moment we use context that was provided in
the
> constructor.

Does it make any practical change? haraken@, wdyt?

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:167: m_sensorProxy =
provider->createSensorProxy(m_type, document->page(),
On 2017/01/20 09:25:32, shalamov wrote:
> should we throw "InvalidStateError" if context is no longer associated with a
> page (page is null)?

page is used only in PageVisibilityObserver which can contain nullptr

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp (right):

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:23: auto
callback = WTF::bind(&SensorReadingUpdater::onAnimationFrame,
On 2017/01/20 09:25:32, shalamov wrote:
> can be moved inside if (...)

Done.

https://codereview.chromium.org/2644873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:37:
m_hasPendingAnimationFrameTask = false;
On 2017/01/20 09:25:33, shalamov wrote:
> haraken@ is right, looks like if task was enqueued, setting to false would not
> stop consecutive "enqueue" calls when callback is called, right?

I thought it as just a "reset" of the internal state of the object, anyway the
latest patch does not contain it :)

[shalamov, 2017-01-24 12:05:45]

lgtm

I have not more comments, but would be nice to get haraken@ opinion about
execution context.

[commit-bot: I haz the power, 2017-01-24 12:27:19]

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

[commit-bot: I haz the power, 2017-01-24 12:27:20]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Reilly Grant (use Gerrit), 2017-01-24 19:49:52]

lgtm

[haraken, 2017-01-24 21:19:14]

https://codereview.chromium.org/2644873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp (right):

https://codereview.chromium.org/2644873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:20: if
(m_document != m_sensorProxy->document()) {

This condition doesn't look correct to me.

Note that WeakMember is cleared only when a GC is triggered; i.e., it doesn't
get cleared until you hit a next GC.

I think what you want to do here is:

- Make SensorProxy (or SensorReadingUpdater) inherit from ContextClient
- Then SensorProxy::getExecutionContext() returns false after the Document is
detached. You can use it to check if the Document is detached or not.

[Mikhail, 2017-01-25 10:50:40]

On 2017/01/24 21:19:14, haraken wrote:
>
https://codereview.chromium.org/2644873002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp
(right):
> 
>
https://codereview.chromium.org/2644873002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:20: if
> (m_document != m_sensorProxy->document()) {
> 
> This condition doesn't look correct to me.
> 
> Note that WeakMember is cleared only when a GC is triggered; i.e., it doesn't
> get cleared until you hit a next GC.
> 
> I think what you want to do here is:
> 
> - Make SensorProxy (or SensorReadingUpdater) inherit from ContextClient
> - Then SensorProxy::getExecutionContext() returns false after the Document is
> detached. You can use it to check if the Document is detached or not.

Let me clarify: m_sensorProxy->document() now always returns the actual
(attached) document; in this condition I just check that the stored document is
not the same as the currently attached one.

Inheriting from ContextClient is problematic here IMO as it will require to
re-create SensorProxy each time the document is detached.

[haraken, 2017-01-25 11:01:30]

On 2017/01/25 10:50:40, Mikhail wrote:
> On 2017/01/24 21:19:14, haraken wrote:
> >
>
https://codereview.chromium.org/2644873002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2644873002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:20: if
> > (m_document != m_sensorProxy->document()) {
> > 
> > This condition doesn't look correct to me.
> > 
> > Note that WeakMember is cleared only when a GC is triggered; i.e., it
doesn't
> > get cleared until you hit a next GC.
> > 
> > I think what you want to do here is:
> > 
> > - Make SensorProxy (or SensorReadingUpdater) inherit from ContextClient
> > - Then SensorProxy::getExecutionContext() returns false after the Document
is
> > detached. You can use it to check if the Document is detached or not.
> 
> Let me clarify: m_sensorProxy->document() now always returns the actual
> (attached) document; in this condition I just check that the stored document
is
> not the same as the currently attached one.
> 
> Inheriting from ContextClient is problematic here IMO as it will require to
> re-create SensorProxy each time the document is detached.

When does 'm_document != m_sensorProxy->document()' start returning true?

I'm wondering why:

- you don't need to reset m_document when the document gets detached. If you use
a WeakMember, it isn't cleared until a next GC hits (which is timing-dependent).

- you need to use a WeakMember in the first place (i.e., why you can't use a
Member)

[Mikhail, 2017-01-25 15:22:49]

On 2017/01/25 11:01:30, haraken wrote:
> When does 'm_document != m_sensorProxy->document()' start returning true?
when the "actual" i.e. current frame's document is updated
> 
> I'm wondering why:
> 
> - you don't need to reset m_document when the document gets detached. If you
use
> a WeakMember, it isn't cleared until a next GC hits (which is
timing-dependent).

it's reset to the current frame's document each time, but now I see that
'IsDetached()' API could be a better approach indeed (used in the latest patch)
>
> - you need to use a WeakMember in the first place (i.e., why you can't use a
> Member)

otherwise the detached document might not get deleted: SensorReadingUpdater will
hold it.

[haraken, 2017-01-25 18:20:06]

https://codereview.chromium.org/2644873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp (right):

https://codereview.chromium.org/2644873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:20: if
(!m_document || m_document->isDetached()) {

Will the following work?

- Make SensorReadingUpdater inherit from ContextClient
- Use if(!getExecutionContext()) here.

https://codereview.chromium.org/2644873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:24: m_document
= m_sensorProxy->document();

Can we set m_document in SensorReadingUpdater's constructor?

Once you make SensorReadingUpdater inherit from ContextClient, it won't be
needed though.

[Mikhail, 2017-01-26 06:19:59]

Thanks for your comments!

https://codereview.chromium.org/2644873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp (right):

https://codereview.chromium.org/2644873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:20: if
(!m_document || m_document->isDetached()) {
On 2017/01/25 18:20:06, haraken wrote:
> 
> Will the following work?
> 
> - Make SensorReadingUpdater inherit from ContextClient
> - Use if(!getExecutionContext()) here.

As far as I see, ContextClient contains ExecutionContext instance which can be
set only once at construction, so inheriting from it would require re-creation
of the SensorReadingUpdater instance every time the context is detached,
further, we'd need to introduce a notification mechanism (or just a similar
SensorReadingUpdater  state checking) on the upper level to be aware if the
current SensorReadingUpdater instance must be re-created.
Considering all above the current approach IMO looks simpler.

https://codereview.chromium.org/2644873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:24: m_document
= m_sensorProxy->document();
On 2017/01/25 18:20:06, haraken wrote:
> 
> Can we set m_document in SensorReadingUpdater's constructor?
> 
> Once you make SensorReadingUpdater inherit from ContextClient, it won't be
> needed though.

Done.

[haraken, 2017-01-26 22:24:14]

LGTM

https://codereview.chromium.org/2644873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp (right):

https://codereview.chromium.org/2644873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:36:
m_document->enqueueAnimationFrameTask(std::move(callback));

A final question: Why do you need m_document in the first place? Can we do
something like this?

if (m_sensorProxy->document() && !m_sensorProxy->document()->isDetached()) {
  m_sensorProxy->document()->enqueueAnimationFrameTask(...);
}

[Mikhail, 2017-01-27 11:08:04]

On 2017/01/26 22:24:14, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/2644873002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp
(right):
> 
>
https://codereview.chromium.org/2644873002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:36:
> m_document->enqueueAnimationFrameTask(std::move(callback));
> 
> A final question: Why do you need m_document in the first place? Can we do
> something like this?
> 
> if (m_sensorProxy->document() && !m_sensorProxy->document()->isDetached()) {
>   m_sensorProxy->document()->enqueueAnimationFrameTask(...);
> }

This is added to catch the following case:
1) AF task is scheduled from 'SensorReadingUpdater::start()' and
'm_hasPendingAnimationFrameTask' is set to 'true'
2) document gets detached (e.g. at page refresh) so that onAnimationFrame() is
never called
3) when sensor is started next time (when there is already an updated attached
document) 'm_hasPendingAnimationFrameTask' is still true, so the new AF task
cannot be scheduled..

[Mikhail, 2017-01-27 11:08:38]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 11:08:55]

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, 2017-01-27 11:10:32]

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

[commit-bot: I haz the power, 2017-01-27 11:10:34]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[haraken, 2017-01-27 11:30:33]

LGTM.

I still think it would be tidier to:

- Make SensorProxy inherit from ContextLifecycleObserver
- Make SensorProxy invalidate all Updaters when contextDestroyed gets called

but that wouldn't be a big deal. Sorry about the noise and thanks for being
persistent!

https://codereview.chromium.org/2644873002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp (right):

https://codereview.chromium.org/2644873002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:27: if
(!m_document)

Don't you need to add '|| m_document->isDetached()'?

[Mikhail, 2017-01-27 11:56:57]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 11:57:19]

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

[Mikhail, 2017-01-27 11:58:13]

On 2017/01/27 11:30:33, haraken wrote:
> LGTM.
> 
> I still think it would be tidier to:
> 
> - Make SensorProxy inherit from ContextLifecycleObserver
> - Make SensorProxy invalidate all Updaters when contextDestroyed gets called
> 
> but that wouldn't be a big deal. Sorry about the noise and thanks for being
> persistent!

Thank you for your comments!
> 
>
https://codereview.chromium.org/2644873002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp
(right):
> 
>
https://codereview.chromium.org/2644873002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:27: if
> (!m_document)
> 
> Don't you need to add '|| m_document->isDetached()'?
yeah, done.

[commit-bot: I haz the power, 2017-01-27 13:24:15]

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

[commit-bot: I haz the power, 2017-01-27 13:24:16]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2017-01-27 13:27:00]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2017-01-27 13:27:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, alexander.shalamov@intel.com, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2644873002/#ps120001
(title: "Comments from haraken@. Rebased.")

[commit-bot: I haz the power, 2017-01-27 13:27:18]

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, 2017-01-27 13:31:04]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485523620521140,
"parent_rev": "933ee58984d38b13be78bb9859b5e9109ad565a5", "commit_rev":
"c78e848fc18f203167cb53aedc0e3736bce149b1"}

[commit-bot: I haz the power, 2017-01-27 13:31:39]

Description was changed from

==========
[Sensors] Fix reading updates after page refresh

This patch fixes reading updates after page refresh. The route cause of the
problem was that the outdated document instance was cached in SensorProxy.

BUG=679243
BUG=606766
==========

to

==========
[Sensors] Fix reading updates after page refresh

This patch fixes reading updates after page refresh. The route cause of the
problem was that the outdated document instance was cached in SensorProxy.

BUG=679243
BUG=606766

Review-Url: https://codereview.chromium.org/2644873002
Cr-Commit-Position: refs/heads/master@{#446658}
Committed:
https://chromium.googlesource.com/chromium/src/+/c78e848fc18f203167cb53aedc0e...
==========

[commit-bot: I haz the power, 2017-01-27 13:31:41]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/c78e848fc18f203167cb53aedc0e...