[Windows] Fix for Browser IO MessageLoop spinning the CPU.

[Windows] Fix for Browser IO MessageLoop spinning the CPU.

This corrects an integer overflow issue which resulted in the Browser IO
MessageLoop spinning, eating up 100% of a CPU core.  The conversion from
a double to a 32-bit int was sometimes out-of-range: A large positive
double value was being assigned as a negative int value.  Because of
this, MessagePumpWin acted as if a delayed task was past-due for running
and refused to sleep the thread.  Downstream code paths would instead
see the task as "in the future" and then refuse to run it and remove it
from the queue.  This resulted in a "never-wait, never-dequeue" bad
spinning state.

While researching this problem, a number of code points have been
exposed that are erroneously posting delayed tasks with huge delays,
likely due to bad math on 64-bit platforms.  Added DLOG warnings to
whine at developers to avoid this in the future.

See bug for additional details.

BUG=450045
TBR=darin@chromium.org

Committed: https://crrev.com/9caeaa287bca5d34a21d73d103b9af7fd3676936
Cr-Commit-Position: refs/heads/master@{#313238}

[miu, 2015-01-27 04:20:53]

miu@chromium.org changed reviewers:
+ cpu@chromium.org, darin@chromium.org, wfh@chromium.org

[miu, 2015-01-27 04:20:54]

PTAL.  Due to the severity of the bug, I will likely commit TBR once the try
bots are green, and will be happy to follow-up tomorrow with any further
changes.

[miu, 2015-01-27 06:04:23]

Patchset #4 (id:60001) has been deleted

[miu, 2015-01-27 06:14:30]

Patchset #4 (id:80001) has been deleted

[miu, 2015-01-27 06:35:06]

On 2015/01/27 04:20:54, miu wrote:
> PTAL.  Due to the severity of the bug, I will likely commit TBR once the try
> bots are green, and will be happy to follow-up tomorrow with any further
> changes.

Ok, committing TBR now due to severity.  Note that this change HAS been seen by
two developers who were familiar with the problem (from bug 450045).

[miu, 2015-01-27 06:35:12]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2015-01-27 06:35:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/873393004/40001

[commit-bot: I haz the power, 2015-01-27 06:43:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-27 06:44:35]

Patchset 3 (id:??) landed as
https://crrev.com/9caeaa287bca5d34a21d73d103b9af7fd3676936
Cr-Commit-Position: refs/heads/master@{#313238}

[darin (slow to review), 2015-01-27 07:52:39]

https://codereview.chromium.org/873393004/diff/40001/base/message_loop/messag...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/873393004/diff/40001/base/message_loop/messag...
base/message_loop/message_loop.cc:308:
incoming_task_queue_->AddToIncomingQueue(from_here, task, delay, false);
would it be better to move this DLOG_IF to the implementation of
AddToIncomingQueue instead? that way it can be done in one place instead of two?

https://codereview.chromium.org/873393004/diff/40001/base/message_loop/messag...
File base/message_loop/message_pump_win.cc (right):

https://codereview.chromium.org/873393004/diff/40001/base/message_loop/messag...
base/message_loop/message_pump_win.cc:79: (timeout >
std::numeric_limits<int>::max() ?
you didn't want to use the max_int / 2 comparison here?

[miu, 2015-01-27 19:19:46]

Addressed these comments in follow-up change:
https://codereview.chromium.org/878033005/

I'll request code review once trybots (and local desktop) pass testing.


On 2015/01/27 07:52:39, darin wrote:
>
https://codereview.chromium.org/873393004/diff/40001/base/message_loop/messag...
> base/message_loop/message_loop.cc:308:
> incoming_task_queue_->AddToIncomingQueue(from_here, task, delay, false);
> would it be better to move this DLOG_IF to the implementation of
> AddToIncomingQueue instead? that way it can be done in one place instead of
two?

Done.  Good call, as this now warns callers of the MessageLoopProxy post task
methods.

>
https://codereview.chromium.org/873393004/diff/40001/base/message_loop/messag...
> base/message_loop/message_pump_win.cc:79: (timeout >
> std::numeric_limits<int>::max() ?
> you didn't want to use the max_int / 2 comparison here?

No.  The max_int / 2 is just a threshold for the warning logging (per discussion
in bug w/ scottmg@).  Here in MessagePumpWin, we're simply capping the wait time
to the largest value that can be represented within the type.