Fix flaky crashes

Fix flaky crashes.


BUG=528110
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201861

[peria, 2015-09-04 05:29:38]

peria@chromium.org changed reviewers:
+ haraken@chromium.org, timvolodine@chromium.org

[peria, 2015-09-04 05:29:38]

PTL.
Using ASSERT was too aggressive for current code.

[haraken, 2015-09-04 05:32:45]

https://codereview.chromium.org/1329863002/diff/1/Source/core/frame/PlatformE...
File Source/core/frame/PlatformEventDispatcher.cpp (right):

https://codereview.chromium.org/1329863002/diff/1/Source/core/frame/PlatformE...
Source/core/frame/PlatformEventDispatcher.cpp:23: // FIXME: If we can avoid
registering a controller twice, we can change

FIXME => TODO

https://codereview.chromium.org/1329863002/diff/1/Source/core/frame/PlatformE...
Source/core/frame/PlatformEventDispatcher.cpp:25: if
(!m_controllers.contains(controller))

Would't it be better to early-return if the controller exists in the map?

[peria, 2015-09-04 05:41:18]

https://codereview.chromium.org/1329863002/diff/1/Source/core/frame/PlatformE...
File Source/core/frame/PlatformEventDispatcher.cpp (right):

https://codereview.chromium.org/1329863002/diff/1/Source/core/frame/PlatformE...
Source/core/frame/PlatformEventDispatcher.cpp:23: // FIXME: If we can avoid
registering a controller twice, we can change
On 2015/09/04 05:32:45, haraken wrote:
> 
> FIXME => TODO

Done.

https://codereview.chromium.org/1329863002/diff/1/Source/core/frame/PlatformE...
Source/core/frame/PlatformEventDispatcher.cpp:25: if
(!m_controllers.contains(controller))
On 2015/09/04 05:32:45, haraken wrote:
> 
> Would't it be better to early-return if the controller exists in the map?

Done.

[haraken, 2015-09-04 05:43:05]

BTW, is the duplicated registration an expected behavior?

I'm wondering if we should land this CL or fix the call sites.

[peria, 2015-09-04 06:11:06]

On 2015/09/04 05:43:05, haraken wrote:
> BTW, is the duplicated registration an expected behavior?
> 
> I'm wondering if we should land this CL or fix the call sites.

I'm not sure.
In past implementation, addController() was the judgement to avoid duplicate,
so it is no wonder that some controllers are registered twice.
It may include the case that an old controller has not been unregistered
correctly,
and a newly created controller is assigned on the same address and be
registered.

I think it good to fix the flakiness with this CL and to fix the call sites
later.

[haraken, 2015-09-04 06:12:25]

On 2015/09/04 06:11:06, peria wrote:
> On 2015/09/04 05:43:05, haraken wrote:
> > BTW, is the duplicated registration an expected behavior?
> > 
> > I'm wondering if we should land this CL or fix the call sites.
> 
> I'm not sure.
> In past implementation, addController() was the judgement to avoid duplicate,
> so it is no wonder that some controllers are registered twice.
> It may include the case that an old controller has not been unregistered
> correctly,
> and a newly created controller is assigned on the same address and be
> registered.
> 
> I think it good to fix the flakiness with this CL and to fix the call sites
> later.

Sounds reasonable. LGTM.

[peria, 2015-09-04 06:14:00]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-09-04 06:14:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1329863002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1329863002/20001

[peria, 2015-09-04 06:16:16]

Tim,
If you feel this CL strange, feel free to revert or
update it.

[commit-bot: I haz the power, 2015-09-04 09:05:52]

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

[commit-bot: I haz the power, 2015-09-04 09:05:53]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[timvolodine, 2015-09-04 18:07:03]

On 2015/09/04 09:05:53, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Yes this is strange, I suspected with the previous patch that this could be an
issue, but was under the impression it could not happen in current code. From
looking at the bug: does this only happen for the screen orientation api?
Anyways while this is being investigated the fix is lgtm. (the check was present
in the old code as you correctly observed)

[peria, 2015-09-07 03:29:37]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-09-07 03:29:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1329863002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1329863002/20001

[peria, 2015-09-07 03:29:50]

On 2015/09/04 18:07:03, timvolodine wrote:
> On 2015/09/04 09:05:53, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> 
> Yes this is strange, I suspected with the previous patch that this could be an
> issue, but was under the impression it could not happen in current code. From
> looking at the bug: does this only happen for the screen orientation api?
> Anyways while this is being investigated the fix is lgtm. (the check was
present
> in the old code as you correctly observed)

Thank you for the approval.

[commit-bot: I haz the power, 2015-09-07 07:34:45]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201861