Worker: Fix test flakiness (timeout) on DedicatedWorkerTest

Worker: Fix test flakiness (timeout) on DedicatedWorkerTest

These tests wait for notifications from a worker context using nested loops. If
notifications are consecutively delivered, the balance between enterRunLoop()
and exitRunLoop() is lost and timeout happens.

This CL queues notifications from a worker context and makes sure to keep the
balance.

BUG=655089
Committed: https://crrev.com/d7babd6989862cc47c28874106f6dede10b059f5
Cr-Commit-Position: refs/heads/master@{#436552}

[nhiroki, 2016-12-06 04:14:30]

Description was changed from

==========
Worker: Fix test flakiness (timeout) on Dedicatedworkertest

These tests wait for notifications from a worker context using nested loops. If
notifications are consecutively delivered, the balance between enterRunLoop()
and exitRunLoop() is lost and timeout happens.

This CL queues notifications from a worker context and makes sure to keep the
balance.

BUG=655089
==========

to

==========
Worker: Fix test flakiness (timeout) on DedicatedWorkerTest

These tests wait for notifications from a worker context using nested loops. If
notifications are consecutively delivered, the balance between enterRunLoop()
and exitRunLoop() is lost and timeout happens.

This CL queues notifications from a worker context and makes sure to keep the
balance.

BUG=655089
==========

[nhiroki, 2016-12-06 04:17:03]

The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run

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

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

[nhiroki, 2016-12-06 05:09:07]

nhiroki@chromium.org changed reviewers:
+ falken@chromium.org

[nhiroki, 2016-12-06 05:09:08]

falken@, ptal.

I want this fix because this flakiness seems more likely to occur after
per-frame task scheduler changes.

Thanks.

https://codereview.chromium.org/2553973002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/DedicatedWorkerTest.cpp (left):

https://codereview.chromium.org/2553973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/DedicatedWorkerTest.cpp:356:
workerMessagingProxy()->workerGlobalScopeMayHavePendingActivity());
This expectation may fail if the next notification (PendingActivityReported) is
delivered before this line, so I removed this.

[falken, 2016-12-06 05:29:12]

I guess it's not from this patch, but I don't understand the difference between
"mayHavePendingActivity" and "hasPendingActivity". Somewhat unintuitively, it
looks like hasPendingActivity can be true while mayHavePendingActivity is false:
  return m_unconfirmedMessageCount || m_workerGlobalScopeMayHavePendingActivity;

Could you possibly add a comment to
bool m_workerGlobalScopeMayHavePendingActivity;

and address why it's "may have"?

[commit-bot: I haz the power, 2016-12-06 05:46:14]

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

[commit-bot: I haz the power, 2016-12-06 05:46:15]

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

[nhiroki, 2016-12-06 06:33:00]

On 2016/12/06 05:29:12, falken wrote:
> I guess it's not from this patch, but I don't understand the difference
between
> "mayHavePendingActivity" and "hasPendingActivity". Somewhat unintuitively, it
> looks like hasPendingActivity can be true while mayHavePendingActivity is
false:
>   return m_unconfirmedMessageCount ||
m_workerGlobalScopeMayHavePendingActivity;
> 
> Could you possibly add a comment to
> bool m_workerGlobalScopeMayHavePendingActivity;
> 
> and address why it's "may have"?

Good point. I created a separate CL:
https://codereview.chromium.org/2555673002/

Can you take a look at that CL first?

[nhiroki, 2016-12-06 07:09:17]

The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 07:09:32]

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

[nhiroki, 2016-12-06 07:11:26]

On 2016/12/06 06:33:00, nhiroki wrote:
> On 2016/12/06 05:29:12, falken wrote:
> > I guess it's not from this patch, but I don't understand the difference
> between
> > "mayHavePendingActivity" and "hasPendingActivity". Somewhat unintuitively,
it
> > looks like hasPendingActivity can be true while mayHavePendingActivity is
> false:
> >   return m_unconfirmedMessageCount ||
> m_workerGlobalScopeMayHavePendingActivity;
> > 
> > Could you possibly add a comment to
> > bool m_workerGlobalScopeMayHavePendingActivity;
> > 
> > and address why it's "may have"?
> 
> Good point. I created a separate CL:
> https://codereview.chromium.org/2555673002/
> 
> Can you take a look at that CL first?

Rebased on the CL. Can you take another look at the patchset 2? Thanks!

[falken, 2016-12-06 07:24:31]

lgtm

[commit-bot: I haz the power, 2016-12-06 08:55:33]

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

[commit-bot: I haz the power, 2016-12-06 08:55:34]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-12-06 09:18:16]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-12-06 09:18:31]

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-12-06 09:25:51]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481015896039540,
"parent_rev": "e87aab86664c3e62793fff7c2176b321e740906a", "commit_rev":
"cca2b235fb472ea9871157eec64ac98cffa7e5c2"}

[commit-bot: I haz the power, 2016-12-06 09:26:16]

Description was changed from

==========
Worker: Fix test flakiness (timeout) on DedicatedWorkerTest

These tests wait for notifications from a worker context using nested loops. If
notifications are consecutively delivered, the balance between enterRunLoop()
and exitRunLoop() is lost and timeout happens.

This CL queues notifications from a worker context and makes sure to keep the
balance.

BUG=655089
==========

to

==========
Worker: Fix test flakiness (timeout) on DedicatedWorkerTest

These tests wait for notifications from a worker context using nested loops. If
notifications are consecutively delivered, the balance between enterRunLoop()
and exitRunLoop() is lost and timeout happens.

This CL queues notifications from a worker context and makes sure to keep the
balance.

BUG=655089
==========

[commit-bot: I haz the power, 2016-12-06 09:26:17]

Committed patchset #2 (id:20001)

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

Description was changed from

==========
Worker: Fix test flakiness (timeout) on DedicatedWorkerTest

These tests wait for notifications from a worker context using nested loops. If
notifications are consecutively delivered, the balance between enterRunLoop()
and exitRunLoop() is lost and timeout happens.

This CL queues notifications from a worker context and makes sure to keep the
balance.

BUG=655089
==========

to

==========
Worker: Fix test flakiness (timeout) on DedicatedWorkerTest

These tests wait for notifications from a worker context using nested loops. If
notifications are consecutively delivered, the balance between enterRunLoop()
and exitRunLoop() is lost and timeout happens.

This CL queues notifications from a worker context and makes sure to keep the
balance.

BUG=655089

Committed: https://crrev.com/d7babd6989862cc47c28874106f6dede10b059f5
Cr-Commit-Position: refs/heads/master@{#436552}
==========

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

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