Skip to content

Commit 1682be9

Browse files
David Kilzerddkilzer
authored andcommitted
WebPageProxy crash in dispatchSetObscuredContentInsets() and clearTextIndicatorWithAnimation()
<https://bugs.webkit.org/show_bug.cgi?id=309108> <rdar://167427221> Reviewed by Anne van Kesteren. Crashes occur when a `WebPageProxy` object is destroyed when a `WeakPtr` is used to make a method call since it doesn't keep the object alive. Promote unsafe `WeakPtr<WebPageProxy>` usage to `RefPtr<WebPageProxy` to ensure the object stays alive for the duration of the method call. Also promote `WeakPtr<WebProcessProxy>` in completion callbacks where the process object could be destroyed during sandbox extension processing. * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::loadSimulatedRequest): (WebKit::WebPageProxy::loadAlternateHTML): (WebKit::WebPageProxy::reload): (WebKit::WebPageProxy::executeEditCommand): (WebKit::WebPageProxy::contextMenuItemSelected): * Source/WebKit/UIProcess/mac/WebPageProxyMac.mm: (WebKit::WebPageProxy::didPerformDictionaryLookup): (WebKit::WebPageProxy::scheduleSetObscuredContentInsetsDispatch): Originally-landed-as: 305413.397@rapid/safari-7624.2.5.110-branch (79f65b1). rdar://176067210 Canonical link: https://commits.webkit.org/314290@main
1 parent 4c0d424 commit 1682be9

2 files changed

Lines changed: 30 additions & 24 deletions

File tree

Source/WebKit/UIProcess/WebPageProxy.cpp

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2505,8 +2505,11 @@ RefPtr<API::Navigation> WebPageProxy::loadSimulatedRequest(WebCore::ResourceRequ
25052505

25062506
process->markProcessAsRecentlyUsed();
25072507
process->assumeReadAccessToBaseURL(*this, baseURL, [weakProcess = WeakPtr { process }, loadParameters = WTF::move(loadParameters), simulatedResponse = WTF::move(simulatedResponse), webPageID = m_webPageID] () mutable {
2508-
weakProcess->send(Messages::WebPage::LoadSimulatedRequestAndResponse(WTF::move(loadParameters), simulatedResponse), webPageID);
2509-
weakProcess->startResponsivenessTimer();
2508+
RefPtr protectedProcess = weakProcess.get();
2509+
if (!protectedProcess)
2510+
return;
2511+
protectedProcess->send(Messages::WebPage::LoadSimulatedRequestAndResponse(WTF::move(loadParameters), simulatedResponse), webPageID);
2512+
protectedProcess->startResponsivenessTimer();
25102513
});
25112514

25122515
return navigation;
@@ -2568,14 +2571,18 @@ void WebPageProxy::loadAlternateHTML(Ref<WebCore::DataSegment>&& htmlData, const
25682571
] () mutable {
25692572
process->markProcessAsRecentlyUsed();
25702573
process->assumeReadAccessToBaseURLs(*this, { baseURL.string(), unreachableURL.string() }, [weakThis = WeakPtr { *this }, weakProcess = WeakPtr { process }, baseURL, unreachableURL, loadParameters = WTF::move(loadParameters)] () mutable {
2571-
if (!weakThis || !weakProcess)
2574+
RefPtr protectedThis = weakThis.get();
2575+
if (!protectedThis)
2576+
return;
2577+
RefPtr protectedProcess = weakProcess.get();
2578+
if (!protectedProcess)
25722579
return;
25732580
if (baseURL.protocolIsFile())
2574-
weakProcess->addPreviouslyApprovedFileURL(baseURL);
2581+
protectedProcess->addPreviouslyApprovedFileURL(baseURL);
25752582
if (unreachableURL.protocolIsFile())
2576-
weakProcess->addPreviouslyApprovedFileURL(unreachableURL);
2577-
weakThis->send(Messages::WebPage::LoadAlternateHTML(WTF::move(loadParameters)));
2578-
weakProcess->startResponsivenessTimer();
2583+
protectedProcess->addPreviouslyApprovedFileURL(unreachableURL);
2584+
protectedThis->send(Messages::WebPage::LoadAlternateHTML(WTF::move(loadParameters)));
2585+
protectedProcess->startResponsivenessTimer();
25792586
});
25802587
};
25812588

@@ -2635,19 +2642,20 @@ RefPtr<API::Navigation> WebPageProxy::reload(OptionSet<WebCore::ReloadOption> op
26352642
if (!url.isEmpty()) {
26362643
// We may not have an extension yet if back/forward list was reinstated after a WebProcess crash or a browser relaunch
26372644
maybeInitializeSandboxExtensionHandle(protect(legacyMainFrameProcess()), URL { url }, currentResourceDirectoryURL(), true, [weakThis = WeakPtr { *this }, process = WTF::move(process), options = WTF::move(options), sandboxExtensionHandle = WTF::move(sandboxExtensionHandle), navigation](std::optional<SandboxExtension::Handle>&& sandboxExtension) mutable {
2638-
if (!weakThis)
2645+
RefPtr protectedThis = weakThis.get();
2646+
if (!protectedThis)
26392647
return;
26402648
if (sandboxExtension)
26412649
sandboxExtensionHandle = WTF::move(*sandboxExtension);
2642-
weakThis->send(Messages::WebPage::Reload(navigation->navigationID(), options, WTF::move(sandboxExtensionHandle)));
2650+
protectedThis->send(Messages::WebPage::Reload(navigation->navigationID(), options, WTF::move(sandboxExtensionHandle)));
26432651
process->startResponsivenessTimer();
26442652

2645-
if (weakThis->shouldForceForegroundPriorityForClientNavigation())
2646-
weakThis->setClientNavigationActivity(navigation);
2653+
if (protectedThis->shouldForceForegroundPriorityForClientNavigation())
2654+
protectedThis->setClientNavigationActivity(navigation);
26472655

26482656

26492657
#if ENABLE(SPEECH_SYNTHESIS)
2650-
weakThis->resetSpeechSynthesizer();
2658+
protectedThis->resetSpeechSynthesizer();
26512659
#endif
26522660
});
26532661
}
@@ -3834,13 +3842,14 @@ void WebPageProxy::executeEditCommand(const String& commandName, const String& a
38343842

38353843
auto completionHandler = [weakThis = WeakPtr { *this }, commandName, argument, frameID] () mutable {
38363844
static NeverDestroyed<String> ignoreSpellingCommandName(MAKE_STATIC_STRING_IMPL("ignoreSpelling"));
3837-
if (!weakThis)
3845+
RefPtr protectedThis = weakThis.get();
3846+
if (!protectedThis)
38383847
return;
38393848

38403849
if (commandName == ignoreSpellingCommandName)
3841-
++weakThis->m_pendingLearnOrIgnoreWordMessageCount;
3850+
++protectedThis->m_pendingLearnOrIgnoreWordMessageCount;
38423851

3843-
weakThis->sendToProcessContainingFrame(frameID, Messages::WebPage::ExecuteEditCommand(commandName, argument));
3852+
protectedThis->sendToProcessContainingFrame(frameID, Messages::WebPage::ExecuteEditCommand(commandName, argument));
38443853
};
38453854

38463855
if (auto pasteAccessCategory = pasteAccessCategoryForCommand(commandName)) {
@@ -11940,9 +11949,8 @@ void WebPageProxy::contextMenuItemSelected(const WebContextMenuItemData& item, c
1194011949
}
1194111950
auto targetFrameID = focusedOrMainFrame() ? std::optional(focusedOrMainFrame()->frameID()) : std::nullopt;
1194211951
platformDidSelectItemFromActiveContextMenu(item, [weakThis = WeakPtr { *this }, item, targetFrameID] () mutable {
11943-
if (!weakThis)
11944-
return;
11945-
weakThis->sendToProcessContainingFrame(targetFrameID, Messages::WebPage::DidSelectItemFromActiveContextMenu(item));
11952+
if (RefPtr protectedThis = weakThis.get())
11953+
protectedThis->sendToProcessContainingFrame(targetFrameID, Messages::WebPage::DidSelectItemFromActiveContextMenu(item));
1194611954
});
1194711955
}
1194811956

Source/WebKit/UIProcess/mac/WebPageProxyMac.mm

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -347,9 +347,8 @@ static inline bool expectsLegacyImplicitRubberBandControl()
347347
DictionaryLookup::showPopup(dictionaryPopupInfo, protect(pageClient->viewForPresentingRevealPopover()).get(), [this](TextIndicator& textIndicator) {
348348
setTextIndicator(textIndicator, WebCore::TextIndicatorLifetime::Permanent);
349349
}, nullptr, [weakThis = WeakPtr { *this }] {
350-
if (!weakThis)
351-
return;
352-
weakThis->clearTextIndicatorWithAnimation(WebCore::TextIndicatorDismissalAnimation::None);
350+
if (RefPtr protectedThis = weakThis.get())
351+
protectedThis->clearTextIndicatorWithAnimation(WebCore::TextIndicatorDismissalAnimation::None);
353352
});
354353
}
355354
}
@@ -495,9 +494,8 @@ static inline bool expectsLegacyImplicitRubberBandControl()
495494
m_didScheduleSetObscuredContentInsetsDispatch = true;
496495

497496
callOnMainRunLoop([weakThis = WeakPtr { *this }] {
498-
if (!weakThis)
499-
return;
500-
weakThis->dispatchSetObscuredContentInsets();
497+
if (RefPtr protectedThis = weakThis.get())
498+
protectedThis->dispatchSetObscuredContentInsets();
501499
});
502500
}
503501

0 commit comments

Comments
 (0)