Have ScreenOrientationController use a timer for async event dispatch again

Have ScreenOrientationController use a timer for async event dispatch again

Essentially revert r193927's switch to using a task to queue handling of
change event dispatch for ScreenOrientationController. Queuing a task
via the execution context / Document is unsafe for this controller object
as its lifetime is that of its frame. Should the frame be detached and
finalized while the Document remains alive, the queued task risks accessing
a freed ScreenOrientationController upon running.

Go back to using a one-shot timer; as these are now task based, the
original motivation for r193927 has additionally fallen away (wean Blink
off from using one-shot shared timers.)

R=mlamouri
BUG=501888
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197466

[sof, 2015-06-19 07:18:58]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org, mlamouri@chromium.org

[sof, 2015-06-19 07:18:58]

please take a look.

Unsafe lifetime assumption made in r193927 between Document and LocalFrame. I
really should have identified it as a problem at the time; sorry.

[mlamouri (slow - plz ping), 2015-06-19 09:54:27]

lgtm with the comment applied

https://codereview.chromium.org/1190293002/diff/1/LayoutTests/screen_orientat...
File
LayoutTests/screen_orientation/screenorientation-detached-notify-no-crash.html
(right):

https://codereview.chromium.org/1190293002/diff/1/LayoutTests/screen_orientat...
LayoutTests/screen_orientation/screenorientation-detached-notify-no-crash.html:23:
var currentType = window.screen.orientation.type;
Could you explain the purpose of that line?

[sof, 2015-06-19 10:57:17]

https://codereview.chromium.org/1190293002/diff/1/LayoutTests/screen_orientat...
File
LayoutTests/screen_orientation/screenorientation-detached-notify-no-crash.html
(right):

https://codereview.chromium.org/1190293002/diff/1/LayoutTests/screen_orientat...
LayoutTests/screen_orientation/screenorientation-detached-notify-no-crash.html:23:
var currentType = window.screen.orientation.type;
On 2015/06/19 09:54:26, Mounir Lamouri wrote:
> Could you explain the purpose of that line?

A bit magic looking; isActiveAndVisible() will only return true if
ScreenOrientationController's m_orientation has been set. Change notifications
aren't sent if it returns false.

[sof, 2015-06-19 10:59:58]

https://codereview.chromium.org/1190293002/diff/1/LayoutTests/screen_orientat...
File
LayoutTests/screen_orientation/screenorientation-detached-notify-no-crash.html
(right):

https://codereview.chromium.org/1190293002/diff/1/LayoutTests/screen_orientat...
LayoutTests/screen_orientation/screenorientation-detached-notify-no-crash.html:23:
var currentType = window.screen.orientation.type;
On 2015/06/19 10:57:17, sof wrote:
> On 2015/06/19 09:54:26, Mounir Lamouri wrote:
> > Could you explain the purpose of that line?
> 
> A bit magic looking; isActiveAndVisible() will only return true if
> ScreenOrientationController's m_orientation has been set. Change notifications
> aren't sent if it returns false.

Added a comment.

[sof, 2015-06-19 11:01:37]

Thanks for the review; I'll get this merged back to M44.

[sof, 2015-06-19 11:01:58]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-06-19 11:01:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1190293002/#ps20001
(title: "add clarifying comment to test")

[commit-bot: I haz the power, 2015-06-19 11:02:12]

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

[haraken, 2015-06-19 11:04:15]

https://codereview.chromium.org/1190293002/diff/20001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/1190293002/diff/20001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:142:
m_dispatchEventTimer.startOneShot(0, FROM_HERE);

Would you help me understand why the new code is safe? Does the Timer heap has
an ability to not fire a Timer for a detached frame?

[sof, 2015-06-19 11:08:27]

https://codereview.chromium.org/1190293002/diff/20001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/1190293002/diff/20001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:142:
m_dispatchEventTimer.startOneShot(0, FROM_HERE);
On 2015/06/19 11:04:15, haraken wrote:
> 
> Would you help me understand why the new code is safe? Does the Timer heap has
> an ability to not fire a Timer for a detached frame?

This controller owns the timer, so it'll be finalized (and stop'ed) along with
it.

[haraken, 2015-06-19 11:11:40]

https://codereview.chromium.org/1190293002/diff/20001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/1190293002/diff/20001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:142:
m_dispatchEventTimer.startOneShot(0, FROM_HERE);
On 2015/06/19 11:08:27, sof wrote:
> On 2015/06/19 11:04:15, haraken wrote:
> > 
> > Would you help me understand why the new code is safe? Does the Timer heap
has
> > an ability to not fire a Timer for a detached frame?
> 
> This controller owns the timer, so it'll be finalized (and stop'ed) along with
> it.

Ah, makes sense. LGTM.

[commit-bot: I haz the power, 2015-06-19 12:19:15]

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