Bug 188727 - Pointer lock causes abandons documents
Summary: Pointer lock causes abandons documents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-18 16:59 PDT by Simon Fraser (smfr)
Modified: 2022-10-08 14:30 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.33 KB, patch)
2018-09-28 15:31 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (3.51 KB, patch)
2018-09-28 15:36 PDT, Jeremy Jones
simon.fraser: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.94 MB, application/zip)
2018-09-28 17:54 PDT, EWS Watchlist
no flags Details
Patch for landing. (3.81 KB, patch)
2018-12-20 11:42 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2018-08-18 16:59:56 PDT
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*)
Comment 1 Simon Fraser (smfr) 2018-09-07 18:59:15 PDT
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 ]
Comment 2 Radar WebKit Bug Importer 2018-09-07 18:59:36 PDT
<rdar://problem/44248197>
Comment 3 Jeremy Jones 2018-09-28 15:31:03 PDT
Created attachment 351123 [details]
Patch
Comment 4 Simon Fraser (smfr) 2018-09-28 15:35:40 PDT
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?
Comment 5 EWS Watchlist 2018-09-28 15:36:09 PDT
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.
Comment 6 Jeremy Jones 2018-09-28 15:36:38 PDT
Created attachment 351125 [details]
Patch
Comment 7 EWS Watchlist 2018-09-28 17:54:37 PDT
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
Comment 8 EWS Watchlist 2018-09-28 17:54:39 PDT
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
Comment 9 Jeremy Jones 2018-10-17 16:24:27 PDT
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.
Comment 10 Simon Fraser (smfr) 2018-11-08 05:36:36 PST
Did this land?
Comment 11 Jeremy Jones 2018-12-20 11:42:30 PST
Created attachment 357842 [details]
Patch for landing.
Comment 12 WebKit Commit Bot 2018-12-20 15:46:11 PST
Comment on attachment 357842 [details]
Patch for landing.

Clearing flags on attachment: 357842

Committed r239469: <https://trac.webkit.org/changeset/239469>
Comment 13 Ahmad Saleem 2022-10-08 14:30:01 PDT
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!