Bug 188727 – Pointer lock causes abandons documents

• Component: DOM
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

fast/shadow-dom/pointerlockelement-in-slot.html abandons its document (see bug 186214) because PointerLockController::didAcquirePointerLock holds onto Documents:

Backtrace for token 1362
1   0x2020a6a67 WebCore::Node::ref()
2   0x20137af21 unsigned int WTF::refIfNotNull<WebCore::Document>(WebCore::Document*)
3   0x20137aed8 WTF::RefPtr<WebCore::Document, WTF::DumbPtrTraits<WebCore::Document> >::RefPtr(WebCore::Document*)
4   0x20137adcd WTF::RefPtr<WebCore::Document, WTF::DumbPtrTraits<WebCore::Document> >::RefPtr(WebCore::Document*)
5   0x202697393 WTF::RefPtr<WebCore::Document, WTF::DumbPtrTraits<WebCore::Document> >::operator=(WebCore::Document*)
6   0x202a3f004 WebCore::PointerLockController::didAcquirePointerLock()
7   0x10dbc40a5 WebKit::WebPage::didAcquirePointerLock()
8   0x10dc46a6e void IPC::callMemberFunctionImpl<WebKit::WebPage, void (WebKit::WebPage::*)(), std::__1::tuple<> >(WebKit::WebPage*, void (WebKit::WebPage::*)(), std::__1::tuple<>&&, std::__1::integer_sequence<unsigned long>)
9   0x10dc469e0 void IPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(), std::__1::tuple<>, std::__1::integer_sequence<unsigned long> >(std::__1::tuple<>&&, WebKit::WebPage*, void (WebKit::WebPage::*)())
10  0x10dc406e1 void IPC::handleMessage<Messages::WebPage::DidAcquirePointerLock, WebKit::WebPage, void (WebKit::WebPage::*)()>(IPC::Decoder&, WebKit::WebPage*, void (WebKit::WebPage::*)())
11  0x10dc2fab6 WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::Decoder&)
12  0x10dbb84ae WebKit::WebPage::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
13  0x10d730a0c IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&)
14  0x10de2990d WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
15  0x10d72bb5c IPC::Connection::dispatchMessage(IPC::Decoder&)
16  0x10d71e6cd IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
17  0x10d71e091 IPC::Connection::SyncMessageState::dispatchMessages(IPC::Connection*)

[Simon Fraser (smfr)]

These tests are all leaky:
pointer-lock/lock-lost-on-esc-in-fullscreen.html [ Leak ]
pointer-lock/locked-element-removed-from-dom.html [ Leak Pass ]
pointer-lock/mouse-event-delivery.html [ Leak Pass ]

[Radar WebKit Bug Importer]

<rdar://problem/44248197>

[Jeremy Jones]

Created attachment 351123 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 351123 [details]
Patch

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

You can run tests with --world-leaks to test this.

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests (OOPS!).

More words please.

> Source/WebCore/page/PointerLockController.h:72
> +    WeakPtr<Document> m_documentOfRemovedElementWhileWaitingForUnlock;
> +    WeakPtr<Document> m_documentAllowedToRelockWithoutUserGesture;

The names of these suggests that they should get cleared later, so what caused the leaks?

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


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

[Jeremy Jones]

Created attachment 351125 [details]
Patch

[EWS Watchlist]

Comment on attachment 351125 [details]
Patch

Attachment 351125 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9388242

New failing tests:
media/range-extract-contents-crash.html

[EWS Watchlist]

Created attachment 351151 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Jeremy Jones]

Comment on attachment 351123 [details]
Patch

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

>> Source/WebCore/page/PointerLockController.h:72
>> +    WeakPtr<Document> m_documentAllowedToRelockWithoutUserGesture;
> 
> The names of these suggests that they should get cleared later, so what caused the leaks?

The behavior of PointerLockController::documentDetached should be changed to clear m_documentAllowedToRelockWithoutUserGesture, since there is no point in relocating an element in a document that has been removed.

Maybe we  should keep around m_documentOfRemovedElementWhileWaitingForUnlock after documentDetached  so we know how to handle responses coming from the page. If this is weak, it may still not fulfill that purpose. My guess is that it may not be necessary in that case, I need to confirm with a test.

[Simon Fraser (smfr)]

Did this land?

[Jeremy Jones]

Created attachment 357842 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 357842 [details]
Patch for landing.

Clearing flags on attachment: 357842

Committed r239469: <https://trac.webkit.org/changeset/239469>

[Ahmad Saleem]

This landed and didn't backed out (while using BugID to search on Webkit GitHub):

Commit - https://github.com/WebKit/WebKit/commit/afef071f0dfd605f6f97238cebe6fadfdfbfed57

Marking this as "RESOLVED FIXED". Thanks!