Bug 70328 – Properly suspend/resume DeviceMotion/DeviceOrientation objects

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 111435 [details]
Patch

[WebKit Review Bot]

Attachment 111435 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/Document.cpp:167:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 111435 [details]
Patch

Attachment 111435 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10126232

[Kenneth Rohde Christiansen]

Created attachment 111443 [details]
Patch1

[Early Warning System Bot]

Comment on attachment 111443 [details]
Patch1

Attachment 111443 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10125265

[WebKit Review Bot]

Comment on attachment 111443 [details]
Patch1

Attachment 111443 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10121265

[Gustavo Noronha (kov)]

Comment on attachment 111443 [details]
Patch1

Attachment 111443 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10142102

[Kenneth Rohde Christiansen]

Created attachment 111623 [details]
Patch

[Andreas Kling]

Comment on attachment 111623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111623&action=review

> Source/WebCore/dom/DeviceMotionController.cpp:109
> +    // If we have a client and there are listeners registered,
> +    // then tell the client to start updating

I think the code is sufficiently self-explanatory.

> Source/WebCore/dom/DeviceOrientationController.cpp:111
> +    // If we have a client and there are listeners registered,
> +    // then tell the client to start updating

Ditto.

> Source/WebCore/page/GeolocationController.cpp:92
> +    // If we have a client and there are observers registered,
> +    // then tell the client to start updating

Ditto.

> Source/WebCore/dom/Document.cpp:1928
> +    ScriptExecutionContext::stopActiveDOMObjects();
> +

No need for page() null check here?

[Kenneth Rohde Christiansen]

Created attachment 111630 [details]
Patch

Patch fixing Kling's comments

[zalan]

>void DeviceOrientationController::suspend()
>{
>    if (m_client)
>        m_client->stopUpdating();
>}
>
>void DeviceOrientationController::resume()
>{
>    if (m_client && !m_listeners.isEmpty())
>        m_client->startUpdating();
>}
I missing some explanation here why these 2 functions are not symmetrical.

[Kenneth Rohde Christiansen]

(In reply to comment #11)
> >void DeviceOrientationController::suspend()
> >{
> >    if (m_client)
> >        m_client->stopUpdating();
> >}
> >
> >void DeviceOrientationController::resume()
> >{
> >    if (m_client && !m_listeners.isEmpty())
> >        m_client->startUpdating();
> >}
> I missing some explanation here why these 2 functions are not symmetrical.

They could be symmetrical, as if the set is empty then it is already stopped. But it doesn't really make any difference.

[Simon Hausmann]

Comment on attachment 111630 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111630&action=review

> Source/WebCore/dom/DeviceOrientationController.cpp:78
> +    if (m_client && wasEmpty)

How can client be a null pointer? It looks like it's passed in the constructor and the destructor seems to de-reference it unconditionally.

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> (From update of attachment 111630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111630&action=review
> 
> > Source/WebCore/dom/DeviceOrientationController.cpp:78
> > +    if (m_client && wasEmpty)
> 
> How can client be a null pointer? It looks like it's passed in the constructor and the destructor seems to de-reference it unconditionally.

I'll remove that for now... it is being checked in all other clients, so I should check those as well.

[Simon Hausmann]

Comment on attachment 111630 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 111630 [details]
Patch

Rejecting attachment 111630 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
#2 FAILED at 75.
Hunk #3 succeeded at 1835 (offset -44 lines).
2 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/dom/Document.cpp.rej
patching file Source/WebCore/dom/Document.h
patching file Source/WebCore/dom/ScriptExecutionContext.h
patching file Source/WebCore/page/GeolocationController.cpp
patching file Source/WebCore/page/GeolocationController.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Simon Hausmann', u'--f..." exit_code: 1

Full output: http://queues.webkit.org/results/10177392

[Kenneth Rohde Christiansen]

Created attachment 111746 [details]
Patch (rebaseD)

[WebKit Review Bot]

Comment on attachment 111746 [details]
Patch (rebaseD)

Clearing flags on attachment: 111746

Committed r97964: <http://trac.webkit.org/changeset/97964>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

Created attachment 111760 [details]
Buildfix

[Csaba Osztrogonác]

Comment on attachment 111760 [details]
Buildfix

OMG, I rolled out bad patch. I'm going to land them with this fix.

[Csaba Osztrogonác]

(In reply to comment #21)
> (From update of attachment 111760 [details])
> OMG, I rolled out bad patch. I'm going to land them with this fix.

Patch landed again with fix: http://trac.webkit.org/changeset/97982

[Dirk Pranke]

Reverted r97982 for reason:

crashing in asserts in chromium debug builds

Committed r98050: <http://trac.webkit.org/changeset/98050>

[Dirk Pranke]

Sorry, all, we're still seeing crashes on the chromium debug bots in GeolocationClientMock::stopUpdating for several tests, so I'm rolling this out to see if it fixes things.

See, for example:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20%28dbg%29%281%29/builds/7437/steps/webkit_tests/logs/stdio

ASSERTION FAILED: m_isActive
Backtrace:
	WebCore::GeolocationClientMock::stopUpdating [0x01900E55+53] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\platform\mock\geolocationclientmock.cpp:146)
	WebKit::WebGeolocationClientMock::stopUpdating [0x00A113B5+53] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webkit\chromium\src\webgeolocationclientmock.cpp:102)
	WebKit::GeolocationClientProxy::stopUpdating [0x00A3D8A6+38] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webkit\chromium\src\geolocationclientproxy.cpp:69)
	WebCore::GeolocationController::removeObserver [0x016E53F1+209] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\page\geolocationcontroller.cpp:77)
	WebCore::Geolocation::stopUpdating [0x01587FAD+61] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\page\geolocation.cpp:729)
	WebCore::Geolocation::reset [0x01585E64+100] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\page\geolocation.cpp:266)
	WebCore::Geolocation::disconnectFrame [0x01585E96+22] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\page\geolocation.cpp:274)
	WebCore::Navigator::disconnectFrame [0x0164961E+142] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\page\navigator.cpp:80)
	WebCore::DOMWindow::clear [0x014857B4+612] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\page\domwindow.cpp:516)
	WebCore::Frame::clearDOMWindow [0x0143644D+125] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\page\frame.cpp:591)
	WebCore::FrameLoader::clear [0x01409375+309] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:541)
	WebCore::DocumentWriter::begin [0x014CD367+327] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\documentwriter.cpp:129)
	WebCore::FrameLoader::receivedFirstData [0x0140953E+158] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:580)
	WebCore::FrameLoader::willSetEncoding [0x0140B244+36] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:982)
	WebCore::DocumentWriter::setEncoding [0x014CDC2F+31] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\documentwriter.cpp:240)
	WebCore::DocumentLoader::commitData [0x014AA9A4+116] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\documentloader.cpp:316)
	WebKit::WebFrameImpl::commitDocumentData [0x00A2FE32+50] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webkit\chromium\src\webframeimpl.cpp:1045)
	WebKit::FrameLoaderClientImpl::committedLoad [0x00A7A3A0+160] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webkit\chromium\src\frameloaderclientimpl.cpp:1106)
	WebCore::DocumentLoader::commitLoad [0x014AA8BA+202] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\documentloader.cpp:303)
	WebCore::DocumentLoader::receivedData [0x014AAAF6+70] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\documentloader.cpp:330)
	WebCore::MainResourceLoader::addData [0x01784DDA+58] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\mainresourceloader.cpp:169)
	WebCore::ResourceLoader::didReceiveData [0x0177E399+153] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\resourceloader.cpp:287)
	WebCore::MainResourceLoader::didReceiveData [0x0178600D+301] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\mainresourceloader.cpp:468)
	WebCore::ResourceLoader::didReceiveData [0x0177ECCE+94] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\resourceloader.cpp:441)
	WebCore::ResourceHandleInternal::didReceiveData [0x00A8EE43+147] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webkit\chromium\src\resourcehandle.cpp:140)
	webkit_glue::WebURLLoaderImpl::Context::OnReceivedData [0x00B8C8EF+159] (e:\b\build\slave\webkit_win__dbg__1_\build\src\webkit\glue\weburlloader_impl.cc:584)
	webkit_glue::WebURLLoaderImpl::Context::HandleDataURL [0x00B8CEF0+272] (e:\b\build\slave\webkit_win__dbg__1_\build\src\webkit\glue\weburlloader_impl.cc:670)
	DispatchToMethod<webkit_glue::WebURLLoaderImpl::Context,void (__thiscall webkit_glue::WebURLLoaderImpl::Context::*)(void)> [0x00B8DFBF+15] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\tuple.h:537)
	RunnableMethod<webkit_glue::WebURLLoaderImpl::Context,void (__thiscall webkit_glue::WebURLLoaderImpl::Context::*)(void),Tuple0>::Run [0x00B8DD25+53] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\task.h:374)
	base::subtle::TaskClosureAdapter::Run [0x0231A402+50] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\task.cc:71)
	base::internal::Invoker1<0,base::internal::InvokerStorage1<void (__thiscall base::subtle::TaskClosureAdapter::*)(void),base::subtle::TaskClosureAdapter *>,void (__thiscall base::subtle::TaskClosureAdapter::*)(void)>::DoInvoke [0x0231497D+45] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\bind_internal.h:596)
	base::Callback<void __cdecl(void)>::Run [0x008C350F+47] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\callback.h:269)
	MessageLoop::RunTask [0x0230AD73+515] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:494)
	MessageLoop::DeferOrRunPendingTask [0x0230AF83+51] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:508)
	MessageLoop::DoWork [0x0230B95D+221] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:695)
	base::MessagePumpForUI::DoRunLoop [0x0237C5A4+84] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_pump_win.cc:203)
	base::MessagePumpWin::RunWithDispatcher [0x0237BE22+130] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_pump_win.cc:51)
	base::MessagePumpWin::Run [0x0237C08C+28] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_pump_win.h:64)
	MessageLoop::RunInternal [0x0230A9D7+247] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:450)
	MessageLoop::RunHandler [0x0230A7AE+46] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:424)
	MessageLoop::Run [0x0230A02A+58] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:342)
	webkit_support::RunMessageLoop [0x0090B8BF+15] (e:\b\build\slave\webkit_win__dbg__1_\build\src\webkit\support\webkit_support.cc:402)
	TestShell::waitTestFinished [0x0042E93D+141] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\tools\dumprendertree\chromium\testshellwin.cpp:106)
	TestShell::runFileTest [0x004495CF+479] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\tools\dumprendertree\chromium\testshell.cpp:236)
	runTest [0x0041D65D+733] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\tools\dumprendertree\chromium\dumprendertree.cpp:126)
	main [0x0041CE99+2329] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\tools\dumprendertree\chromium\dumprendertree.cpp:261)
	__tmainCRTStartup [0x00AB6D77+279] (f:\dd\vctools\crt_bld\self_x86\crt\src\crt0.c:266)
	mainCRTStartup [0x00AB6C4F+15] (f:\dd\vctools\crt_bld\self_x86\crt\src\crt0.c:182)
	RegisterWaitForInputIdle [0x7C817077+73]

[Kenneth Rohde Christiansen]

Created attachment 111942 [details]
Patch

Dirk would it be possible for you to test this on Chromium?

[Dirk Pranke]

I can try to test this patch, but I probably won't get to it before Monday. It may be easier to just land it and watch the Chromium bots at http://build.chromium.org/p/chromium.webkit/waterfall .

[Kenneth Rohde Christiansen]

(In reply to comment #26)
> I can try to test this patch, but I probably won't get to it before Monday. It may be easier to just land it and watch the Chromium bots at http://build.chromium.org/p/chromium.webkit/waterfall .

No problem, I will also first look at the patch again tomorrow.

[Kenneth Rohde Christiansen]

Let's leave geolocation out for now as it is not a per document object

[Kenneth Rohde Christiansen]

Created attachment 112474 [details]
Patch

[Simon Hausmann]

Comment on attachment 112474 [details]
Patch

r=me

[Early Warning System Bot]

Comment on attachment 112474 [details]
Patch

Attachment 112474 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10220184

[WebKit Review Bot]

Comment on attachment 112474 [details]
Patch

Attachment 112474 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10213554

[Kenneth Rohde Christiansen]

Created attachment 112480 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 112480 [details]
Patch for landing

Clearing flags on attachment: 112480

Committed r98481: <http://trac.webkit.org/changeset/98481>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.