Bug 188727

Summary: Pointer lock causes abandons documents
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: DOMAssignee: Jeremy Jones <jeremyj-wk>
Status: RESOLVED FIXED    
Severity: Normal CC: ahmad.saleem792, ap, cdumez, commit-queue, ews-watchlist, jeremyj-wk, rniwa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
simon.fraser: review+, ews-watchlist: commit-queue-
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Patch for landing. none

Simon Fraser (smfr)
Reported 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*)
Attachments
Patch (3.33 KB, patch)
2018-09-28 15:31 PDT, Jeremy Jones 		no flags
DetailsFormatted DiffDiff
Patch (3.51 KB, patch)
2018-09-28 15:36 PDT, Jeremy Jones 		simon.fraser: review+
ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1 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 ]
Radar WebKit Bug Importer
Comment 2 2018-09-07 18:59:36 PDT
<rdar://problem/44248197>
Jeremy Jones
Comment 3 2018-09-28 15:31:03 PDT
Created attachment 351123 [details] Patch
Simon Fraser (smfr)
Comment 4 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?
EWS Watchlist
Comment 5 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.
Jeremy Jones
Comment 6 2018-09-28 15:36:38 PDT
Created attachment 351125 [details] Patch
EWS Watchlist
Comment 7 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
EWS Watchlist
Comment 8 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
Jeremy Jones
Comment 9 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.
Simon Fraser (smfr)
Comment 10 2018-11-08 05:36:36 PST
Did this land?
Jeremy Jones
Comment 11 2018-12-20 11:42:30 PST
Created attachment 357842 [details] Patch for landing.
WebKit Commit Bot
Comment 12 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>
Ahmad Saleem
Comment 13 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!
Note You need to log in before you can comment on or make changes to this bug.