Skip to content

Commit

Permalink
[Site Isolation] Null pointer dereferences in ServicesOverlayController
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=264005
rdar://117767532

Reviewed by Alex Christensen.

We should early return instead of crash when the main frame is being hosted in another process.

This probably still isn't the correct behavior and will need to be fixed later.

* Source/WebCore/page/mac/ServicesOverlayController.h:
* Source/WebCore/page/mac/ServicesOverlayController.mm:
(WebCore::ServicesOverlayController::remainingTimeUntilHighlightShouldBeShown const):
(WebCore::ServicesOverlayController::buildPhoneNumberHighlights):
(WebCore::ServicesOverlayController::buildSelectionHighlight):
(WebCore::ServicesOverlayController::mouseEvent):
(WebCore::ServicesOverlayController::handleClick):
(WebCore::ServicesOverlayController::mainFrame const): Deleted.

Canonical link: https://commits.webkit.org/270275@main
  • Loading branch information
charliewolfe committed Nov 6, 2023
1 parent 58df23f commit 7413ad1
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 14 deletions.
1 change: 0 additions & 1 deletion Source/WebCore/page/mac/ServicesOverlayController.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ class ServicesOverlayController : private DataDetectorHighlightClient, private P

Vector<SimpleRange> telephoneNumberRangesForFocusedFrame();

LocalFrame& mainFrame() const;
Page& page() const { return m_page; }

Page& m_page;
Expand Down
39 changes: 26 additions & 13 deletions Source/WebCore/page/mac/ServicesOverlayController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -278,11 +278,15 @@ static void compactRectsWithGapRects(Vector<LayoutRect>& rects, const Vector<Gap
if (!highlight)
return 0_s;

RefPtr localMainFrame = dynamicDowncast<LocalFrame>(m_page.mainFrame());
if (!localMainFrame)
return 0_s;

Seconds minimumTimeUntilHighlightShouldBeShown = 200_ms;
if (CheckedRef(m_page.focusController())->focusedOrMainFrame().selection().selection().isContentEditable())
minimumTimeUntilHighlightShouldBeShown = 1_s;

bool mousePressed = mainFrame().eventHandler().mousePressed();
bool mousePressed = localMainFrame->eventHandler().mousePressed();

// Highlight hysteresis is only for selection services, because telephone number highlights are already much more stable
// by virtue of being expanded to include the entire telephone number. However, we will still avoid highlighting
Expand Down Expand Up @@ -332,8 +336,12 @@ static void compactRectsWithGapRects(Vector<LayoutRect>& rects, const Vector<Gap

void ServicesOverlayController::buildPhoneNumberHighlights()
{
RefPtr localMainFrame = dynamicDowncast<LocalFrame>(m_page.mainFrame());
if (!localMainFrame)
return;

Vector<SimpleRange> phoneNumberRanges;
for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) {
for (Frame* frame = &m_page.mainFrame(); frame; frame = frame->tree().traverseNext()) {
auto* localFrame = dynamicDowncast<LocalFrame>(frame);
if (!localFrame)
continue;
Expand All @@ -350,7 +358,7 @@ static void compactRectsWithGapRects(Vector<LayoutRect>& rects, const Vector<Gap

HashSet<RefPtr<DataDetectorHighlight>> newPotentialHighlights;

auto& mainFrameView = *mainFrame().view();
auto& mainFrameView = *localMainFrame->view();

for (auto& range : phoneNumberRanges) {
// FIXME: This makes a big rect if the range extends from the end of one line to the start of the next. Handle that case better?
Expand Down Expand Up @@ -390,10 +398,14 @@ static void compactRectsWithGapRects(Vector<LayoutRect>& rects, const Vector<Gap
if (!PAL::isDataDetectorsFrameworkAvailable())
return;

RefPtr localMainFrame = dynamicDowncast<LocalFrame>(m_page.mainFrame());
if (!localMainFrame)
return;

HashSet<RefPtr<DataDetectorHighlight>> newPotentialHighlights;

if (auto selectionRange = m_page.selection().firstRange()) {
auto* mainFrameView = mainFrame().view();
RefPtr mainFrameView = localMainFrame->view();
if (!mainFrameView)
return;

Expand Down Expand Up @@ -566,7 +578,11 @@ static void compactRectsWithGapRects(Vector<LayoutRect>& rects, const Vector<Gap

bool ServicesOverlayController::mouseEvent(PageOverlay&, const PlatformMouseEvent& event)
{
m_mousePosition = mainFrame().view()->windowToContents(event.position());
RefPtr localMainFrame = dynamicDowncast<LocalFrame>(m_page.mainFrame());
if (!localMainFrame)
return false;

m_mousePosition = localMainFrame->view()->windowToContents(event.position());

bool mouseIsOverActiveHighlightButton = false;
determineActiveHighlight(mouseIsOverActiveHighlightButton);
Expand Down Expand Up @@ -629,7 +645,11 @@ static void compactRectsWithGapRects(Vector<LayoutRect>& rects, const Vector<Gap

void ServicesOverlayController::handleClick(const IntPoint& clickPoint, DataDetectorHighlight& highlight)
{
auto* frameView = mainFrame().view();
RefPtr localMainFrame = dynamicDowncast<LocalFrame>(m_page.mainFrame());
if (!localMainFrame)
return;

RefPtr frameView = localMainFrame->view();
if (!frameView)
return;

Expand All @@ -645,13 +665,6 @@ static void compactRectsWithGapRects(Vector<LayoutRect>& rects, const Vector<Gap
m_page.chrome().client().handleTelephoneNumberClick(plainText(highlight.range()), windowPoint, frameView->contentsToWindow(CheckedRef(m_page.focusController())->focusedOrMainFrame().editor().firstRectForRange(highlight.range())));
}

LocalFrame& ServicesOverlayController::mainFrame() const
{
auto* localMainFrame = dynamicDowncast<LocalFrame>(m_page.mainFrame());
ASSERT(localMainFrame);
return *localMainFrame;
}

} // namespace WebKit

#endif // (ENABLE(SERVICE_CONTROLS) || ENABLE(TELEPHONE_NUMBER_DETECTION)) && PLATFORM(MAC)

0 comments on commit 7413ad1

Please sign in to comment.